Not returning orphan/unknown tenant volumes during docker volume ls #1114

pshahzeb · 2017-03-30T06:16:05Z

Orphan/unknown tenant volumes should not be returned as a list of volumes when a specific tenant is specified. (docker volume ls)

They must be listed only when the tenant regex is "*" i.e. volumes belonging to any tenant. Such regex comes when we list volumes as a part of admin cli volume ls

Fixes #1111

Testing:

VM1 on ESX1 belongs to tenant1
VM2 on ESX2 belongs to tenant2

Create a volume from VM1 and list volumes

root@sc-rdops-vm02-dhcp-52-237:~# docker volume create -d vsphere --name vol_tenant1_esx1
vol_tenant1_esx1
root@sc-rdops-vm02-dhcp-52-237:~# docker volume ls
DRIVER              VOLUME NAME
vsphere             vol_tenant1_esx1@sharedVmfs-0

Admin cli output on ESX1

[root@sc2-rdops-vm02-dhcp-74-64:~] /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 vm-group
c4884ced-b78d-4a33-ba11-81da39ecd5b6  tenant1_esx1                                                 ubuntu-VM0.1

[root@sc2-rdops-vm02-dhcp-74-64:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py volume ls
Volume            Datastore     VM-Group      Capacity  Used  Filesystem  Policy  Disk Format  Attached-to  Access      Attach-as               Created By    Created Date
----------------  ------------  ------------  --------  ----  ----------  ------  -----------  -----------  ----------  ----------------------  ------------  ------------------------
vol_tenant1_esx1  sharedVmfs-0  tenant1_esx1  100MB     13MB  ext4        N/A     thin         detached     read-write  independent_persistent  ubuntu-VM0.1  Thu Mar 30 02:14:20 2017

docker volume ls on VM2. Can't see volume created by VM1.

root@sc-rdops-vm02-dhcp-52-237:~# docker volume ls
DRIVER              VOLUME NAME

Admin cli output on ESX2.

[root@sc2-rdops-vm02-dhcp-78-203:~] /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 vm-group
2daf7cb1-b74c-4ef8-a617-56a001011980  tenant2_esx2                                                 ubuntu-VM0.2

[root@sc2-rdops-vm02-dhcp-78-203:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py volume ls
Volume            Datastore     VM-Group  Capacity  Used  Filesystem  Policy  Disk Format  Attached-to  Access      Attach-as               Created By    Created Date
----------------  ------------  --------  --------  ----  ----------  ------  -----------  -----------  ----------  ----------------------  ------------  ------------------------
vol_tenant1_esx1  sharedVmfs-0  N/A       100MB     13MB  ext4        N/A     thin         detached     read-write  independent_persistent  ubuntu-VM0.1  Thu Mar 30 02:14:20 2017

CC @ashahi1

Checking the tenant reg exp to see if the call was made to return volumes from any tenant and returning orphan/unknown tenant volumes only in such cases. Fixes #1111

msterin

the fix looks good , but some style comments.
Also, tenant=None version seems to be broken - was not a part of this PR. but since you are touching this code anyways...

msterin · 2017-03-30T06:29:40Z

esx_service/utils/vmdk_utils.py

-                                        'filename': file_name,
-                                        'datastore': datastore,
-                                        'tenant' : auth_data_const.ORPHAN_TENANT})
+


Line 130 looks broken to me - path is .../dockvol, and should be ../dockvol/__DEFAULT.

msterin · 2017-03-30T06:33:13Z

esx_service/utils/vmdk_utils.py

+                        for file_name in list_vmdks(root):
+                            volumes.append({'path': root,
+                                            'filename': file_name,
+                                            'datastore': datastore,


btw , L168-173 more python-ish would be something like this

volumes += [ {'path': root, 'filename': file_name, 'datastore': datastore, 'tenant' : auth_data_const.ORPHAN_TENANT} for file_name in list_vmdks(root)]

also applies to other places in this function

msterin · 2017-03-30T06:34:25Z

esx_service/utils/vmdk_utils.py

-                                        'tenant' : auth_data_const.ORPHAN_TENANT})
+
+                    # return orphan volumes only in case when volumes from any tenants are asked
+                    if tenant_re == "*":


nit:
I think it's better to say

if tenant != '*' return volumes

makes much less indent and easier to read.

pshahzeb · 2017-03-30T18:51:29Z

@msterin
True that tenant=None version is broken and doesn't hold valid in current state where all volumes are created inside tenant directories. Getting rid of this code would also require rewriting a couple of old test cases we have in vmdkops_admin_test
I think the entire change above and refactoring with your suggestions can be separately done after the .13 cut.
CC @lipingxue

pshahzeb · 2017-03-30T19:03:08Z

Created #1121 for this.

lipingxue

LGTM

msterin · 2017-03-31T01:16:50Z

LGTM for now - but please file an issue for the bug and the refactor.

pshahzeb · 2017-03-31T01:57:27Z

#1121 Issue to track refactoring of get_volumes() util and fixing the related test cases as well

Not returning orphan/unknown tenant volumes during docker volume ls

e2d590b

Checking the tenant reg exp to see if the call was made to return volumes from any tenant and returning orphan/unknown tenant volumes only in such cases. Fixes #1111

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

msterin suggested changes Mar 30, 2017

View reviewed changes

pshahzeb mentioned this pull request Mar 30, 2017

Refactoring get_volumes implementation in vmdk_utils #1121

Closed

lipingxue approved these changes Mar 30, 2017

View reviewed changes

triggering CI

63fdcf3

msterin approved these changes Mar 31, 2017

View reviewed changes

pshahzeb merged commit bd24c33 into master Mar 31, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Not returning orphan/unknown tenant volumes during docker volume ls #1114

Not returning orphan/unknown tenant volumes during docker volume ls #1114

pshahzeb commented Mar 30, 2017

msterin left a comment

msterin Mar 30, 2017

msterin Mar 30, 2017

msterin Mar 30, 2017

pshahzeb commented Mar 30, 2017

pshahzeb commented Mar 30, 2017

lipingxue left a comment

msterin commented Mar 31, 2017

pshahzeb commented Mar 31, 2017

Not returning orphan/unknown tenant volumes during docker volume ls #1114

Not returning orphan/unknown tenant volumes during docker volume ls #1114

Conversation

pshahzeb commented Mar 30, 2017

msterin left a comment

Choose a reason for hiding this comment

msterin Mar 30, 2017

Choose a reason for hiding this comment

msterin Mar 30, 2017

Choose a reason for hiding this comment

msterin Mar 30, 2017

Choose a reason for hiding this comment

pshahzeb commented Mar 30, 2017

pshahzeb commented Mar 30, 2017

lipingxue left a comment

Choose a reason for hiding this comment

msterin commented Mar 31, 2017

pshahzeb commented Mar 31, 2017