Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Updates in-use volume removal error message #1273

Merged
merged 3 commits into from
May 18, 2017
Merged

Updates in-use volume removal error message #1273

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16343fc
Resolves #1208 - Updates error message printed on removing in-use vol…
venilnoronha May 16, 2017
45f490f
Removes VM-uuid from removeVMDK error
venilnoronha May 17, 2017
1a8bbb7
Updates getStatusAttached method description
venilnoronha May 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions esx_service/vmdk_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #1275.

Expand Down Expand Up @@ -598,13 +598,16 @@ def removeVMDK(vmdk_path, vol_name=None, vm_name=None, tenant_uuid=None, datasto
logging.info("*** removeVMDK: %s", vmdk_path)

# Check the current volume status
kv_status_attached, kv_uuid, attach_mode = getStatusAttached(vmdk_path)
kv_status_attached, kv_uuid, attach_mode, attached_vm_name = getStatusAttached(vmdk_path)
if kv_status_attached:
ret = handle_stale_attach(vmdk_path, kv_uuid)
if ret:
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha. I see. You are right.

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll fix that.

vol_name, attached_vm_name, kv_uuid))

# Cleaning .vmdk file
clean_err = cleanVMDK(vmdk_path, vol_name)
Expand Down Expand Up @@ -1117,7 +1120,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'''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is also None in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it.


vol_meta = kv.getAll(vmdk_path)
try:
Expand All @@ -1126,14 +1129,20 @@ def getStatusAttached(vmdk_path):
attach_as = kv.DEFAULT_ATTACH_AS

if not vol_meta or kv.STATUS not in vol_meta:
return False, None, attach_as
return False, None, attach_as, None

attached = (vol_meta[kv.STATUS] == kv.ATTACHED)
try:
uuid = vol_meta[kv.ATTACHED_VM_UUID]
except:
uuid = None
return attached, uuid, attach_as

try:
vm_name = vol_meta[kv.ATTACHED_VM_NAME]
except:
vm_name = None

return attached, uuid, attach_as, vm_name

def handle_stale_attach(vmdk_path, kv_uuid):
'''
Expand Down Expand Up @@ -1250,7 +1259,7 @@ def disk_attach(vmdk_path, vm):
return error or unit:bus numbers of newly attached disk.
'''

kv_status_attached, kv_uuid, attach_mode = getStatusAttached(vmdk_path)
kv_status_attached, kv_uuid, attach_mode, _ = getStatusAttached(vmdk_path)
logging.info("Attaching {0} as {1}".format(vmdk_path, attach_mode))

# If the volume is attached then check if the attach is stale (VM is powered off).
Expand Down