enforce boot-from-volume via flavor #190

mariusleu · 2021-03-02T15:12:32Z

Booting from volume can now be enforced via flavor.
A flavor must have extra property boot_from_volume=true to
enable this.
It works by enforcing a boot_index=0 block device mapping based on
the source image, like the user would have requested it.
If the user specifies a root volume explicitly, this enforcement
will be skipped.

grandchild

See some stylistic comments below:

grandchild · 2021-03-12T22:17:33Z

nova/tests/unit/compute/test_compute.py

@@ -8774,6 +8774,81 @@ def test_create_instance_associates_security_groups(self):
        self.assertEqual(1, len(reqspec.security_groups))
        self.assertEqual(group.name, reqspec.security_groups[0].name)

+    def test_create_instance_enforced_bfv(self):
+        # Tests that, if a user creates a instance from an image,


Suggested change

# Tests that, if a user creates a instance from an image,

# Tests that, if a user creates an instance from an image,

grandchild · 2021-03-12T22:30:09Z

nova/compute/api.py

+                return
+            elif (root_bdm.get('destination_type') == 'local' and
+                  root_bdm.get('source_type') == 'image'):
+                # In this case overwrite the image mapping to a volume.


This comment feels redundant and/or possibly misleading. I guess it's supposed to describe what's happening in all the rest of the function, but it looks like it's only describing the next line.

The code does describe itself well enough without this comment.

The other comment above is not quite as obvious, but the early return is a pretty strong indicator of "we're done early here". Personally, I'd also remove it.

grandchild · 2021-03-12T22:36:58Z

nova/compute/api.py

+            elif (root_bdm.get('destination_type') == 'local' and
+                  root_bdm.get('source_type') == 'image'):


I'd switch the order here. Source -> Destination.

Or even refactor out into a separate variable or function that describes what this combination of conditions actually means. I read in the docs that image -> local is special for booting directly from a Glance image(?), and this could be made more explicit here.

grandchild · 2021-03-12T22:56:47Z

Two other questions:

Do I read the code correctly, in that the enforcing of the BFV is only enforced in the special case that the request asks for boot-from-glance? The docs read like there's a bunch of other options when doing block device mappings. Do we not use all the others? If we do, then boot_from_volume=true reads a bit more powerful than it actually is.
How does this change relate to this spec? From what I can tell they're not quite the same, but what is the difference in terms of the problem-to-solve, or the solution?

joker-at-work · 2021-03-26T11:23:52Z

2. How does this change relate to this spec? From what I can tell they're not quite the same, but what is the difference in terms of the problem-to-solve, or the solution?

I skimmed through the spec and from the problem-to-solve it seems to be the same. The spec is just not as restrictive as we want to be. We want to *force` BFV while the spec wants to default to BFV but allow the user to override it.

joker-at-work · 2021-03-26T13:14:14Z

Do I read the code correctly, in that the enforcing of the BFV is only enforced in the special case that the request asks for boot-from-glance? The docs read like there's a bunch of other options when doing block device mappings. Do we not use all the others? If we do, then boot_from_volume=true reads a bit more powerful than it actually is.

The point of this is prohibiting use of ephemeral storage, because we will not have any. Therefore, anything having the destination "local" needs to be prohibited. In the docs you link, I only see 2 options that don't have volume as destination. Those two need to be prohibited.

joker-at-work · 2021-04-20T09:00:37Z

nova/tests/unit/compute/test_compute.py

+                                                read_deleted="yes"):
+            instance_type = orig_get_flavor_by_flavor_id(flavor_id,
+                                                                ctxt,
+                                                                read_deleted)


These lines don't look like they're indented correctly. Is this only github?

Yes, I think it is a Github issue. pep8 passes.

joker-at-work · 2021-04-20T09:06:40Z

nova/compute/api.py

+            elif (root_bdm.get('source_type') == 'image' and
+                  root_bdm.get('destination_type') == 'local'):


As Jakob pointed out, there could be a blank disk/swap disk added via blank -> local. We don't support ephemeral storage and thus also cannot support that case. Anything requesting destination_type == 'local' needs to be prohibited.

joker-at-work · 2021-04-20T09:13:18Z

nova/compute/api.py

+            self._enforce_boot_from_volume(image_meta, instance_type,
+                                           block_device_mapping)


Doesn't the above-called block_device.from_legacy_mapping() already add the image-bdm?

nova/nova/block_device.py

Lines 355 to 356 in 5b05204

if not volume_backed and image_uuid:

image_bdm = create_image_bdm(image_uuid, boot_index=0)

The flavor would then be unusable ...

Oh, right. This worked well initially, when the image mapping was silently overwritten to a volume mapping.

But now since we throw an error, it will indeed make it unusable. I'll revisit this.

Please have another look. Thanks.

Booting from volume can now be enforced via flavor. A flavor must have extra property `boot_from_volume=true` to enable this. It works by enforcing a boot_index=0 block device mapping based on the source image, like the user would have requested it. If the user specifies a root volume explicitly, this enforcement will be skipped.

mariusleu requested review from grandchild, GlaciErrDev and joker-at-work March 2, 2021 15:12

grandchild requested changes Mar 12, 2021

View reviewed changes

mariusleu force-pushed the bfv_flavors branch from b788213 to 1e9f565 Compare April 13, 2021 11:29

mariusleu requested a review from grandchild April 13, 2021 11:29

mariusleu force-pushed the bfv_flavors branch from 1e9f565 to f76078f Compare April 20, 2021 08:16

joker-at-work reviewed Apr 20, 2021

View reviewed changes

mariusleu force-pushed the bfv_flavors branch 2 times, most recently from 511b92c to 949dfee Compare April 23, 2021 09:29

mariusleu force-pushed the bfv_flavors branch from 949dfee to e652877 Compare April 23, 2021 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enforce boot-from-volume via flavor #190

enforce boot-from-volume via flavor #190

mariusleu commented Mar 2, 2021

grandchild left a comment

grandchild Mar 12, 2021

grandchild Mar 12, 2021

grandchild Mar 12, 2021

grandchild commented Mar 12, 2021

joker-at-work commented Mar 26, 2021

joker-at-work commented Mar 26, 2021

joker-at-work Apr 20, 2021

mariusleu Apr 20, 2021

joker-at-work Apr 20, 2021

joker-at-work Apr 20, 2021

mariusleu Apr 20, 2021

mariusleu Apr 23, 2021

	# Tests that, if a user creates a instance from an image,
	# Tests that, if a user creates an instance from an image,

		elif (root_bdm.get('destination_type') == 'local' and
		root_bdm.get('source_type') == 'image'):

		elif (root_bdm.get('source_type') == 'image' and
		root_bdm.get('destination_type') == 'local'):

		self._enforce_boot_from_volume(image_meta, instance_type,
		block_device_mapping)

	if not volume_backed and image_uuid:
	image_bdm = create_image_bdm(image_uuid, boot_index=0)

enforce boot-from-volume via flavor #190

Are you sure you want to change the base?

enforce boot-from-volume via flavor #190

Conversation

mariusleu commented Mar 2, 2021

grandchild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandchild commented Mar 12, 2021

joker-at-work commented Mar 26, 2021

joker-at-work commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment