From 976d316eb71cede6294f92b30e3011e4c618975c Mon Sep 17 00:00:00 2001
From: Lucian Petrut <lpetrut@cloudbasesolutions.com>
Date: Thu, 29 Nov 2018 13:51:14 +0200
Subject: [PATCH] Prevent reverting resizes from leaking files

When reverting resizes, the manager will request instance files
cleanup to be skipped if the hosts use shared storage, in which
case we'd leak files if multiple CSVs are used.

It's safe to cleanup the instance files when reverting resizes
as long as we preserve the "*_revert" dir.

Closes-Bug: #1805833

Change-Id: I09428c2e25bed41f99b51c08a677d7152d9680ee
---
 compute_hyperv/nova/vmops.py            | 10 ++++++++
 compute_hyperv/tests/unit/test_vmops.py | 32 ++++++++++++++++++-------
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/compute_hyperv/nova/vmops.py b/compute_hyperv/nova/vmops.py
index 1418a0e4..8b101b3e 100644
--- a/compute_hyperv/nova/vmops.py
+++ b/compute_hyperv/nova/vmops.py
@@ -24,6 +24,7 @@ import time
 
 from eventlet import timeout as etimeout
 from nova.api.metadata import base as instance_metadata
+from nova.compute import task_states
 from nova.compute import vm_states
 from nova import exception
 from nova import objects
@@ -884,6 +885,15 @@ class VMOps(object):
         instance_path = self._pathutils.get_instance_dir(instance.name,
                                                          create_dir=False)
 
+        # When reverting resizes, the manager will request instance files
+        # cleanup to be skipped if the hosts use shared storage, in which
+        # case we'd leak files if multiple CSVs are used. It's safe to
+        # cleanup the instance files when reverting resizes as long as we
+        # preserve the "*_revert" dir.
+        if instance.task_state == task_states.RESIZE_REVERTING:
+            destroy_disks = True
+            cleanup_migration_files = False
+
         try:
             if self._vmutils.vm_exists(instance_name):
 
diff --git a/compute_hyperv/tests/unit/test_vmops.py b/compute_hyperv/tests/unit/test_vmops.py
index 136bd14c..cea300ca 100644
--- a/compute_hyperv/tests/unit/test_vmops.py
+++ b/compute_hyperv/tests/unit/test_vmops.py
@@ -17,6 +17,7 @@ import os
 import ddt
 from eventlet import timeout as etimeout
 import mock
+from nova.compute import task_states
 from nova.compute import vm_states
 from nova import exception
 from nova import objects
@@ -1317,9 +1318,11 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
         self._pathutils.check_remove_dir.assert_has_calls(
             exp_check_remove_dir_calls)
 
-    @ddt.data({"force_destroy": True},
+    @ddt.data({"force_destroy": True, "destroy_disks": False},
               {'vm_exists': False, 'planned_vm_exists': False},
-              {'vm_exists': False, 'planned_vm_exists': True})
+              {'vm_exists': False, 'planned_vm_exists': True},
+              {'task_state': task_states.RESIZE_REVERTING},
+              {'cleanup_migr_files': False})
     @ddt.unpack
     @mock.patch('compute_hyperv.nova.vmops.VMOps._delete_disk_files')
     @mock.patch('compute_hyperv.nova.vmops.VMOps.power_off')
@@ -1327,10 +1330,14 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
     def test_destroy(self, mock_unplug_vifs, mock_power_off,
                      mock_delete_disk_files, vm_exists=True,
                      planned_vm_exists=False,
-                     force_destroy=False):
+                     force_destroy=False,
+                     task_state=task_states.DELETING,
+                     destroy_disks=True,
+                     cleanup_migr_files=True):
         self.flags(force_destroy_instances=force_destroy, group="hyperv")
 
-        mock_instance = fake_instance.fake_instance_obj(self.context)
+        mock_instance = fake_instance.fake_instance_obj(
+            self.context, task_state=task_state)
         self._vmops._vmutils.vm_exists.return_value = vm_exists
         self._vmops._migrutils.planned_vm_exists.return_value = (
             planned_vm_exists)
@@ -1339,7 +1346,8 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
             instance=mock_instance,
             block_device_info=mock.sentinel.FAKE_BD_INFO,
             network_info=mock.sentinel.fake_network_info,
-            cleanup_migration_files=mock.sentinel.cleanup_migr_files)
+            cleanup_migration_files=cleanup_migr_files,
+            destroy_disks=destroy_disks)
 
         self._vmops._vmutils.vm_exists.assert_called_with(
             mock_instance.name)
@@ -1366,10 +1374,16 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
             mock_instance, mock.sentinel.fake_network_info)
         self._vmops._volumeops.disconnect_volumes.assert_called_once_with(
             mock.sentinel.FAKE_BD_INFO)
-        mock_delete_disk_files.assert_called_once_with(
-            mock_instance,
-            self._pathutils.get_instance_dir.return_value,
-            mock.sentinel.cleanup_migr_files)
+
+        reverting_resize = task_state == task_states.RESIZE_REVERTING
+        exp_migr_files_cleanup = cleanup_migr_files and not reverting_resize
+        if destroy_disks or reverting_resize:
+            mock_delete_disk_files.assert_called_once_with(
+                mock_instance,
+                self._pathutils.get_instance_dir.return_value,
+                exp_migr_files_cleanup)
+        else:
+            mock_delete_disk_files.assert_not_called()
 
     @mock.patch('compute_hyperv.nova.vmops.VMOps.power_off')
     def test_destroy_exception(self, mock_power_off):