XenAPI/Stops the migration of volume backed VHDS

This commit aims to correct problems with the resize_up codebase that allows
the snapshot and migration of volume backed VDI/VHDs.  Since these are empty
stub disks, and the XenAPI does not allow these VDIs to be snapped, this results
in an SR_OPERATION_NOT_ALLOWED or similar error on attempt.

This change adds a check into the _process_ephemeral_chain_recursive method to
run the current userdevice through volume_utils.is_booted_from_volume.  To
achieve this, the method has been opened in scope to accept custom user_device
objects.  In a future commit we will need to rename this method for clarity
and correct its dependancies that call it.  I have added a TODO for this to be
done by myself. The check will ensure that the userdevice is not volume backed
and then continue to snapshot and migrate the disk as needed, else increment
and move on.

Closes-Bug: #1745072
Change-Id: I7cd2977c8268c1f73062b5d0b2b68ea686db99fe
(cherry picked from commit eefb20e465)

nova/tests/unit/virt/xenapi/test_vmops.py

@@ -1323,11 +1323,11 @@ class MigrateDiskResizingUpTestCase(VMOpsTestBase):
             yield [leaf, parent]
 
     @mock.patch.object(volume_utils, 'is_booted_from_volume',
-            return_value=False)
+                       return_value=False)
     def test_migrate_disk_resizing_up_works_no_ephemeral(self,
-            mock_is_booted_from_volume,
-            mock_apply_orig, mock_update_progress, mock_get_all_vdi_uuids,
-            mock_shutdown, mock_migrate_vhd, mock_get_vdi_for_vm):
+            mock_is_booted_from_volume, mock_apply_orig, mock_update_progress,
+            mock_get_all_vdi_uuids, mock_shutdown, mock_migrate_vhd,
+            mock_get_vdi_for_vm):
         context = "ctxt"
         instance = {"name": "fake", "uuid": "uuid"}
         dest = "dest"
@@ -1365,11 +1365,11 @@ class MigrateDiskResizingUpTestCase(VMOpsTestBase):
         self.assertEqual(prog_expected, mock_update_progress.call_args_list)
 
     @mock.patch.object(volume_utils, 'is_booted_from_volume',
-            return_value=False)
-    def test_migrate_disk_resizing_up_works_with_two_ephemeral(self,
-            mock_is_booted_from_volume,
-            mock_apply_orig, mock_update_progress, mock_get_all_vdi_uuids,
-            mock_shutdown, mock_migrate_vhd, mock_get_vdi_for_vm):
+                       return_value=False)
+    def test_migrate_disk_resizing_up_ephemerals_no_volume(self,
+            mock_is_booted_from_volume, mock_apply_orig, mock_update_progress,
+            mock_get_all_vdi_uuids, mock_shutdown, mock_migrate_vhd,
+            mock_get_vdi_for_vm):
         context = "ctxt"
         instance = {"name": "fake", "uuid": "uuid"}
         dest = "dest"
@@ -1416,12 +1416,11 @@ class MigrateDiskResizingUpTestCase(VMOpsTestBase):
             ]
         self.assertEqual(prog_expected, mock_update_progress.call_args_list)
 
-    @mock.patch.object(volume_utils, 'is_booted_from_volume',
-            return_value=True)
-    def test_migrate_disk_resizing_up_booted_from_volume(self,
-            mock_is_booted_from_volume,
-            mock_apply_orig, mock_update_progress, mock_get_all_vdi_uuids,
-            mock_shutdown, mock_migrate_vhd, mock_get_vdi_for_vm):
+    @mock.patch.object(volume_utils, 'is_booted_from_volume')
+    def test_migrate_disk_resizing_up_ephemerals_mixed_volumes(self,
+            mock_is_booted_from_volume, mock_apply_orig, mock_update_progress,
+            mock_get_all_vdi_uuids, mock_shutdown, mock_migrate_vhd,
+            mock_get_vdi_for_vm):
         context = "ctxt"
         instance = {"name": "fake", "uuid": "uuid"}
         dest = "dest"
@@ -1431,6 +1430,10 @@ class MigrateDiskResizingUpTestCase(VMOpsTestBase):
         mock_get_all_vdi_uuids.return_value = ["vdi-eph1", "vdi-eph2"]
         mock_get_vdi_for_vm.side_effect = [({}, {"uuid": "4-root"}),
                                            ({}, {"uuid": "5-root"})]
+        # Here we mock the is_booted_from_volume call to emulate the
+        # 4-root and 4-parent VDI's being volume based, while 5-root
+        # and 5-Parent are local ephemeral drives that should be migrated.
+        mock_is_booted_from_volume.side_effect = [True, False, True, False]
 
         with mock.patch.object(vm_utils, '_snapshot_attached_here_impl',
                 self._fake_snapshot_attached_here):
@@ -1441,15 +1444,11 @@ class MigrateDiskResizingUpTestCase(VMOpsTestBase):
                 vm_ref, min_userdevice=4)
         mock_apply_orig.assert_called_once_with(instance, vm_ref)
         mock_shutdown.assert_called_once_with(instance, vm_ref)
