From 17d60107ca73547593a8cb9b50ba7e94d0cb5d33 Mon Sep 17 00:00:00 2001 From: Marius Leustean Date: Tue, 5 Nov 2024 15:26:51 +0200 Subject: [PATCH] [vmware] Improve the order of attaching/detaching volumes Currently the ordering is done only by sorting the mount_device, which might lead into incorrect index of the boot device in the OS. With this patch, we are looking at the boot_index to make sure the boot device is attached first (and detached the last). The rest of block devices will still be sorted by mount_device. Change-Id: I17af17955159fe4a6cd18028e6efe1feb47e4ff9 --- nova/tests/unit/virt/vmwareapi/test_vmops.py | 36 +++++++++++++++++--- nova/virt/vmwareapi/vmops.py | 25 +++++++++++--- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/nova/tests/unit/virt/vmwareapi/test_vmops.py b/nova/tests/unit/virt/vmwareapi/test_vmops.py index 393c95f9834..bc3d2b60cdb 100644 --- a/nova/tests/unit/virt/vmwareapi/test_vmops.py +++ b/nova/tests/unit/virt/vmwareapi/test_vmops.py @@ -1269,17 +1269,45 @@ def test_finish_revert_in_place_migration_relocate_fails(self): def test_attach_volumes(self, fake_attach_volume): block_device_info = { 'block_device_mapping': [ - {'mount_device': '/dev/sda', 'connection_info': {'id': 'c'}}, + {'mount_device': '/dev/sda', 'connection_info': {'id': 'a'}}, {'mount_device': '/dev/sdb', 'connection_info': {'id': 'b'}}, - {'mount_device': '/dev/sdc', 'connection_info': {'id': 'a'}}, + {'mount_device': '/dev/sdc', 'connection_info': {'id': 'c'}}, + {'mount_device': '/dev/sdd', 'boot_index': 1, + 'connection_info': {'id': 'd'}}, + {'mount_device': '/dev/sde', 'boot_index': 0, + 'connection_info': {'id': 'e'}}, ] } self._vmops._attach_volumes(self._instance, block_device_info, mock.sentinel.adapter_type) fake_attach_volume.assert_has_calls([ - mock.call({'id': 'c'}, self._instance, mock.sentinel.adapter_type), - mock.call({'id': 'b'}, self._instance, mock.sentinel.adapter_type), + mock.call({'id': 'e'}, self._instance, mock.sentinel.adapter_type), + mock.call({'id': 'd'}, self._instance, mock.sentinel.adapter_type), mock.call({'id': 'a'}, self._instance, mock.sentinel.adapter_type), + mock.call({'id': 'b'}, self._instance, mock.sentinel.adapter_type), + mock.call({'id': 'c'}, self._instance, mock.sentinel.adapter_type), + ]) + + @mock.patch.object(volumeops.VMwareVolumeOps, 'detach_volume') + def test_detach_volumes(self, fake_attach_volume): + block_device_info = { + 'block_device_mapping': [ + {'mount_device': '/dev/sda', 'connection_info': {'id': 'a'}}, + {'mount_device': '/dev/sdb', 'connection_info': {'id': 'b'}}, + {'mount_device': '/dev/sdc', 'connection_info': {'id': 'c'}}, + {'mount_device': '/dev/sdd', 'boot_index': 1, + 'connection_info': {'id': 'd'}}, + {'mount_device': '/dev/sde', 'boot_index': 0, + 'connection_info': {'id': 'e'}}, + ] + } + self._vmops._detach_volumes(self._instance, block_device_info) + fake_attach_volume.assert_has_calls([ + mock.call({'id': 'c'}, self._instance), + mock.call({'id': 'b'}, self._instance), + mock.call({'id': 'a'}, self._instance), + mock.call({'id': 'd'}, self._instance), + mock.call({'id': 'e'}, self._instance), ]) @mock.patch.object(vmops.VMwareVMOps, '_get_instance_metadata') diff --git a/nova/virt/vmwareapi/vmops.py b/nova/virt/vmwareapi/vmops.py index 6978fa3244a..31d7e440c20 100644 --- a/nova/virt/vmwareapi/vmops.py +++ b/nova/virt/vmwareapi/vmops.py @@ -3285,9 +3285,7 @@ def _detach_volumes(self, instance, block_device_info): disks = driver.block_device_info_get_mapping(block_device_info) # Detach the volumes in reverse order, so if we roll it back # that the device order will still be preserved - for disk in sorted(disks, - reverse=True, - key=itemgetter('mount_device')): + for disk in self._sort_disks(disks, reverse=True): try: self._volumeops.detach_volume(disk['connection_info'], instance) @@ -3299,8 +3297,7 @@ def _attach_volumes(self, instance, block_device_info, adapter_type, existing_disks=None): disks = driver.block_device_info_get_mapping(block_device_info) # make sure the disks are attached by the device_name order - for disk in sorted(disks, - key=itemgetter('mount_device')): + for disk in self._sort_disks(disks): if existing_disks and disk['volume_id'] in existing_disks: continue @@ -3308,6 +3305,24 @@ def _attach_volumes(self, instance, block_device_info, adapter_type, self._volumeops.attach_volume(disk['connection_info'], instance, adapter_type) + def _sort_disks(self, disks, reverse=False): + """Since mount_device can be controlled from outside, we want to + make sure the boot_index >= 0 devices are always at the top. + """ + boot_disks = sorted( + (d for d in disks if d.get('boot_index') is not None), + key=itemgetter('boot_index'), + reverse=reverse) + other_disks = sorted( + (d for d in disks if d.get('boot_index') is None), + key=itemgetter('mount_device'), + reverse=reverse) + + if reverse: + return other_disks + boot_disks + else: + return boot_disks + other_disks + def _find_esx_host(self, cluster_ref, ds_ref): """Find ESX host in the specified cluster which is also connected to the specified datastore.