Check for mounted volumes when adding VMs to a new vmgroup on create. #1190

govint · 2017-04-25T13:34:20Z

Added check in auth.api.py:_tenant_create() to check for volumes mounted before allowing to move the VMs into the VM group being created.

Test:

bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description                Default_datastore  VM_list
------------------------------------  --------  -------------------------  -----------------  -------
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup  _VM_DS

bin/vmdkops_admin.py vmgroup create --name vmg1 --vm-list 2 --default-datastore /vmfs/volumes/bigone
VMs already have volumes in use, cannot add VMs to vmgroup. VM '2' has volumes mounted.

govint · 2017-04-25T13:34:45Z

Fixes #1189

msterin · 2017-04-25T15:40:52Z

Can you elaborate what is the problem ? The bug description is not clear on this (too many symptoms and conditions). It is also not clear why a VM with volumes mounted should be blocked from moving to a group.... I thought unmount() is unrelated to specific tenant location and access right ?

shuklanirdesh82 · 2017-04-25T17:51:53Z

@msterin's comment #1190 (comment)

Can you elaborate what is the problem ? The bug description is not clear on this (too many symptoms and conditions). It is also not clear why a VM with volumes mounted should be blocked from moving to a group....

Sorry Mark for the confusion. I have updated the bug description to reduce the confusion ( #1189 ).

@govint has summarize at #1189 (comment)

govint · 2017-04-25T18:02:55Z

This PR is only to handle the check for mounted volumes in a VM when a vmgroup is being created. The check is done for vmgroup replace and vmgroup rm commands.

_DEFAULT is a vmgroup in itself so, trying to create a new vmgroup from VMs in _DEFAULT (added by default btw) should make the same check that volumes aren't already attached to those VMs.

The issue with using VM uuid is a separate issue addressed via #1188.

msterin · 2017-04-25T20:25:20Z

I still do not understand what is the issue with placing a VM with an attached volime in a group

shuklanirdesh82 · 2017-04-25T21:57:50Z

@govint, I have an offline chat with @msterin and summarizing the discussion here for your reference.

It would be a troublesome for admin by blocking "vmgroup creation with vms having volumes mounted".

There are two proposed approaches while dealing with VMs having volumes mounted on.

Approach #1: Block the request at first attempt and allow Admin to perform the operation (if still wants) by supporting usability feature of --force option.

Approach #2: Let the request go through and perform necessary cleanup/update at metadata file (KV file) whenever getting the unmount request after the operation (moving vm across vmgroup).

An approach#1 would be preferable for the admin to decide whether moving VMs across vmgroups is desirable at any given point (when volumes are still in use by some of the vms). It would be troublesome for the admin if we stop the operation until VMs are freed up. By supplying --force option should unmount all the volumes immediately for the given VM and new mount request will be served on the new datastore that reduces many corner cases too.

While going with Approach #2 (current behavior), we may run into some corner cases (unknown at this point) to maintain consistency.

By simply blocking/stopping the request doesn't sound convincing by taking account of usability from an admin point of view.

@msterin, Please update as per necessity if I have misunderstood any part.

govint · 2017-04-25T23:39:58Z

Can I suggest the approach as proposed which matches the behavior in the documentation. Flexibility to move a VM is still there to create a vmgroup but allowing a force option will need kludge's in the service coffee to handle such cases. Prefer keeping the behavior based on use case vs. by testing which is a continuous feature creep. .

govint · 2017-04-26T00:05:12Z

Plus, adding a VM to create a vmgroup has to be done on the specific esx host where the VM runs. Which itself is cumbersome. To avoid any issue with trying to verify the VM with it's uuid. .

msterin · 2017-04-28T07:27:16Z

I don't think documentation is our north star. Good user experience is. I simply do not understand what an admin is supposed to do if she wants to change a VM membership in a group, and is blocked by "volumes attached" error message.

govint · 2017-04-28T08:07:14Z

This check is in place for these actions on a vmgroup - replace, remove. And the doc says no VM in a vmgroup is allowed to have volumes in datastores not assigned to the VMgroup. User is expected to have stopped using the volumes in the datastore of the old group before assigning.

msterin · 2017-04-28T16:44:23Z

Please do answer my question. I am the admin. I receive this error. What do I do next ?

pshahzeb · 2017-05-01T21:24:32Z

@msterin I think admin can first get ensure the volumes aren't attached (may be stop the containers) and then change the membership? what do you think?

pshahzeb

LGTM

shuklanirdesh82 · 2017-05-01T23:14:37Z

#1190 (comment)
I think admin can first get ensure the volumes aren't attached (may be stop the containers) and then change the membership? what do you think?

@pshahzeb Does it show how many volumes are currently attached to which vms? Let's say in the large scale setup 10 VMs are there and some of them are having volumes mounted. There is a requirement to move 10 VM from _DEFAULT tenant to "named" tenant and with admin cli, admin can create new named tenant with all 10 vms in one shot.

what is the expected behavior for admin_cli vmgroup create with --vm-list set to all 10 vms? It would be nightmare for Admin to detach volumes one by one in addition to that admin might not be having access to docker host too. Other problem I am thinking is: when the admin in the process of detaching volumes, it is likely to be new mount request comes up.

shaominchen

The change looks good to me. I assume the same check has been added to other places where this check is needed.

msterin · 2017-05-02T01:21:52Z

@pshahzeb - which containers ? How does the admin finds them ? Specific steps/commands please. FWIW I don't think it's trivial or obvious, which is why I think --force option is goodness. But if we decide against it, we need step by step instructions on how to find offending containers.

govint · 2017-05-02T05:15:20Z

It's actually not about which containers. The VM is being moved and it can't have volumes mounted. The same applies to VM group remove and replace of vms

…

On May 2, 2017 6:51 AM, "Mark Sterin" ***@***.***> wrote: @pshahzeb <https://github.com/pshahzeb> - which containers ? How does the admin finds them ? Specific steps/commands please. FWIW I don't think it's trivial or obvious, which is why I think --force option is goodness. But if we decide against it, we need step by step instructions on how to find offending containers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1190 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APHseHH1HzaH29O8AzdzkY-G9e67QEIEks5r1oUwgaJpZM4NHfba> .

pdhamdhere · 2017-05-02T05:54:35Z

esx_service/utils/auth_api.py

@@ -472,6 +472,12 @@ def _tenant_create(name, default_datastore, description="", vm_list=None, privil
        if error_info:
            return error_info, None

+        error_info = vmdk_utils.check_volumes_mounted(vms)
+        if error_info:
+            error_info.msg = "VMs already have volumes in use, cannot add VMs to vmgroup. " + error_info.msg


error_info.msg = "Cannot add VM to vmgroup " + error_info.msg

to be consistent with other error message and error could be "VM not found"

pdhamdhere · 2017-05-02T05:55:42Z

esx_service/utils/auth_api.py

@@ -692,6 +698,12 @@ def _tenant_vm_add(name, vm_list):
    if error_info:
        return error_info

+    error_info = vmdk_utils.check_volumes_mounted(vms)
+    if error_info:
+        error_info.msg = "VMs already have volumes in use, cannot add VMs to vmgroup. " + error_info.msg


Same thing.

error_info.msg = "Cannot complete vmgroup add " + error_info.msg

pdhamdhere · 2017-05-02T06:00:28Z

@msterin Agree. Admin experience isn't ideal. However, this is defensive edge case. Generally you would expect Admin to create vmgroup first before handing over control to developers. If needed we can document (KB) this error case.

govint · 2017-05-02T06:10:38Z

User experience is even worse if some containers are running in the VM and we force move the VM to another vmgroup with volumes attached and end up unmounting the volumes and making those containers fail in any way.

The flip side is to allow access automatically in the target VM group for the datastores on which the volumes in use are located. Which is not desireable at all. Thats not the user intention to move a VM and the target group automatically gets access to some datastores. And as VMs get added to a certain group it starts accquiring access to all datastores?

From the service logs, the error was the target vmgroup refused the detach request on unmount because the vmgroup didn't have those rights on the datastore.

Supporting a force option will need intrusive special casing for these scenarios.

pdhamdhere

LGTM.

msterin · 2017-05-02T22:42:59Z

@pdhamdhere I agree that user experience being bad in edge case is acceptable, that's why I asked to document step by step what is the manual actions the admin needs to take if she does want to move the VM. IMO the end result of this will be killing containers (if she finds them) which is similar to --force option but more complex and unpredictable.

govint · 2017-05-03T05:27:06Z

@msterin @pdhamdhere I'll make a doc PR to describe how vmgroup migration can be done.

vmwclabot added the cla-not-required label Apr 25, 2017

pshahzeb requested a review from shuklanirdesh82 May 1, 2017 21:22

pshahzeb previously approved these changes May 1, 2017

View reviewed changes

pshahzeb requested a review from shaominchen May 1, 2017 21:53

shaominchen previously approved these changes May 1, 2017

View reviewed changes

pdhamdhere suggested changes May 2, 2017

View reviewed changes

govint dismissed stale reviews from shaominchen and pshahzeb via 7766ad4 May 2, 2017 06:05

govint force-pushed the check-volumes branch from b5c3c6d to 7766ad4 Compare May 2, 2017 06:05

Check for mounted volumes when adding VMs to a new vmgroup on create.

14b6279

govint force-pushed the check-volumes branch from 7766ad4 to 14b6279 Compare May 2, 2017 06:12

pdhamdhere approved these changes May 2, 2017

View reviewed changes

Merge branch 'master' into check-volumes

9b8c8fd

govint merged commit 63f0062 into master May 3, 2017

govint deleted the check-volumes branch May 3, 2017 05:27

govint mentioned this pull request May 3, 2017

Unable to unmount/detach volume after vmgroup membership change #1189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for mounted volumes when adding VMs to a new vmgroup on create. #1190

Check for mounted volumes when adding VMs to a new vmgroup on create. #1190

govint commented Apr 25, 2017 •

edited by pdhamdhere

govint commented Apr 25, 2017

msterin commented Apr 25, 2017

shuklanirdesh82 commented Apr 25, 2017

govint commented Apr 25, 2017

msterin commented Apr 25, 2017

shuklanirdesh82 commented Apr 25, 2017

govint commented Apr 25, 2017 via email

govint commented Apr 26, 2017 via email

msterin commented Apr 28, 2017

govint commented Apr 28, 2017

msterin commented Apr 28, 2017

pshahzeb commented May 1, 2017

pshahzeb left a comment

shuklanirdesh82 commented May 1, 2017

shaominchen left a comment

msterin commented May 2, 2017

govint commented May 2, 2017 via email

pdhamdhere May 2, 2017

govint May 2, 2017

pdhamdhere May 2, 2017

govint May 2, 2017

pdhamdhere commented May 2, 2017

govint commented May 2, 2017 •

edited

pdhamdhere left a comment

msterin commented May 2, 2017

govint commented May 3, 2017

Check for mounted volumes when adding VMs to a new vmgroup on create. #1190

Check for mounted volumes when adding VMs to a new vmgroup on create. #1190

Conversation

govint commented Apr 25, 2017 • edited by pdhamdhere

govint commented Apr 25, 2017

msterin commented Apr 25, 2017

shuklanirdesh82 commented Apr 25, 2017

govint commented Apr 25, 2017

msterin commented Apr 25, 2017

shuklanirdesh82 commented Apr 25, 2017

govint commented Apr 25, 2017 via email

govint commented Apr 26, 2017 via email

msterin commented Apr 28, 2017

govint commented Apr 28, 2017

msterin commented Apr 28, 2017

pshahzeb commented May 1, 2017

pshahzeb left a comment

Choose a reason for hiding this comment

shuklanirdesh82 commented May 1, 2017

shaominchen left a comment

Choose a reason for hiding this comment

msterin commented May 2, 2017

govint commented May 2, 2017 via email

pdhamdhere May 2, 2017

Choose a reason for hiding this comment

govint May 2, 2017

Choose a reason for hiding this comment

pdhamdhere May 2, 2017

Choose a reason for hiding this comment

govint May 2, 2017

Choose a reason for hiding this comment

pdhamdhere commented May 2, 2017

govint commented May 2, 2017 • edited

pdhamdhere left a comment

Choose a reason for hiding this comment

msterin commented May 2, 2017

govint commented May 3, 2017

govint commented Apr 25, 2017 •

edited by pdhamdhere

govint commented May 2, 2017 •

edited