-
         m_vhd_expected = [mock.call(self.vmops._session, instance,
                                     "4-parent", dest, sr_path, 1, 1),
                           mock.call(self.vmops._session, instance,
-                                    "5-parent", dest, sr_path, 1, 2),
-                          mock.call(self.vmops._session, instance,
-                                    "4-root", dest, sr_path, 0, 1),
-                          mock.call(self.vmops._session, instance,
-                                    "5-root", dest, sr_path, 0, 2)]
+                                    "4-root", dest, sr_path, 0, 1)]
+
         self.assertEqual(m_vhd_expected, mock_migrate_vhd.call_args_list)
 
         prog_expected = [

nova/virt/xenapi/vmops.py

@@ -1210,7 +1210,8 @@ class VMOps(object):
             LOG.debug("Migrated root base vhds", instance=instance)
 
         def _process_ephemeral_chain_recursive(ephemeral_chains,
-                                               active_vdi_uuids):
+                                               active_vdi_uuids,
+                                               ephemeral_disk_index=0):
             # This method is called several times, recursively.
             # The first phase snapshots the ephemeral disks, and
             # migrates the read only VHD files.
@@ -1223,48 +1224,55 @@ class VMOps(object):
                 # all the ephemeral disks, so its time to power down
                 # and complete the migration of the diffs since the snapshot
                 LOG.debug("Migrated all base vhds.", instance=instance)
-                return power_down_and_transfer_leaf_vhds(
-                            active_root_vdi_uuid,
-                            active_vdi_uuids)
+                return power_down_and_transfer_leaf_vhds(active_root_vdi_uuid,
+                                                         active_vdi_uuids)
 
             remaining_chains = []
             if number_of_chains > 1:
                 remaining_chains = ephemeral_chains[1:]
 
-            ephemeral_disk_index = len(active_vdi_uuids)
             userdevice = int(DEVICE_EPHEMERAL) + ephemeral_disk_index
 
-            # Here we take a snapshot of the ephemeral disk,
-            # and migrate all VHDs in the chain that are not being written to
-            # Once that is completed, we call back into this method to either:
-            # - migrate any remaining ephemeral disks
-            # - or, if all disks are migrated, we power down and complete
-            #   the migration but copying the diffs since all the snapshots
-            #   were taken
-            with vm_utils.snapshot_attached_here(self._session, instance,
-                    vm_ref, label, str(userdevice)) as chain_vdi_uuids:
+            # Ensure we are not snapshotting a volume
+            if not volume_utils.is_booted_from_volume(self._session, vm_ref,
+                                                      userdevice):
 
-                # remember active vdi, we will migrate these later
-                vdi_ref, vm_vdi_rec = vm_utils.get_vdi_for_vm_safely(
+                # Here we take a snapshot of the ephemeral disk,
+                # and migrate all VHDs in the chain that are not being written
+                # to. Once that is completed, we call back into this method to
+                # either:
+                # - migrate any remaining ephemeral disks
+                # - or, if all disks are migrated, we power down and complete
+                #   the migration but copying the diffs since all the snapshots
+                #   were taken
+
+                with vm_utils.snapshot_attached_here(self._session, instance,
+                            vm_ref, label, str(userdevice)) as chain_vdi_uuids:
+
+                    # remember active vdi, we will migrate these later
+                    vdi_ref, vm_vdi_rec = vm_utils.get_vdi_for_vm_safely(
                         self._session, vm_ref, str(userdevice))
-                active_uuid = vm_vdi_rec['uuid']
-                active_vdi_uuids.append(active_uuid)
+                    active_uuid = vm_vdi_rec['uuid']
+                    active_vdi_uuids.append(active_uuid)
 
-                # migrate inactive vhds
-                inactive_vdi_uuids = chain_vdi_uuids[1:]
-                ephemeral_disk_number = ephemeral_disk_index + 1
-                for seq_num, vdi_uuid in enumerate(inactive_vdi_uuids,
-                                                   start=1):
-                    vm_utils.migrate_vhd(self._session, instance, vdi_uuid,
-                                         dest, sr_path, seq_num,
-                                         ephemeral_disk_number)
+                    # migrate inactive vhds
+                    inactive_vdi_uuids = chain_vdi_uuids[1:]
+                    ephemeral_disk_number = ephemeral_disk_index + 1
+                    for seq_num, vdi_uuid in enumerate(inactive_vdi_uuids,
+                                                       start=1):
+                        vm_utils.migrate_vhd(self._session, instance, vdi_uuid,
+                                             dest, sr_path, seq_num,
+                                             ephemeral_disk_number)
 
-                LOG.debug("Read-only migrated for disk: %s", userdevice,
-                          instance=instance)
-                # This is recursive to simplify the taking and cleaning up
-                # of all the ephemeral disk snapshots
-                return _process_ephemeral_chain_recursive(remaining_chains,
-                                                          active_vdi_uuids)
+                    LOG.debug("Read-only migrated for disk: %s", userdevice,
+                              instance=instance)
+
+            # This method is recursive, so we will increment our index
+            # and process again until the chains are empty.
+            ephemeral_disk_index = ephemeral_disk_index + 1
+            return _process_ephemeral_chain_recursive(remaining_chains,
+                                                      active_vdi_uuids,
+                                                      ephemeral_disk_index)
 
         @step
         def transfer_ephemeral_disks_then_all_leaf_vdis():

nova/virt/xenapi/volume_utils.py

@@ -342,9 +342,11 @@ def find_vbd_by_number(session, vm_ref, dev_number):
                 LOG.debug(msg, exc_info=True)
 
 
-def is_booted_from_volume(session, vm_ref):
+def is_booted_from_volume(session, vm_ref, user_device=0):
     """Determine if the root device is a volume."""
-    vbd_ref = find_vbd_by_number(session, vm_ref, 0)
+    # TODO(bkaminski):  We have opened the scope of this method to accept
+    # userdevice.  We should rename this method and its references for clarity.
+    vbd_ref = find_vbd_by_number(session, vm_ref, user_device)
     vbd_other_config = session.VBD.get_other_config(vbd_ref)
     if vbd_other_config.get('osvol', False):
         return True