VMware: Detach volume should not delete vmdk

During volume detach a delete_virtual_disk_spec is used to remove
the device from the running instance. This spec also "destroy" the
underlying vmdk file, this is not right, we should not delete vmdk
file when detach it from a VM instance.

The fix was remove the fileOperation field when detach vmdk volume
so as to make sure the volume will not be destroyed; but for iscsi
we still need to destroy the volume.

Also I changed the function name from delete_virtual_disk_spec to
detach_virtual_disk_spec which is more accurate.

Change-Id: Ibd218d6a8cfeede4f5ca74f28bc0c3d0c185bb14
Closes-Bug: #1241350
(cherry picked from commit 0405d7b937)

nova/tests/virt/vmwareapi/test_vmwareapi.py

@@ -957,7 +957,7 @@ class VMwareAPIVMTestCase(test.NoDBTestCase):
         self.mox.StubOutWithMock(volumeops.VMwareVolumeOps,
                                  'detach_disk_from_vm')
         volumeops.VMwareVolumeOps.detach_disk_from_vm(mox.IgnoreArg(),
-                self.instance, device)
+                self.instance, device, destroy_disk=True)
         self.mox.ReplayAll()
         self.conn.detach_volume(connection_info, self.instance, mount_point,
                                 encryption=None)

nova/tests/virt/vmwareapi/test_vmwareapi_vm_util.py

@@ -370,3 +370,23 @@ class VMwareVMUtilTestCase(test.NoDBTestCase):
         self.assertTrue(hasattr(propdict['some.thing'], 'value'))
         self.assertEqual("else", propdict['some.thing'].value)
         self.assertEqual("value", propdict['another.thing'])
+
+    def _test_detach_virtual_disk_spec(self, destroy_disk=False):
+        virtual_device_config = vm_util.detach_virtual_disk_spec(
+                                                     fake.FakeFactory(),
+                                                     'fake_device',
+                                                     destroy_disk)
+        self.assertEqual('remove', virtual_device_config.operation)
+        self.assertEqual('fake_device', virtual_device_config.device)
+        self.assertEqual('ns0:VirtualDeviceConfigSpec',
+                         virtual_device_config.obj_name)
+        if destroy_disk:
+            self.assertEqual('destroy', virtual_device_config.fileOperation)
+        else:
+            self.assertFalse(hasattr(virtual_device_config, 'fileOperation'))
+
+    def test_detach_virtual_disk_spec(self):
+        self._test_detach_virtual_disk_spec(destroy_disk=False)
+
+    def test_detach_virtual_disk_destroy_spec(self):
+        self._test_detach_virtual_disk_spec(destroy_disk=True)

nova/tests/virt/vmwareapi/test_vmwareapi_volumeops.py

@@ -0,0 +1,70 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=43
+
+#    Copyright 2013 IBM Corp.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import contextlib
+import mock
+
+from nova import test
+from nova.tests.virt.vmwareapi import stubs
+from nova.virt.vmwareapi import driver
+from nova.virt.vmwareapi import fake as vmwareapi_fake
+from nova.virt.vmwareapi import volumeops
+
+
+class VMwareVolumeOpsTestCase(test.NoDBTestCase):
+
+    def setUp(self):
+        super(VMwareVolumeOpsTestCase, self).setUp()
+        vmwareapi_fake.reset()
+        stubs.set_stubs(self.stubs)
+
+        self._session = driver.VMwareAPISession()
+        self._volumeops = volumeops.VMwareVolumeOps(self._session)
+        self.instance = {'name': 'fake_name', 'uuid': 'fake_uuid'}
+
+    def _test_detach_disk_from_vm(self, destroy_disk=False):
+        def fake_call_method(module, method, *args, **kwargs):
+            vmdk_detach_config_spec = kwargs.get('spec')
+            virtual_device_config = vmdk_detach_config_spec.deviceChange[0]
+            self.assertEqual('remove', virtual_device_config.operation)
+            self.assertEqual('ns0:VirtualDeviceConfigSpec',
+                              virtual_device_config.obj_name)
+            if destroy_disk:
+                self.assertEqual('destroy',
+                                 virtual_device_config.fileOperation)
+            else:
+                self.assertFalse(hasattr(virtual_device_config,
+                                 'fileOperation'))
+            return 'fake_configure_task'
+        with contextlib.nested(
+            mock.patch.object(self._session, '_wait_for_task'),
+            mock.patch.object(self._session, '_call_method',
+                              fake_call_method)
+        ) as (_wait_for_task, _call_method):
+            fake_device = vmwareapi_fake.DataObject()
+            fake_device.backing = vmwareapi_fake.DataObject()
+            fake_device.backing.fileName = 'fake_path'
+            fake_device.key = 'fake_key'
+            self._volumeops.detach_disk_from_vm('fake_vm_ref', self.instance,
+                                                fake_device, destroy_disk)
+            _wait_for_task.assert_has_calls([
+                   mock.call(self.instance['uuid'], 'fake_configure_task')])
+
+    def test_detach_with_destroy_disk_from_vm(self):
+        self._test_detach_disk_from_vm(destroy_disk=True)
+
+    def test_detach_without_destroy_disk_from_vm(self):
+        self._test_detach_disk_from_vm(destroy_disk=False)

