From 44995b430a604ec24e56f49f9dde27c65ed8cc45 Mon Sep 17 00:00:00 2001 From: "zhong.zhou" Date: Wed, 17 Jul 2024 18:29:46 +0800 Subject: [PATCH 1/3] nova-manage: modify image properties in request_spec At present, we can modify the properties in the instance system_metadata through the sub command image_property of nova-manage, but there may be inconsistencies between their values and those in request_specs. And the migration is based on request_specs, so the same image properties are also written to request_specs. Closes-Bug: 2078999 Change-Id: Id36ecd022cb6f7f9a0fb131b0d202b79715870a9 (cherry picked from commit 2a1fad41453ca7ce15b1cd9b517055c4ccdd12cf) (cherry picked from commit ebae97c62f1af6b3b9f6da2abfa920d6528ddb1b) (cherry picked from commit ee30457accabcea10a62652d14d2cf08a6d57ac0) (cherry picked from commit 3fe5c69b73f01a95fa6df017ea0557298fd6126c) --- nova/cmd/manage.py | 10 ++++++++-- nova/tests/unit/cmd/test_manage.py | 14 ++++++++++++-- ...mage-property-bug-2078999-c493fc259d316c24.yaml | 8 ++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/nova-manage-image-property-bug-2078999-c493fc259d316c24.yaml diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index 08b8ebb3104..5b655be35f2 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -3258,9 +3258,10 @@ def _validate_image_properties(self, image_properties): # Return the dict so we can update the instance system_metadata return image_properties - def _update_image_properties(self, instance, image_properties): + def _update_image_properties(self, ctxt, instance, image_properties): """Update instance image properties + :param ctxt: nova.context.RequestContext :param instance: The instance to update :param image_properties: List of image properties and values to update """ @@ -3284,8 +3285,13 @@ def _update_image_properties(self, instance, image_properties): for image_property, value in image_properties.items(): instance.system_metadata[f'image_{image_property}'] = value + request_spec = objects.RequestSpec.get_by_instance_uuid( + ctxt, instance.uuid) + request_spec.image = instance.image_meta + # Save and return 0 instance.save() + request_spec.save() return 0 @action_description(_( @@ -3320,7 +3326,7 @@ def set(self, instance_uuid=None, image_properties=None): instance = objects.Instance.get_by_uuid( cctxt, instance_uuid, expected_attrs=['system_metadata']) return self._update_image_properties( - instance, image_properties) + ctxt, instance, image_properties) except ValueError as e: print(str(e)) return 6 diff --git a/nova/tests/unit/cmd/test_manage.py b/nova/tests/unit/cmd/test_manage.py index 10c1a77c948..7c44bc6e8fb 100644 --- a/nova/tests/unit/cmd/test_manage.py +++ b/nova/tests/unit/cmd/test_manage.py @@ -4100,6 +4100,8 @@ def test_show_image_properties_unknown_failure( image_property='hw_disk_bus') self.assertEqual(1, ret, 'return code') + @mock.patch('nova.objects.RequestSpec.save') + @mock.patch('nova.objects.RequestSpec.get_by_instance_uuid') @mock.patch('nova.objects.Instance.get_by_uuid') @mock.patch('nova.context.target_cell') @mock.patch('nova.objects.Instance.save') @@ -4108,7 +4110,8 @@ def test_show_image_properties_unknown_failure( @mock.patch('nova.context.get_admin_context', new=mock.Mock(return_value=mock.sentinel.ctxt)) def test_set_image_properties( - self, mock_instance_save, mock_target_cell, mock_get_instance + self, mock_instance_save, mock_target_cell, mock_get_instance, + mock_get_request_spec, mock_request_spec_save ): mock_target_cell.return_value.__enter__.return_value = \ mock.sentinel.cctxt @@ -4117,9 +4120,11 @@ def test_set_image_properties( vm_state=obj_fields.InstanceState.STOPPED, system_metadata={ 'image_hw_disk_bus': 'virtio', - } + }, + image_ref='' ) mock_get_instance.return_value = instance + mock_get_request_spec.return_value = objects.RequestSpec() ret = self.commands.set( instance_uuid=uuidsentinel.instance, image_properties=['hw_cdrom_bus=sata'] @@ -4136,7 +4141,12 @@ def test_set_image_properties( instance.system_metadata.get('image_hw_disk_bus'), 'image_hw_disk_bus' ) + image_props = mock_get_request_spec.return_value.image.properties + self.assertEqual('sata', image_props.get('hw_cdrom_bus')) + self.assertEqual('virtio', image_props.get('hw_disk_bus')) + mock_instance_save.assert_called_once() + mock_request_spec_save.assert_called_once() @mock.patch('nova.objects.Instance.get_by_uuid') @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid', diff --git a/releasenotes/notes/nova-manage-image-property-bug-2078999-c493fc259d316c24.yaml b/releasenotes/notes/nova-manage-image-property-bug-2078999-c493fc259d316c24.yaml new file mode 100644 index 00000000000..03123855e0e --- /dev/null +++ b/releasenotes/notes/nova-manage-image-property-bug-2078999-c493fc259d316c24.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Before the `Bug 2078999 `_ was fixed, + the ``nova-manage image_property set`` command would update the image properties + embedded in the instance but would not update the ones in the request specs. This + led to an unexpected rollback of the image properties that were updated by the + command after an instance migration. From 26e815dbe77006c97f24a9e32a7ed5b1e9612538 Mon Sep 17 00:00:00 2001 From: Zhang Hua Date: Fri, 24 May 2024 15:49:12 +0800 Subject: [PATCH 2/3] Fix deepcopy usage for BlockDeviceMapping in get_root_info The method get_root_info sometimes receives a BlockDeviceMapping object, which lacks a copy method. The previous code assumed root_bdm was always an instance of DriverBlockDevice, a subclass of dict that supports the copy() method. However, during testing, it was discovered that root_bdm could also be a BlockDeviceMapping object, which does not have a copy method. To address this, the change replaces the copy() call with copy.deepcopy() according to the suggestion in the comment [1], which works for both BlockDeviceMapping and DriverBlockDevice instances. The deepcopy method is supported because oslo.versionedobjects implements the __deepcopy__ method. This change ensures the function handles both object types correctly, preventing the AttributeError observed during testing. [1] https://review.opendev.org/c/openstack/nova/+/909611/4/nova/virt/libvirt/blockinfo.py Change-Id: I9432718586855ff57e8e6a5cae064e0685dd01e8 (cherry picked from commit 065bf99fc79a3d086e1859f9542afaafa8c3bf00) Signed-off-by: Zhang Hua (cherry picked from commit 9ff4953954dddf9985698869cbe9ff5d00857210) (cherry picked from commit 608a73ee68e6036188b3d2087fddfe8209f50260) (cherry picked from commit 5b57acbbda8224a5afc26b90b09c0a24b4dc3129) --- .../tests/unit/virt/libvirt/test_blockinfo.py | 44 +++++++++++++++++++ nova/virt/libvirt/blockinfo.py | 8 ++-- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_blockinfo.py b/nova/tests/unit/virt/libvirt/test_blockinfo.py index 5a0dbb40ce3..4e245093af6 100644 --- a/nova/tests/unit/virt/libvirt/test_blockinfo.py +++ b/nova/tests/unit/virt/libvirt/test_blockinfo.py @@ -1289,6 +1289,7 @@ def test_get_root_info_no_bdm_empty_image_meta(self, mock_find_dev): @mock.patch('nova.virt.libvirt.blockinfo.get_info_from_bdm') def test_get_root_info_bdm(self, mock_get_info): + # call get_root_info() with DriverBlockDevice instance = objects.Instance(**self.test_instance) image_meta = objects.ImageMeta.from_dict(self.test_image_meta) root_bdm = {'mount_device': '/dev/vda', @@ -1318,6 +1319,49 @@ def test_get_root_info_bdm(self, mock_get_info): {}, 'virtio') mock_get_info.reset_mock() + @mock.patch('nova.virt.libvirt.blockinfo.get_info_from_bdm') + def test_get_root_info_bdm_with_deepcopy(self, mock_get_info): + # call get_root_info() with BlockDeviceMapping + instance = objects.Instance(**self.test_instance) + image_meta = objects.ImageMeta.from_dict(self.test_image_meta) + root_bdm = objects.BlockDeviceMapping(self.context, + **fake_block_device.FakeDbBlockDeviceDict( + {'id': 3, 'instance_uuid': uuids.instance, + 'device_name': '/dev/sda', + 'source_type': 'blank', + 'destination_type': 'local', + 'device_type': 'cdrom', + 'disk_bus': 'virtio', + 'volume_id': 'fake-volume-id-1', + 'boot_index': 0})) + # No root_device_name + blockinfo.get_root_info( + instance, 'kvm', image_meta, root_bdm, 'virtio', 'ide') + mock_get_info.assert_called_once_with( + instance, 'kvm', image_meta, root_bdm, {}, 'virtio') + mock_get_info.reset_mock() + # Both device names + blockinfo.get_root_info( + instance, 'kvm', image_meta, root_bdm, 'virtio', 'scsi', + root_device_name='/dev/sda') + mock_get_info.assert_called_once_with( + instance, 'kvm', image_meta, root_bdm, {}, 'virtio') + mock_get_info.reset_mock() + # Missing device names + original_bdm = copy.deepcopy(root_bdm) + root_bdm.device_name = '' + blockinfo.get_root_info( + instance, 'kvm', image_meta, root_bdm, 'virtio', 'scsi', + root_device_name='/dev/sda') + mock_get_info.assert_called_with( + instance, 'kvm', image_meta, mock.ANY, {}, 'virtio') + actual_call = mock_get_info.call_args + _, _, _, actual_bdm, _, _ = actual_call[0] + self.assertEqual( + original_bdm.obj_to_primitive(), + actual_bdm.obj_to_primitive() + ) + def test_get_boot_order_simple(self): disk_info = { 'disk_bus': 'virtio', diff --git a/nova/virt/libvirt/blockinfo.py b/nova/virt/libvirt/blockinfo.py index 4efc6fbaeb1..4d03dc38ea2 100644 --- a/nova/virt/libvirt/blockinfo.py +++ b/nova/virt/libvirt/blockinfo.py @@ -69,6 +69,7 @@ """ +import copy import itertools import operator @@ -444,12 +445,13 @@ def get_root_info(instance, virt_type, image_meta, root_bdm, 'dev': block_device.strip_dev(root_device_name), 'boot_index': '1'} + root_bdm_copy = root_bdm if not get_device_name(root_bdm) and root_device_name: - root_bdm = root_bdm.copy() - root_bdm['device_name'] = root_device_name + root_bdm_copy = copy.deepcopy(root_bdm) + root_bdm_copy['device_name'] = root_device_name return get_info_from_bdm( - instance, virt_type, image_meta, root_bdm, {}, disk_bus, + instance, virt_type, image_meta, root_bdm_copy, {}, disk_bus, ) From 75497b0ba61e85afed3f4a0660c20b2d13cb2ae0 Mon Sep 17 00:00:00 2001 From: Elod Illes Date: Tue, 13 May 2025 15:06:27 +0200 Subject: [PATCH 3/3] [tool] Fix backport validator for non-SLURP non-SLURP branches are EOL'd in case they reach their end of maintained phase. This could produce a situation when a patch is merged in a non-SLURP branch that was deleted in the meantime and it's further backports fail on gate with backport validator as the hash of the non-SLURP version of the patch is not on any branch. This patch fixes the above issue as follows: in case a hash is not found on any branch, then it checks if it can be found under any *-eol tag and only fails if there is not found either. Change-Id: I56705bce8ee4354cd5cb1577a520c2d1c525f57b (cherry picked from commit e383b465458969ec9271013f2b9e9f24b8225418) (cherry picked from commit 8b0ae7243f8d581e1e73f0b9dcccf710666d931f) (cherry picked from commit 88e49dd65c58536ba8dd39ab7cfde669a433f3f6) (cherry picked from commit db438e55e62599faf2931d0992a5c7689ade3610) (cherry picked from commit 0fdd21fb4ba4d8c0f5ad45cb8bf1d2698c382c6d) --- tools/check-cherry-picks.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/check-cherry-picks.sh b/tools/check-cherry-picks.sh index 74887a9178b..fe75867e59f 100755 --- a/tools/check-cherry-picks.sh +++ b/tools/check-cherry-picks.sh @@ -26,8 +26,11 @@ branches+="" for hash in $hashes; do branch=$(git branch -a --contains "$hash" 2>/dev/null| grep -oE '(master|stable/[a-z0-9.]+|unmaintained/[a-z0-9.]+)') if [ $? -ne 0 ]; then - echo "Cherry pick hash $hash not on any master, stable or unmaintained branches" - exit 1 + branch=$(git tag --contains "$hash" 2>/dev/null| grep -oE '([0-9.]+-eol)') + if [ $? -ne 0 ]; then + echo "Cherry pick hash $hash not on any master, stable, unmaintained or EOL'd branches" + exit 1 + fi fi branches+=" $branch" checked=$(($checked + 1))