Merge "Lock snapshot/destroy operations"

compute_hyperv/nova/conf.py

@@ -54,6 +54,13 @@ hyperv_opts = [
                 default=True,
                 help="Allow the VM the failback to its original host once it "
                      "is available."),
+    cfg.BoolOpt('force_destroy_instances',
+                default=False,
+                help="If this option is enabled, instance destroy requests "
+                     "are executed immediately, regardless of instance "
+                     "pending tasks. In some situations, the destroy "
+                     "operation will fail (e.g. due to file locks), "
+                     "requiring subsequent retries."),
 ]
 
 CONF = nova.conf.CONF

compute_hyperv/nova/constants.py

@@ -110,3 +110,7 @@ OPTIONAL = "optional"
 
 IMAGE_PROP_VTPM = "os_vtpm"
 IMAGE_PROP_VTPM_SHIELDED = "os_shielded_vm"
+
+# We have to make sure that such locks are not used outside the driver in
+# order to avoid deadlocks. For this reason, we'll use the 'hv-' scope.
+SNAPSHOT_LOCK_TEMPLATE = "%(instance_uuid)s-hv-snapshot"

compute_hyperv/nova/snapshotops.py

@@ -19,10 +19,14 @@ Management class for VM snapshot operations.
 import os
 
 from nova.compute import task_states
+from nova import exception
 from nova.image import glance
+from nova import utils
+from os_win import exceptions as os_win_exc
 from os_win import utilsfactory
 from oslo_log import log as logging
 
+from compute_hyperv.nova import constants
 from compute_hyperv.nova import pathutils
 
 LOG = logging.getLogger(__name__)
@@ -45,6 +49,20 @@ class SnapshotOps(object):
                                         purge_props=False)
 
     def snapshot(self, context, instance, image_id, update_task_state):
+        # This operation is not fully preemptive at the moment. We're locking
+        # it as well as the destroy operation (if configured to do so).
+        @utils.synchronized(constants.SNAPSHOT_LOCK_TEMPLATE %
+                            dict(instance_uuid=instance.uuid))
+        def instance_synchronized_snapshot():
+            self._snapshot(context, instance, image_id, update_task_state)
+
+        try:
+            instance_synchronized_snapshot()
+        except os_win_exc.HyperVVMNotFoundException:
+            # the instance might disappear before starting the operation.
+            raise exception.InstanceNotFound(instance_id=instance.uuid)
+
+    def _snapshot(self, context, instance, image_id, update_task_state):
         """Create snapshot from a running VM instance."""
         instance_name = instance.name
 

compute_hyperv/nova/vmops.py

@@ -856,8 +856,26 @@ class VMOps(object):
             if backup_location:
                 self._pathutils.check_remove_dir(backup_location)
 
-    def destroy(self, instance, network_info, block_device_info,
-                destroy_disks=True, cleanup_migration_files=True):
+    def destroy(self, instance, *args, **kwargs):
+        # Nova allows destroying instances regardless of pending tasks.
+        # In some cases, we may not be able to properly delete instances
+        # while having a pending task (e.g. when snapshotting, due to file
+        # locks).
+        #
+        # We may append other locks for operations that are non preemptive.
+        # We should not rely on instance task states, which may be hanging.
+        @utils.synchronized(constants.SNAPSHOT_LOCK_TEMPLATE %
+                            dict(instance_uuid=instance.uuid))
+        def synchronized_destroy():
+            self._destroy(instance, *args, **kwargs)
+
+        if CONF.hyperv.force_destroy_instances:
+            self._destroy(instance, *args, **kwargs)
+        else:
+            synchronized_destroy()
+
+    def _destroy(self, instance, network_info, block_device_info,
+                 destroy_disks=True, cleanup_migration_files=True):
         instance_name = instance.name
         LOG.info("Got request to destroy instance", instance=instance)
 

compute_hyperv/tests/unit/test_snapshotops.py

@@ -17,6 +17,8 @@ import os
 
 import mock
 from nova.compute import task_states
+from nova import exception
+from os_win import exceptions as os_win_exc
 
 from compute_hyperv.nova import snapshotops
 from compute_hyperv.tests import fake_instance
@@ -123,3 +125,18 @@ class SnapshotOpsTestCase(test_base.HyperVBaseTestCase):
 
     def test_snapshot_no_base_disk(self):
         self._test_snapshot(base_disk_path=None)
+
+    @mock.patch.object(snapshotops.SnapshotOps, '_snapshot')
+    def test_snapshot_instance_not_found(self, mock_snapshot):
+        mock_instance = fake_instance.fake_instance_obj(self.context)
+        mock_snapshot.side_effect = os_win_exc.HyperVVMNotFoundException(
+            vm_name=mock_instance.name)
+
+        self.assertRaises(exception.InstanceNotFound,
+                          self._snapshotops.snapshot,
+                          self.context, mock_instance, mock.sentinel.image_id,
+                          mock.sentinel.update_task_state)
+
+        mock_snapshot.assert_called_once_with(self.context, mock_instance,
+                                              mock.sentinel.image_id,
+                                              mock.sentinel.update_task_state)

compute_hyperv/tests/unit/test_vmops.py

@@ -1319,7 +1319,7 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
         self._pathutils.check_remove_dir.assert_has_calls(
             exp_check_remove_dir_calls)
 
-    @ddt.data({},
+    @ddt.data({"force_destroy": True},
               {'vm_exists': False, 'planned_vm_exists': False},
               {'vm_exists': False, 'planned_vm_exists': True})
     @ddt.unpack
@@ -1328,7 +1328,10 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
     @mock.patch('compute_hyperv.nova.vmops.VMOps.unplug_vifs')
     def test_destroy(self, mock_unplug_vifs, mock_power_off,
                      mock_delete_disk_files, vm_exists=True,
-                     planned_vm_exists=False):
+                     planned_vm_exists=False,
+                     force_destroy=False):
+        self.flags(force_destroy_instances=force_destroy, group="hyperv")
+
         mock_instance = fake_instance.fake_instance_obj(self.context)
         self._vmops._vmutils.vm_exists.return_value = vm_exists
         self._vmops._migrutils.planned_vm_exists.return_value = (