Handling tenancy related edge cases #1060

pshahzeb · 2017-03-21T22:20:54Z

Disallow tenant rm, tenant vm rm, tenant vm replace when VMs which are part of the given tenant have any volumes currently mounted and being used. Safe approach to alert the user so that he can complete the ongoing jobs and then manipulate the tenancy membership changes.
Send empty list as output to docker volume ls command to forcefully make docker
print emtpy list and not the cached volume names
[Minor] print to indicate negative test case and avoid CI output confusion

Fixes #1045
Create tenant1 and add a vm to it.

[root@sc-rdops-vm05-dhcp-133-3:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group ls
Uuid                                  Name      Description               Default_datastore  VM_list
------------------------------------  --------  ------------------------  -----------------  ------------
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default tenant
b06c6101-6b01-44c5-a318-09bb85392fd9  tenant1                                                photon-VM0.1

Create a volume and attach it to a container inside this VM. Keep the container running.

root@photon-xVmYMbyTn [ ~ ]# docker volume create --driver=vsphere --name  myVolume
myVolume
root@photon-xVmYMbyTn [ ~ ]# docker run -it --volume-driver=vsphere -v myVolume@sharedVmfs-0:/vol1 --name ub ubuntu
root@ada941b998cf:/#

Try to remove the VM from tenant, replace the VM with another VM and try to delete the tenant.
Error with appropriate message

[root@sc-rdops-vm05-dhcp-133-3:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group vm rm --name tenant1 --vm-list photon-VM0.1
Cannot complete vm-group vm rm. VM 'photon-VM0.1' has volumes mounted.
[root@sc-rdops-vm05-dhcp-133-3:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group vm replace --name tenant1 --vm-list photon-VM1.1
Cannot complete vm-group vm replace. VM 'photon-VM0.1' has volumes mounted.
[root@sc-rdops-vm05-dhcp-133-3:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group rm --name tenant1
Cannot complete tenant rm. VM 'photon-VM0.1' has volumes mounted.

VM is orphan now. docker volume ls shows empty list of volumes

root@photon-xVmYMbyTn [ ~ ]# docker volume ls
DRIVER              VOLUME NAME

Fixes #990
Create a volume from a VM and then delete the VM's tenant

root@photon-xVmYMbyTn [ ~ ]# docker volume create --driver=vsphere --name i_will_be_orphan
i_will_be_orphan
root@photon-xVmYMbyTn [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             i_will_be_orphan@sharedVmfs-0

[root@sc-rdops-vm05-dhcp-133-3:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group ls
Uuid                                  Name     Description  Default_datastore  VM_list
------------------------------------  -------  -----------  -----------------  ------------
b06c6101-6b01-44c5-a318-09bb85392fd9  tenant1                                  photon-VM0.1

[root@sc-rdops-vm05-dhcp-133-3:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group rm --name tenant1
vm-group rm succeeded

docker volume ls from the VM gives empty list

root@photon-xVmYMbyTn [ ~ ]# docker volume ls
DRIVER              VOLUME NAME

shaominchen

LGTM. Just one comment which is just refactoring the code - you can decide if you want to address it in a separate change or not.

shaominchen · 2017-03-22T18:15:22Z

esx_service/vmdk_ops.py

@@ -786,9 +786,23 @@ def executeRequest(vm_uuid, vm_name, config_path, cmd, full_vol_name, opts):
    vm_datastore_url = vmdk_utils.get_datastore_url_from_config_path(config_path)
    vm_datastore = get_datastore_name(vm_datastore_url)

+    # Detach is implicity allowed if a disk has been attached
+    # Do not find tenant or privileges; just detach the disk
+    if cmd == "detach":


Can we introduce an enum to define all the commands?

Good suggestion, but outside of scope for this PR. @shaominchen - Please file an issue (or code it up and submit a PR)

Generally, executeRequest() became too messy. It was a small function with a switch for 5 simple commands, now it's quite complex. Would be nice to clean up. Certainly not in this PR

Filed issue #1070.

pdhamdhere · 2017-03-22T19:13:24Z

esx_service/vmdk_ops.py

@@ -656,11 +656,11 @@ def attachVMDK(vmdk_path, vm_uuid):


 # Return error, or None for OK.
-def detachVMDK(vmdk_path, vm_uuid):
+def detachVMDK(full_vol_name, vm_uuid):


I prefer full_vmdk_path rather than full_vol_name.

so is it a name (vol@DS) or a path?
A comment (in a dockstring) would be good

pdhamdhere · 2017-03-22T19:18:44Z

esx_service/vmdk_ops.py

-        return err(error_info)
+        # For docker volume ls, docker prints a list of cached volume names in case
+        # of error(in this case, orphan VM).
+        # Explicity providing empty list of volumes to avoid misleading output.


How do you differentiate between genuine error versus VM not member of any tenant?

Please add link to issue.

I'd suggest using if err_info and tenant_uuid: <read_error> and if erron_info and !tenant_uuid - no tenant

But yes, this one hides all real errors from get_tenant, if any. (mainly DB errors/file IO I guess)

@pshahzeb ?

pdhamdhere · 2017-03-22T19:23:01Z

esx_service/vmdk_ops.py

@@ -1297,16 +1311,20 @@ def err(string):
    return {u'Error': string}


-def disk_detach(vmdk_path, vm):
-    """detach disk (by full path) from a vm amd return None or err(msg)"""
+def disk_detach(full_vol_name, vm):


Same here. Since we are manipulating VMDK, I would refer this as full_vmdk_path

But the content of param 'full_vol_name' is qualified volume name in format volume@datastore
Calling it full_vmdk_path would be confusing.
I will update the param comment though

msterin

A few comments inline. The main one - I do not understand how it will behave if there are two volumes with identical (short) names on different datastores, both attached. Which one will detach ?

msterin · 2017-03-22T08:31:42Z

esx_service/cli/vmdkops_admin_test.py

@@ -832,6 +832,7 @@ def test_tenant_access(self):
                            rows[0][3]]
            self.assertEqual(expected_output, actual_output)

+        print("[Negative test case]: Expected invalid values for allow-create option")


msterin · 2017-03-23T07:15:28Z

esx_service/vmdk_ops.py

@@ -656,11 +656,11 @@ def attachVMDK(vmdk_path, vm_uuid):


 # Return error, or None for OK.
-def detachVMDK(vmdk_path, vm_uuid):
+def detachVMDK(full_vol_name, vm_uuid):


so is it a name (vol@DS) or a path?
A comment (in a dockstring) would be good

msterin · 2017-03-23T07:16:19Z

esx_service/vmdk_ops.py

@@ -786,9 +786,23 @@ def executeRequest(vm_uuid, vm_name, config_path, cmd, full_vol_name, opts):
    vm_datastore_url = vmdk_utils.get_datastore_url_from_config_path(config_path)
    vm_datastore = get_datastore_name(vm_datastore_url)

+    # Detach is implicity allowed if a disk has been attached
+    # Do not find tenant or privileges; just detach the disk
+    if cmd == "detach":


Good suggestion, but outside of scope for this PR. @shaominchen - Please file an issue (or code it up and submit a PR)

Generally, executeRequest() became too messy. It was a small function with a switch for 5 simple commands, now it's quite complex. Would be nice to clean up. Certainly not in this PR

msterin · 2017-03-23T07:26:37Z

esx_service/vmdk_ops.py

+    if cmd == "detach":
+        with lockManager.get_lock(vm_uuid):
+            response = detachVMDK(full_vol_name, vm_uuid)
+


nit (can be ignored): generally I'd prefer to have a simple dict-driven code, with something like {"attach": ["auth_needed": True, "func": "code", ,,} ...} to drive the execution for commands. (this is the same note as the above reply to @shaominchen ).

msterin · 2017-03-23T07:28:57Z

esx_service/vmdk_ops.py

-        return err(error_info)
+        # For docker volume ls, docker prints a list of cached volume names in case
+        # of error(in this case, orphan VM).
+        # Explicity providing empty list of volumes to avoid misleading output.


I'd suggest using if err_info and tenant_uuid: <read_error> and if erron_info and !tenant_uuid - no tenant

But yes, this one hides all real errors from get_tenant, if any. (mainly DB errors/file IO I guess)

msterin · 2017-03-23T07:32:37Z

esx_service/vmdk_ops.py

+        logging.debug("findDeviceByName: datastore = %s, backing_disk_name = %s",
+                        datastore, backing_disk_name)
+
+        if backing_disk_name == vol_name:


So what if there are 2 Docker volumes with the same names on different datastores. both attached ? Which one will be detached? I do not see DS names used in matching.... not 'dockvols' part of the path

@msterin
Fixing it to compare both volume name and datastore name with backing disk name and backing disk datastore.
Confirmed that we always get a qualified volume name i.e. volumename@datastore; from docker -> vmdk_plugin -> esx host.

msterin · 2017-03-23T07:35:17Z

esx_service/vmdk_ops.py

@@ -1071,7 +1084,8 @@ def handle_stale_attach(vmdk_path, kv_uuid):
          if cur_vm.runtime.powerState == VM_POWERED_OFF:
             logging.info("Detaching disk %s from VM(powered off) - %s\n",
                             vmdk_path, cur_vm.config.name)
-             device = findDeviceByPath(vmdk_path, cur_vm)
+             vol_name = vmdk_utils.strip_vmdk_extension(os.path.basename(vmdk_path))


strip extension should be responsibility of the caller, but instead done in findDeviceByName (if needed)

pdhamdhere · 2017-03-25T01:27:46Z

@pshahzeb Is it ready for review? Looks like you are still addressing review comments?

pshahzeb · 2017-03-27T07:17:18Z

Yes. This fix fails to detach volumes with two same short names but on different DS. we get short name for volume for detach during volume create process.
Working on it.

lipingxue · 2017-03-30T17:21:48Z

esx_service/utils/vmdk_utils.py

+            # Filename format is as follows:
+            # "[<datastore name>] <parent-directory>/tenant/<vmdk-descriptor-name>"
+            # Trim the datastore name and keep disk path.
+            _, disk_path = d.backing.fileName.split(None, 1)


This cannot trim the datastore correctly if whitespaces are included in the datastore name.

fileName = "[datastor e1] /dockvols/tenant1/vol1.vmdk"

_, disk_path = fileName.split(None, 1)
print disk_path
e1] /dockvols/tenant1/vol1.vmdk

Use "]" as a separator.

lipingxue · 2017-03-30T17:23:46Z

esx_service/vmdk_ops.py

-def disk_detach(full_vol_name, vm):
-    """detach disk (by full_vol_name) from a vm and return None or err(msg)"""
+def disk_detach(vmdk_path, vm):
+    """detach disk (by full path) from a vm amd return None or err(msg)"""


Typo: amd -> and

1. Disallowing tenant-vm membership change when volumes are mounted and in use by VMs 2. Send empty list as output to docker volume ls command to forcefully make docker print emtpy list and not the cached volume names 3. Minor print to indicate negative test case and avoid CI output confusion Fixes #990 #1045

shaominchen

Looks good. A few nits.

shaominchen · 2017-03-30T23:08:58Z

esx_service/utils/auth_api.py

@@ -409,6 +409,17 @@ def _tenant_rm(name, remove_volumes=False):
        error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name)
        return error_info

+    # check if vms that are a part if this tenant have any volumes mounted.


Nit: "if this tenant" => "of this tenant"

shaominchen · 2017-03-30T23:11:25Z

esx_service/utils/auth_api.py

+    # check if vms that are a part if 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)


Nit: "tenant rm" => "_tenant_rm", just for consistency.

shaominchen · 2017-03-30T23:14:26Z

esx_service/utils/error_code.py

@@ -75,6 +76,7 @@ class ErrorCode:
    ErrorCode.VM_IN_ANOTHER_TENANT : "VM '{0}' has already been associated with tenant '{1}', can't add it",
    ErrorCode.VM_LIST_EMPTY : "VM list cannot be empty",
    ErrorCode.VM_DUPLICATE : "VMs {0} contain duplicates, they should be unique",
+    ErrorCode.VM_WITH_MOUNTED_VOLUMES :"VM '{0}' has volumes mounted.",


Nit: add a space after ":"

shaominchen · 2017-03-30T23:15:31Z

esx_service/utils/vmdk_utils.py

@@ -307,6 +311,32 @@ def get_vm_config_path(vm_name):
    vm_config_path = os.path.join(datastore_path, path)
    return vm_config_path

+def check_volumes_mounted(vm_list):
+    """
+    Return error_info any vm in @param vm_list have docker volume mounted


Nit: add an "if" before "any vm"

shaominchen · 2017-03-30T23:19:51Z

esx_service/utils/vmdk_utils.py

+                # Trim the datastore name and keep disk path.
+                _, disk_path = d.backing.fileName.rsplit("]", 1)
+                logging.info("backing disk name is %s", disk_path)
+                # If disk path start with "dockvols", the disk is a docker volume


Can we extract the logic of checking whether a disk is a docker volume (322-332) to a separate method?

lipingxue

LGTM

shaominchen

LGTM

vmwclabot added the cla-not-required label Mar 21, 2017

shaominchen approved these changes Mar 22, 2017

View reviewed changes

pdhamdhere reviewed Mar 22, 2017

View reviewed changes

msterin suggested changes Mar 23, 2017

View reviewed changes

pshahzeb force-pushed the tenancy_edge_cases.pshahzeb branch 2 times, most recently from d14432c to 2e48603 Compare March 29, 2017 22:51

pshahzeb changed the title ~~Handling tenancy related edge cases~~ [No review] Handling tenancy related edge cases Mar 29, 2017

pshahzeb changed the title ~~[No review] Handling tenancy related edge cases~~ Handling tenancy related edge cases Mar 29, 2017

lipingxue suggested changes Mar 30, 2017

View reviewed changes

pshahzeb added 2 commits March 30, 2017 14:35

Better error checks and nits

d1eb652

pshahzeb force-pushed the tenancy_edge_cases.pshahzeb branch from fe64c8d to d1eb652 Compare March 30, 2017 22:59

shaominchen reviewed Mar 30, 2017

View reviewed changes

Addressing nits and segregating docker volume check login

b02bce3

lipingxue approved these changes Mar 30, 2017

View reviewed changes

shaominchen approved these changes Mar 30, 2017

View reviewed changes

msterin approved these changes Mar 31, 2017

View reviewed changes

pshahzeb merged commit 464bf4f into master Mar 31, 2017

shuklanirdesh82 deleted the tenancy_edge_cases.pshahzeb branch March 31, 2017 07:59

pdhamdhere mentioned this pull request Apr 25, 2017

[Multi-ESX] vmgroup is not removable when vms are added from both ESXs (configured as MultiNode) #1188

Closed

pshahzeb mentioned this pull request Apr 30, 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

Handling tenancy related edge cases #1060

Handling tenancy related edge cases #1060

pshahzeb commented Mar 21, 2017 •

edited

shaominchen left a comment

shaominchen Mar 22, 2017

msterin Mar 23, 2017

shaominchen Mar 23, 2017

pdhamdhere Mar 22, 2017

msterin Mar 23, 2017

pdhamdhere Mar 22, 2017

msterin Mar 23, 2017

pdhamdhere Mar 25, 2017

pdhamdhere Mar 22, 2017

pshahzeb Mar 22, 2017

msterin left a comment

msterin Mar 22, 2017

msterin Mar 23, 2017

msterin Mar 23, 2017

msterin Mar 23, 2017

msterin Mar 23, 2017

msterin Mar 23, 2017

pshahzeb Mar 23, 2017

msterin Mar 23, 2017

pdhamdhere commented Mar 25, 2017

pshahzeb commented Mar 27, 2017

lipingxue Mar 30, 2017

lipingxue Mar 30, 2017

shaominchen left a comment

shaominchen Mar 30, 2017

shaominchen Mar 30, 2017

shaominchen Mar 30, 2017

shaominchen Mar 30, 2017

shaominchen Mar 30, 2017

lipingxue left a comment

shaominchen left a comment

Handling tenancy related edge cases #1060

Handling tenancy related edge cases #1060

Conversation

pshahzeb commented Mar 21, 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

msterin 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

pdhamdhere commented Mar 25, 2017

pshahzeb commented Mar 27, 2017

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

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

lipingxue left a comment

Choose a reason for hiding this comment

shaominchen left a comment

Choose a reason for hiding this comment

pshahzeb commented Mar 21, 2017 •

edited