Updates in-use volume removal error message #1273

venilnoronha · 2017-05-17T17:36:37Z

Description

This PR updates the error message that's printed on removal of in-use volumes. Changes to the error message are as follows. Fixes #1208.

Replaced VMDK path with Volume name.
Added VM name.
Removed VM uuid.

Signature of the getStatusAttached(..) function was modified to return a 4th value i.e. vm_name, which is needed for building the error message in removeVMDK(..).

Test

docker volume rm v2 now displays:

Error response from daemon: unable to remove volume: remove v2: VolumeDriver.Remove: Failed to remove volume v2, in use by VM = ubuntu-VM1.2.

ESX Plugin log at /var/log/vmware/vmdk_ops.log displays:

05/17/17 21:39:41 1835942 [ubuntu-VM0.2-sharedVmfs-0._DEFAULT.v2] [INFO   ] *** removeVMDK: /vmfs/volumes/sharedVmfs-0/dockvols/11111111-1111-1111-1111-111111111111/v2.vmdk
05/17/17 21:39:41 1835942 [ubuntu-VM0.2-sharedVmfs-0._DEFAULT.v2] [WARNING] Disk /vmfs/volumes/sharedVmfs-0/dockvols/11111111-1111-1111-1111-111111111111/v2.vmdk already attached to VM=ubuntu-VM1.2
05/17/17 21:39:41 1835942 [ubuntu-VM0.2-sharedVmfs-0._DEFAULT.v2] [INFO   ] *** removeVMDK: /vmfs/volumes/sharedVmfs-0/dockvols/11111111-1111-1111-1111-111111111111/v2.vmdk is in use, volume = v2 VM = ubuntu-VM1.2 VM-uuid = 564ddf99-d58f-db6e-bf3e-d4afd3377873 ({'Error': 'Disk /vmfs/volumes/sharedVmfs-0/dockvols/11111111-1111-1111-1111-111111111111/v2.vmdk already attached to VM=ubuntu-VM1.2'})
05/17/17 21:39:41 1835942 [ubuntu-VM0.2-sharedVmfs-0._DEFAULT.v2] [INFO   ] executeRequest 'remove' completed with ret={'Error': 'Failed to remove volume v2, in use by VM = ubuntu-VM1.2.'}

Please review and merge.

Thanks,
Venil

…ing in-use volumes

shaominchen · 2017-05-17T21:26:37Z

esx_service/vmdk_ops.py