nova/virt/vmwareapi/vm_util.py

@@ -255,13 +255,15 @@ def get_cdrom_attach_config_spec(client_factory,
     return config_spec
 
 
-def get_vmdk_detach_config_spec(client_factory, device):
+def get_vmdk_detach_config_spec(client_factory, device,
+                                destroy_disk=False):
     """Builds the vmdk detach config spec."""
     config_spec = client_factory.create('ns0:VirtualMachineConfigSpec')
 
     device_config_spec = []
-    virtual_device_config_spec = delete_virtual_disk_spec(client_factory,
-                                                          device)
+    virtual_device_config_spec = detach_virtual_disk_spec(client_factory,
+                                                          device,
+                                                          destroy_disk)
 
     device_config_spec.append(virtual_device_config_spec)
 
@@ -461,14 +463,15 @@ def create_virtual_disk_spec(client_factory, controller_key,
     return virtual_device_config
 
 
-def delete_virtual_disk_spec(client_factory, device):
+def detach_virtual_disk_spec(client_factory, device, destroy_disk=False):
     """
-    Builds spec for the deletion of an already existing Virtual Disk from VM.
+    Builds spec for the detach of an already existing Virtual Disk from VM.
     """
     virtual_device_config = client_factory.create(
                             'ns0:VirtualDeviceConfigSpec')
     virtual_device_config.operation = "remove"
-    virtual_device_config.fileOperation = "destroy"
+    if destroy_disk:
+        virtual_device_config.fileOperation = "destroy"
     virtual_device_config.device = device
 
     return virtual_device_config

nova/virt/vmwareapi/volumeops.py

@@ -111,7 +111,8 @@ class VMwareVolumeOps(object):
                 if option.key == volume_option:
                     return option.value
 
-    def detach_disk_from_vm(self, vm_ref, instance, device):
+    def detach_disk_from_vm(self, vm_ref, instance, device,
+                            destroy_disk=False):
         """
         Detach disk from VM by reconfiguration.
         """
@@ -119,7 +120,7 @@ class VMwareVolumeOps(object):
         instance_uuid = instance['uuid']
         client_factory = self._session._get_vim().client.factory
         vmdk_detach_config_spec = vm_util.get_vmdk_detach_config_spec(
-                                    client_factory, device)
+                                    client_factory, device, destroy_disk)
         disk_key = device.key
         LOG.debug(_("Reconfiguring VM instance %(instance_name)s to detach "
                     "disk %(disk_key)s"),
@@ -359,7 +360,8 @@ class VMwareVolumeOps(object):
         self._relocate_vmdk_volume(volume_ref, res_pool, datastore)
 
         # Delete the original disk from the volume_ref
-        self.detach_disk_from_vm(volume_ref, instance, original_device)
+        self.detach_disk_from_vm(volume_ref, instance, original_device,
+                                 destroy_disk=True)
         # Attach the current disk to the volume_ref
         # Get details required for adding disk device such as
         # adapter_type, unit_number, controller_key
@@ -433,7 +435,7 @@ class VMwareVolumeOps(object):
         device = vm_util.get_rdm_disk(hardware_devices, uuid)
         if device is None:
             raise volume_util.StorageError(_("Unable to find volume"))
-        self.detach_disk_from_vm(vm_ref, instance, device)
+        self.detach_disk_from_vm(vm_ref, instance, device, destroy_disk=True)
         LOG.info(_("Mountpoint %(mountpoint)s detached from "
                    "instance %(instance_name)s"),
                  {'mountpoint': mountpoint, 'instance_name': instance_name})