-
Notifications
You must be signed in to change notification settings - Fork 95
Check attached status in removeVMDK before trying to remove volume #720
Conversation
attached, uuid, attach_as = getStatusAttached(vmdk_path) | ||
if attached: | ||
vm = findVmByUuid(uuid) | ||
logging.info("*** removeVMDK: %s is in use by %s VM uuid = %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On another host, the uuid will not translate to a VM object and can't be used. I was thinking more like the changes below,
kv_status_attached, kv_uuid, attach_mode = getStatusAttached(vmdk_path)
if kv_status_attached and kv_uuid != None:
return err("Failed to remove %s - disk is in use by VM %s." % (vmdk_path, kv_uuid))
Its possible that KV is marked as attached but the VM UUID itself is not set. Which should not happen but just in case that scenario does occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@govint
you're right :) i can close this PR if you already have it done already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, please go ahead and lets put this thru @kerneltime's test and close the issue.
f29b876
to
a8bbccc
Compare
In the absence of our own locking framework around vmdk + key/val and the existing bug, this seems like a reasonable fix that will reduce the probability of hitting the bug. |
I ran a quick test for this code change.
The volume continues to be in good state on reattach and remove fails if the volume is attached. The log message on the VM trying to remove
|
@kerneltime, sorry for not posting the tests earlier. from host1:
from host2:
from esx:
from host1:
from esx:
|
# Check the current volume status | ||
attached, uuid, attach_as = getStatusAttached(vmdk_path) | ||
if attached: | ||
logging.info("*** removeVMDK: %s is in use, VM uuid = %s", vmdk_path, uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would be nice to include the vm_name as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kerneltime
It was there in the first incarnation, but @govint alerted me:
On another host, the uuid will not translate to a VM object and can't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Makes sense
|
||
# Check the current volume status | ||
attached, uuid, attach_as = getStatusAttached(vmdk_path) | ||
if attached: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Information in KV file could be stale thus preventing user from deleting volume ever. I believe this is a workaround to real bug where Unlink/Delete deletes sidecar (VMFD) but leaves VMDK behind if VMDK is already in use. Does this bug exist with API too?
Also would it be better to query underlying system rather than relying on KV? For e.g. "vmkfstools -D" tells you whether file is locked and by whom. If it is locked, fail delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdhamdhere
Yes, the real problem is vmkfstools unlinking associated vmdk files without any verification. I still didn't had time to test with the vim.virtualdiskmanager and wasn't aware of the -D flag. Will try to do this later today.
Maybe we also need an background async mechanism to detect and update the cases of stale metadata and avoid being compelled to no trust it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdhamdhere
Using the api has the same destructive effect as vmkfstools -U.
Using vmkfstools -D to find out if its in use works, but not for the volume.vmdk, In my case was the volume-flat.vmdk.
According with this document we should search for the owner being set.
Do we have identified the conditions or code paths for the stale metadata cases? If not a async mechanism (in relation to the ops), we could have consistency/integrity checks at the end of each op that change metadata (while we still hold the data for saving or update).
vmkfstools -D /vmfs/volumes/datastore2/dockvols/vol1174812.vmdk
Lock [type 10c00001 offset 99715072 v 3122, hb offset 3170304
gen 169, mode 0, owner 00000000-00000000-0000-000000000000 mtime 139245
num 0 gblnum 0 gblgen 0 gblbrk 0]
Addr <4, 222, 33>, gen 3104, links 1, type reg, flags 0, uid 0, gid 0, mode 600
len 566, nb 0 tbz 0, cow 0, newSinceEpoch 0, zla 4305, bs 8192
vmkfstools -D /vmfs/volumes/datastore2/dockvols/vol1174812-671ef9f932576c16.vmfd
Lock [type 10c00001 offset 99717120 v 3132, hb offset 3170304
gen 169, mode 0, owner 00000000-00000000-0000-000000000000 mtime 139221
num 0 gblnum 0 gblgen 0 gblbrk 0]
Addr <4, 222, 34>, gen 3108, links 1, type reg, flags 0, uid 0, gid 0, mode 600
len 4096, nb 1 tbz 0, cow 0, newSinceEpoch 1, zla 2, bs 8192
_Field owner is set_
vmkfstools -D /vmfs/volumes/datastore2/dockvols/vol1174812-flat.vmdk
Lock [type 10c00001 offset 99713024 v 3114, hb offset 3170304
gen 169, mode 1, owner 581272ea-1208c2da-516b-b8aeedeb6b16 mtime 139203
num 0 gblnum 0 gblgen 0 gblbrk 0]
Addr <4, 222, 32>, gen 3103, links 1, type reg, flags 0, uid 0, gid 0, mode 600
len 104857600, nb 15 tbz 0, cow 0, newSinceEpoch 15, zla 1, bs 1048576
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to say that when the volume is not in use the lock fields are populated the same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunotm - we cannot use a lock on -flat, because there is no -flat on VSAN or VVOL ; it would be possible but clumsy to find and check locks there
We can of course try lsof
but screen scraping is not healthy :-).
As @pdhamdhere mentioned, relying on KV to deny 'deleteVMDK' will break some cases, e.g. the following:
- volume attached, then VM powered off
- KV says "attached". Volume becomes undeletable from Docker.
A few choices for discussion:
- Use existing flow, but retain KV content (in-memory backup :-)) before delete. If delete fails recover from in-memory backup
- Use suggested PR, and add "volume delete" to vmdkops_admin (and upcoming API) for troubleshoot/recover
- something else ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) Use existing flow, but retain KV content (in-memory backup :-)) before delete. If delete fails recover from in-memory backup
is brilliant workaround!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a volume is attached and VM is powered off then lets do this,
a. check if the volume is attached and
b. check if the VM is powered off and if yes then detach the disk from the VM.
And all of this happens already for the attach path- vmdk_ops.py:handle_stale_attach() :-). Call the same for the removeVMDK path as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunotm, please call handle_stale_attach() if the volume is marked as attached. That is a better check than just to see if the volume is attached. If attached and the attaching VM is powered-off then go ahead clean up the volume config and then remove the VMDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@govint - good point about handle_stale_attach()
reuse, it seems to be simple way to improve the check. However - I am very concerned that it's getting too complex with too many if-then-else and race potentials. E.g. what would happen if the disk is attached to a VM registered on a different ESX host ? Or if VM with the disk is powered off, but for some reason KV does not show attached (I know, it "should not" happen, but the experience says that bugs exist and it could happen).
As mentioned before, this PR is not a fix but a way to reduce the probability of getting into a bad state. The code change here is straightforward with the caveat that the kv store is not authoritative for locking state. To make the source authoritative I would recommend vmfsfilelockinfo
|
The fix is good and a missing check on the removeVMDK() path. With tenant based auth and multiple VMs in a tenant and no guarantee on all those VMs running on the same host this check on the remove path is really the bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call handle_stale_attach() and if no error then proceed to remove the volume
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved once changes are rebased to current code.
…ched, and only proceed if sucessful
9b9219c
to
b4157d6
Compare
@govint |
fixes #719 #716