+                vol_name = vmdk_utils.get_volname_from_vmdk_path(vmdk_path)
+            logging.info("*** removeVMDK: %s is in use, volume = %s VM = %s VM-uuid = %s (%s)",
+                vmdk_path, vol_name, attached_vm_name, kv_uuid, ret)
+            return err("Failed to remove volume {0}, in use by VM = {1} VM-uuid = {2}.".format(


I'd suggest to remove VM-uuid from the error message.

Okay, I'll fix that.

shaominchen

LGTM. Please update the test result in the PR description.

venilnoronha · 2017-05-17T21:52:14Z

Got that done too.

pshahzeb · 2017-05-17T22:31:55Z

esx_service/vmdk_ops.py

@@ -1117,7 +1119,7 @@ def setStatusDetached(vmdk_path):


 def getStatusAttached(vmdk_path):
-    '''Returns (attached, uuid, attach_as) tuple. For 'detached' status uuid is None'''
+    '''Returns (attached, uuid, attach_as, vm_name) tuple. For 'detached' status uuid is None'''


name is also None in this case

pdhamdhere · 2017-05-18T04:03:19Z

esx_service/vmdk_ops.py

@@ -288,7 +288,7 @@ def cloneVMDK(vm_name, vmdk_path, opts={}, vm_uuid=None, datastore_url=None):
    lockname = "{}.{}.{}".format(src_datastore, tenant_name, src_volume)
    with lockManager.get_lock(lockname):
        # Verify if the source volume is in use.
-        attached, uuid, attach_as = getStatusAttached(src_vmdk_path)
+        attached, uuid, attach_as, _ = getStatusAttached(src_vmdk_path)
        if attached:
            if handle_stale_attach(vmdk_path, uuid):
                return err("Source volume cannot be in use when cloning")


How about changing this error message to:
"Source volume {0} in use by VM {1} and can't be cloned.".format(vol_name, attached_vm_name)

Addressed in #1275.

pdhamdhere · 2017-05-18T04:05:05Z

esx_service/vmdk_ops.py

-            logging.info("*** removeVMDK: %s is in use, VM uuid = %s (%s)", vmdk_path, kv_uuid, ret)
-            return err("Failed to remove volume {0}, in use by VM uuid = {1}.".format(
-                vmdk_path, kv_uuid))
+            if vol_name is None:


vol_name can't be NULL. This if block can be removed.

I had to add this because the function signature is removeVMDK(.., vol_name=None, ..) and there are a few places from where it's called without the vol_name parameter. Making the suggested change will need a modification at vmdk_ops.py#L355 and at multiple locations in vmdk_ops_test.py. I can address that.

Aha. I see. You are right.

pdhamdhere · 2017-05-18T04:06:57Z

esx_service/vmdk_ops.py

+                vol_name = vmdk_utils.get_volname_from_vmdk_path(vmdk_path)
+            logging.info("*** removeVMDK: %s is in use, volume = %s VM = %s VM-uuid = %s (%s)",
+                vmdk_path, vol_name, attached_vm_name, kv_uuid, ret)
+            return err("Failed to remove volume {0}, in use by VM = {1}.".format(vol_name, attached_vm_name))


I assume .format can handle NULL attached_vm_name

It would print .. in use by VM = None. Is that alright?

Error looks weird but that would be stale KV. Fine with me.

pdhamdhere · 2017-05-18T18:28:31Z

esx_service/vmdk_ops.py

-            logging.info("*** removeVMDK: %s is in use, VM uuid = %s (%s)", vmdk_path, kv_uuid, ret)
-            return err("Failed to remove volume {0}, in use by VM uuid = {1}.".format(
-                vmdk_path, kv_uuid))
+            if vol_name is None:


Aha. I see. You are right.

pdhamdhere · 2017-05-18T18:29:07Z

esx_service/vmdk_ops.py

+                vol_name = vmdk_utils.get_volname_from_vmdk_path(vmdk_path)
+            logging.info("*** removeVMDK: %s is in use, volume = %s VM = %s VM-uuid = %s (%s)",
+                vmdk_path, vol_name, attached_vm_name, kv_uuid, ret)
+            return err("Failed to remove volume {0}, in use by VM = {1}.".format(vol_name, attached_vm_name))


Error looks weird but that would be stale KV. Fine with me.

venilnoronha · 2017-05-18T18:48:29Z

@pdhamdhere thanks for the merge!

Resolves vmware-archive#1208 - Updates error message printed on remov…

16343fc

…ing in-use volumes

vmwclabot added the cla-not-required label May 17, 2017

tusharnt requested review from pshahzeb and shaominchen May 17, 2017 21:12

shaominchen reviewed May 17, 2017

View reviewed changes

Removes VM-uuid from removeVMDK error

45f490f

shaominchen approved these changes May 17, 2017

View reviewed changes

pshahzeb reviewed May 17, 2017

View reviewed changes

Updates getStatusAttached method description

1a8bbb7

pshahzeb approved these changes May 17, 2017

View reviewed changes

pdhamdhere suggested changes May 18, 2017

View reviewed changes

pdhamdhere approved these changes May 18, 2017

View reviewed changes

pdhamdhere merged commit db554e5 into vmware-archive:master May 18, 2017

venilnoronha mentioned this pull request May 18, 2017

Updates in-use volume clone error message #1275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates in-use volume removal error message #1273

Updates in-use volume removal error message #1273

venilnoronha commented May 17, 2017 •

edited

Loading

shaominchen May 17, 2017

venilnoronha May 17, 2017

shaominchen left a comment

venilnoronha commented May 17, 2017

pshahzeb May 17, 2017

venilnoronha May 17, 2017

pdhamdhere May 18, 2017

venilnoronha May 18, 2017

venilnoronha May 18, 2017

pdhamdhere May 18, 2017

venilnoronha May 18, 2017

pdhamdhere May 18, 2017

pdhamdhere May 18, 2017

venilnoronha May 18, 2017

pdhamdhere May 18, 2017

pdhamdhere May 18, 2017

pdhamdhere May 18, 2017

venilnoronha commented May 18, 2017

Updates in-use volume removal error message #1273

Updates in-use volume removal error message #1273

Conversation

venilnoronha commented May 17, 2017 • edited Loading

Description

Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen left a comment

Choose a reason for hiding this comment

venilnoronha commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha commented May 18, 2017

venilnoronha commented May 17, 2017 •

edited

Loading