In multinode mode, disallow deleting vmgroup which are not empty #1214

pshahzeb · 2017-05-03T01:16:21Z

If vmgroup contains VMs, do now allow vmgroup rm. This is due to limitation of ESX host doesn't have information about VM residing on another host. This limitation only happens in MultiNode mode, but to keep consistency, we are enforcing this in both SingleNode and MultiNode.

Following are the test.
Two esx hosts, configured in multinode and each of them have one VMs. They are added to a shared vmgroup.

ESX1:

[root@sc2-rdops-vm06-dhcp-219-119:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup create --name sharedVMGroup --vm-list ubuntu-VM0.1 --default-datastore sharedVmfs-0
vmgroup 'sharedVMGroup' is created. Do not forget to run 'vmgroup vm add' to add vm to vmgroup.
[root@sc2-rdops-vm06-dhcp-219-119:~] /usr/lib/vmware/vmdkops/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
b44b173a-cbe6-4d05-ae3c-8c90e3287558  sharedVMGroup                             sharedVmfs-0       ubuntu-VM0.1

ESX2

[root@sc2-rdops-vm06-dhcp-209-223:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup vm add --name sharedVMGroup --vm-list ubuntu-VM0.2
vmgroup vm add succeeded

[root@sc2-rdops-vm06-dhcp-209-223:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup vm ls --name sharedVMGroup
Uuid                                  Name
------------------------------------  ------------
564d1a2a-8d8e-1c4f-a1b6-dacf99d3e2b0  ubuntu-VM0.1
564db3f0-b9e9-d036-51f2-c83d54edd602  ubuntu-VM0.2

[root@sc2-rdops-vm06-dhcp-209-223:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup rm --name sharedVMGroup
Cannot delete non empty Vmgroup sharedVMGroup in Multinode mode. Remove VMs from the Vmgroup before deleting it.

Fixes #1188

shuklanirdesh82 · 2017-05-03T01:43:20Z

esx_service/utils/auth_api.py

    # check if vms that are a part of this tenant have any volumes mounted.
    # If they have, can't delete the tenant.
    if tenant.vms:
        logging.info("_tenant_rm. VMs in tenant are %s", tenant.vms)

+        # in multinode mode, we cannot find VM residing on another host
+        # so a gracefull way to admin know about the error


nit: graceful?

Need to fix the inline comment something like in multinode mode, we cannot find VM residing on another host so stopping tenant deletion gracefully

shuklanirdesh82 · 2017-05-03T01:47:02Z

esx_service/utils/error_code.py

@@ -87,6 +88,7 @@ class ErrorCode:
    ErrorCode.TENANT_SET_ACCESS_PRIVILEGES_FAILED : "Vmgroup {0} set access privileges on datastore {1} failed with err: {2}",
    ErrorCode.TENANT_GET_FAILED : "Get vmgroup {0} failed",
    ErrorCode.TENANT_NAME_INVALID : "Vmgroup name {0} is invalid, only {1} is allowed",
+    ErrorCode.TENANT_NOT_EMPTY : "Cannot delete non empty Vmgroup {0} in Multinode mode. Remove VMs from the Vmgroup before deleting it.",


nit: s/Vmgroup/vmgroup/g

Which also becomes stop containers in VMs before moving from vmgroup.

To which vmgroup should the VMs be moved to?

Pls. update documentation to specify the steps to do this.

Beginning to think these x-vmgroup (cross vmgroup) movements of VMs are actually parallels to vmotion!! If a VM has to be moved from one VM group to another we chould possibly do this (definitely not this PR):

Add access to the currently used datastores to the destination vmgroup (option to --retain current location of disks) - its like vmotion where the VM and its containers can continue using the same datastores as before. This is doable now.

Or, the VM is moved to the new VM group and the disks are migrated by initiating a storage vmotion of the in use disks (independent persistent disks cannot be migrated for now, may be with FCD this will be doable later).

govint

Pls. document the steps for a user (from container level) what they must do before removing a vmgroup.

vmgroups are logical collections with physical entities - VMs and datastores. And movements between vmgroups can be impactful right from stopping user workloads (services, apps) and must be done very rarely with tested approaches.

This PR needs tests for the proposed approach to handling VM migration out of a vmgroup. Or make a separate PR for the doc changes.

pshahzeb · 2017-05-03T23:54:59Z

@govint makes sense.
I can make a separate PR for the steps to handle VM migration out of a vmgroup.

pshahzeb · 2017-05-04T02:59:49Z

Did the doc update here itself.
@govint @shuklanirdesh82 @shaominchen Please take a look

shaominchen

I do have some concern about the consistency: with this change, the behavior between SingleNode and MultiNode will be inconsistent: In SingleNode we do allow deleting non-empty vm-group, but in MultiNode we don't. For now with current design we don't seem to have better solutions.

shaominchen · 2017-05-04T04:47:31Z

docs/user-guide/admin-cli.md

@@ -283,6 +283,11 @@ Uuid                                  Name      Description                 Defa
 11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup
 ```

+In multinode mode, VMs from different hosts can be a part of a vmgroup.


Let's keep the terms in document consistent:

multinode => MultiNode

vmgroup => vm-group

Done. We moved from vm-group to vmgroup everywhere.

shaominchen · 2017-05-04T04:49:01Z

docs/user-guide/admin-cli.md

@@ -283,6 +283,11 @@ Uuid                                  Name      Description                 Defa
 11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup
 ```

+In multinode mode, VMs from different hosts can be a part of a vmgroup.
+When in this mode, vmgroup which has member VMs in it cannot be directly deleted.
+First remove remove the VMs individually from the vmgroup using admin cli


Nit: duplicate "remove"

shaominchen · 2017-05-04T04:53:26Z

docs/user-guide/admin-cli.md

+they belong is not permitted. Make sure no volumes are attached.
+To do so:
+1. Get the list of containers running. (docker ps)
+2. If the container has any dvs volume mounted (docker inspect container_name), stop the container.


Nit: dvs => vDVS

shaominchen · 2017-05-04T04:54:27Z

esx_service/utils/auth_api.py

@@ -559,22 +559,29 @@ def _tenant_rm(name, remove_volumes=False):
        error_info = generate_error_info(ErrorCode.TENANT_NOT_EXIST, name)
        return error_info

+    error_info, auth_mgr = get_auth_mgr_object()
+


Nit: remove this empty line

shaominchen · 2017-05-04T04:56:02Z

esx_service/utils/error_code.py

@@ -87,6 +88,7 @@ class ErrorCode:
    ErrorCode.TENANT_SET_ACCESS_PRIVILEGES_FAILED : "Vmgroup {0} set access privileges on datastore {1} failed with err: {2}",
    ErrorCode.TENANT_GET_FAILED : "Get vmgroup {0} failed",
    ErrorCode.TENANT_NAME_INVALID : "Vmgroup name {0} is invalid, only {1} is allowed",
+    ErrorCode.TENANT_NOT_EMPTY : "Cannot delete non empty vmgroup {0} in Multinode mode. Remove VMs from the vmgroup before deleting it.",


Nit: non empty => non-empty; Multinode => MultiNode

pshahzeb · 2017-05-04T15:33:54Z

@shaominchen I agree. Once we have some central control, we can get rid of such strict checks.

shaominchen

LGTM.

shaominchen · 2017-05-04T17:03:34Z

docs/user-guide/admin-cli.md

@@ -283,6 +283,11 @@ Uuid                                  Name      Description                 Defa
 11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup
 ```

+In MultiNode mode, VMs from different hosts can be a part of a vmgroup.


Nit: we are using "vmgroup" and "vm-group" in the same document - better to use same term consistently.

Done. In some places it was still vm-group. Replaced it with vmgroup

shuklanirdesh82

some minor/nits. code looks OK to me.

Please supply comment before merging it.

shuklanirdesh82 · 2017-05-04T21:33:52Z

docs/user-guide/admin-cli.md

@@ -283,6 +283,11 @@ Uuid                                  Name      Description                 Defa
 11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup
 ```

+In MultiNode mode, VMs from different hosts can be a part of a vmgroup.
+When in this mode, vmgroup which has member VMs in it cannot be directly deleted.


nit: cannot be directly deleted => cannot be deleted directly.

shuklanirdesh82 · 2017-05-04T21:35:55Z

docs/user-guide/admin-cli.md

+To do so:
+1. Get the list of containers running. (docker ps)
+2. If the container has any vDVS volume mounted (docker inspect container_name), stop the container.
+3. Ensure that the dvs volumes have status detached (docker volume inspect)


nit: dvs => vDVS, let's stick with the same name to avoid confusion or may be just vsphere driver volumes to keep it simple.

* Update README.md * Update README.md

pdhamdhere · 2017-05-05T16:07:07Z

esx_service/utils/auth_api.py

+            error_info = generate_error_info(ErrorCode.TENANT_NOT_EMPTY, name)
+            logging.error(error_info.msg)
+            return error_info
+
        error_info = vmdk_utils.check_volumes_mounted(tenant.vms)


L578 is dead code right? Should be remvoed.

pdhamdhere · 2017-05-05T16:08:22Z

esx_service/utils/auth_api.py

    # check if vms that are a part of this tenant have any volumes mounted.
    # If they have, can't delete the tenant.
    if tenant.vms:
        logging.info("_tenant_rm. VMs in tenant are %s", tenant.vms)

+        # in multinode mode, we cannot find VM residing on another host so stopping
+        # tenant deletion gracefully
+        if auth_mgr.mode == auth_data.DBMode.MultiNode:


Why only in MultiNode? Keep it consistent and also simplifies code by avoiding unnecessary ifs

I had the same comment above. I will take over and address this.

* Rescanning refCounts if first attempt during driver init fails 1. If the attempt during first refCounting during driver init fails, create a timer and redo the refCounting after a delay. 2. Refcounting with single map and synchronization between mounts and refcounting thread. Skipping unmounts until refcount has been successful. exponential backoff delay to schedule a refcount reattempt.

* Revert "Rescanning refCounts if first attempt during driver init fails (#1178)" This reverts commit 40ab3be. * Revert "Update README.md [SKIP CI] (#1222)" This reverts commit 9da7475.

)

pdhamdhere

LGTM

pdhamdhere · 2017-05-08T21:04:06Z

@pshahzeb Please fix address CI failures and merge.

shaominchen · 2017-05-09T16:13:45Z

@pdhamdhere I've taken over this PR. Some test cases are broken due to the change that a tenant to be removed must be empty. I'm fixing this right now.

… must be empty

Fixes #1188

… must be empty

…r-volume-vsphere into issue_1188.pshahzeb

shaominchen · 2017-05-09T19:02:15Z

@pshahzeb @shuklanirdesh82 Can you please take a quick look at my latest commits, which fixed the broken tests?

d47694d
3bac992

shuklanirdesh82 · 2017-05-09T20:19:08Z

@shaominchen Please rebase with master correctly .. this PR's diff is showing unwanted files.

shaominchen · 2017-05-09T21:20:29Z

@shuklanirdesh82 It's a little bit tricky to fix the wrong commit history due to rebase. I will simply create a new PR for this.

…h ESXs (See original PR #1214 for the details of review comments)

shaominchen · 2017-05-09T23:02:06Z

Replaced with PR #1231

…h ESXs (#1231) * Fix issue #1188: vmgroup is not removable when vms are added from both ESXs (See original PR #1214 for the details of review comments) * Trivial: add a new line at the end of the file * Addressed Nirdesh's comments.

In multinode mode, disallow deleting vmgroup which are not empty

ceafdd7

Fixes #1188

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

shuklanirdesh82 reviewed May 3, 2017

View reviewed changes

govint reviewed May 3, 2017

View reviewed changes

addressing comments and adding notes in docs

ba0cb0d

pshahzeb force-pushed the issue_1188.pshahzeb branch from 53dec06 to ba0cb0d Compare May 4, 2017 02:59

pshahzeb requested a review from shaominchen May 4, 2017 02:59

shaominchen suggested changes May 4, 2017

View reviewed changes

Addressing comments from Sam

fa24b76

shaominchen approved these changes May 4, 2017

View reviewed changes

Minor. Replacing vm-group with vmgroup in a couple of places left over

50219a9

shuklanirdesh82 approved these changes May 4, 2017

View reviewed changes

Update README.md [SKIP CI] (#1222)

9da7475

* Update README.md * Update README.md

pdhamdhere reviewed May 5, 2017

View reviewed changes

Shaomin Chen and others added 4 commits May 5, 2017 11:53

Addressed Prashant's comments.

d64913a

Revert "Update README.md [SKIP CI]" (#1228)

9134fdd

* Revert "Rescanning refCounts if first attempt during driver init fails (#1178)" This reverts commit 40ab3be. * Revert "Update README.md [SKIP CI] (#1222)" This reverts commit 9da7475.

Supplying empty string for network type in config.json (PR #1225) (#1229

1712a34

)

pdhamdhere approved these changes May 8, 2017

View reviewed changes

Shaomin Chen and others added 6 commits May 9, 2017 10:31

Fix broken tests due to the behavior change that tenant to be removed…

7c9715c

… must be empty

In multinode mode, disallow deleting vmgroup which are not empty

2ecc30c

Fixes #1188

addressing comments and adding notes in docs

998e9d9

Addressing comments from Sam

12247fa

Minor. Replacing vm-group with vmgroup in a couple of places left over

25c13d1

Addressed Prashant's comments.

5b5cb2f

Shaomin Chen added 3 commits May 9, 2017 11:20

Fix broken tests due to the behavior change that tenant to be removed…

d47694d

… must be empty

Merge branch 'issue_1188.pshahzeb' of https://github.com/vmware/docke…

7ab8fa4

…r-volume-vsphere into issue_1188.pshahzeb

Move cleanup_test method to test_utils

3bac992

shaominchen pushed a commit that referenced this pull request May 9, 2017

Fix issue #1188: vmgroup is not removable when vms are added from bot…

31d79a6

…h ESXs (See original PR #1214 for the details of review comments)

shaominchen mentioned this pull request May 9, 2017

Fix issue #1188: vmgroup is not removable when vms are added from both ESXs #1231

Merged

shaominchen closed this May 9, 2017

shuklanirdesh82 deleted the issue_1188.pshahzeb branch May 13, 2017 13:47

In multinode mode, disallow deleting vmgroup which are not empty #1214

In multinode mode, disallow deleting vmgroup which are not empty #1214

Conversation

pshahzeb commented May 3, 2017 • edited by shaominchen

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 3, 2017 • edited

Choose a reason for hiding this comment

govint left a comment • edited

Choose a reason for hiding this comment

pshahzeb commented May 3, 2017

pshahzeb commented May 4, 2017 • edited

shaominchen left a 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

pshahzeb commented May 4, 2017

shaominchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuklanirdesh82 left a 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

pdhamdhere left a comment

Choose a reason for hiding this comment

pdhamdhere commented May 8, 2017

shaominchen commented May 9, 2017

shaominchen commented May 9, 2017

shuklanirdesh82 commented May 9, 2017

shaominchen commented May 9, 2017

shaominchen commented May 9, 2017

pshahzeb commented May 3, 2017 •

edited by shaominchen

govint May 3, 2017 •

edited

govint left a comment •

edited

pshahzeb commented May 4, 2017 •

edited