Add "Tenant" column in "vmdkops_admin ls" output and fix "vmdkops_admin set" command #1014

lipingxue · 2017-03-08T23:58:14Z

This PR includes:

Add a new column "Tenant" in vmdkops_admin ls command (fixes [Tenant] Add tenant column to admin ls commands #964)
Change to vmdkops_admin set command which take one more argument "--tenant". (fixes Unable to modify access attribute for volume from admin-cli #998)
admin-cli.md is changed accordingly.

lipingxue · 2017-03-09T00:04:53Z

Test for add column "Tenant" for vmdkops_admin ls output.

I have done the following test manually:
Configuration:
_DEFAULT tenant and a named tenant "tenant1"
create two volumes with the same name "vol1", which belongs to two different tenant "_DEFAULT" and "tenant1" accordingly

Step1:

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Tenant    Volume    Datastore   Created By VM  Created                   Attached To VM (name/uuid)  Policy  Capacity  Used  Disk Format  Filesystem Type  Access      Attach As              
--------  --------  ----------  -------------  ------------------------  --------------------------  ------  --------  ----  -----------  ---------------  ----------  ---------------------- 
_DEFAULT  vol1      datastore1  photon-6       Sat Sep 10 01:11:07 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  testvol   datastore1  photon-6       Sat Sep 10 04:21:02 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  Vol100MB  datastore1  photon-6       Sat Sep 10 08:36:29 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  persistent             
tenant1   vol1      datastore1  photon-6       Sat Sep 10 09:36:51 2016  detached                    N/A     500MB     23MB  thin         ext4             read-write  independent_persistent

Step2: remove "tenant1" without removing volumes, "vol1" which belongs to "tenant1" still show up in the output, but column "Tenant" for that volume shows as "N/A" since "tenant1" has been removed.

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant ls
Uuid                                  Name      Description               Default_datastore  VM_list          
------------------------------------  --------  ------------------------  -----------------  ---------------- 
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default tenant                                      
d91ad806-a9e6-447f-a464-fbd5314db85f  tenant1                                                photon7,photon-6 
 
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant rm --name=tenant1
tenant rm succeeded

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Tenant    Volume    Datastore   Created By VM  Created                   Attached To VM (name/uuid)  Policy  Capacity  Used  Disk Format  Filesystem Type  Access      Attach As              
--------  --------  ----------  -------------  ------------------------  --------------------------  ------  --------  ----  -----------  ---------------  ----------  ---------------------- 
_DEFAULT  vol1      datastore1  photon-6       Sat Sep 10 01:11:07 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  testvol   datastore1  photon-6       Sat Sep 10 04:21:02 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  Vol100MB  datastore1  photon-6       Sat Sep 10 08:36:29 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  persistent             
N/A       vol1      datastore1  photon-6       Sat Sep 10 09:36:51 2016  detached                    N/A     500MB     23MB  thin         ext4             read-write  independent_persistent

lipingxue · 2017-03-09T00:11:11Z

Test for add argument "--tenant" for vmdkops_admin set command
Step1: run vmdkops_admin ls command

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Tenant    Volume    Datastore   Created By VM  Created                   Attached To VM (name/uuid)  Policy  Capacity  Used  Disk Format  Filesystem Type  Access      Attach As              
--------  --------  ----------  -------------  ------------------------  --------------------------  ------  --------  ----  -----------  ---------------  ----------  ---------------------- 
_DEFAULT  vol1      datastore1  photon-6       Sat Sep 10 01:11:07 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  testvol   datastore1  photon-6       Sat Sep 10 04:21:02 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  Vol100MB  datastore1  photon-6       Sat Sep 10 08:36:29 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  persistent             
N/A       vol1      datastore1  photon-6       Sat Sep 10 09:36:51 2016  detached                    N/A     500MB     23MB  thin         ext4             read-write  independent_persistent

Step2: run vmdkops_admin set with non-exist tenant "DEFAULT0"

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py set --volume=Vol100MB@datastore1 --tenant=_DEFAULT0 --options="access=read-only"
Failed to update access=read-only for Vol100MB@datastore1.

Step3: run vmdkops_admin set with exist tenant "_DEFAULT"

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py set --volume=Vol100MB@datastore1 --tenant=_DEFAULT --options="access=read-only"
Successfully updated settings for : Vol100MB@datastore1
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Tenant    Volume    Datastore   Created By VM  Created                   Attached To VM (name/uuid)  Policy  Capacity  Used  Disk Format  Filesystem Type  Access      Attach As              
--------  --------  ----------  -------------  ------------------------  --------------------------  ------  --------  ----  -----------  ---------------  ----------  ---------------------- 
_DEFAULT  vol1      datastore1  photon-6       Sat Sep 10 01:11:07 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  testvol   datastore1  photon-6       Sat Sep 10 04:21:02 2016  detached                    N/A     100MB     13MB  thin         ext4             read-write  independent_persistent 
_DEFAULT  Vol100MB  datastore1  photon-6       Sat Sep 10 08:36:29 2016  detached                    N/A     100MB     13MB  thin         ext4             read-only   persistent             
N/A       vol1      datastore1  photon-6       Sat Sep 10 09:36:51 2016  detached                    N/A     500MB     23MB  thin         ext4             read-write  independent_persistent 
 
[root@localhost:~]

shuklanirdesh82 · 2017-03-09T02:35:22Z

esx_service/cli/vmdkops_admin.py

@@ -527,14 +531,18 @@ def ls_dash_c(columns, tenant_reg):

 def all_ls_headers():
    """ Return a list of all header for ls -l """
-    return ['Volume', 'Datastore', 'Created By VM', 'Created',
+    return ['Tenant', 'Volume', 'Datastore', 'Created By VM', 'Created',


I would vote for displaying Volume Name as the first column ... can you please accommodate the change?

msterin

Good fix. LGTM pending changing column order

I agree with @shuklanirdesh82 that volume should be first
I think "(name/uuid)" in the "Atatched to VM" column header is not needed, it just makes table wider
I suggest having the following columns order:
Volume
Datastore
VM-Group
Capacity
Used
Filesystem
Attached-to
Access
Attach-as
Created By
Created Date

shaominchen

Agree with Mark and Nirdesh's comments. Few more nits below.

shaominchen · 2017-03-09T19:08:20Z

esx_service/cli/vmdkops_admin.py

@@ -415,6 +415,10 @@ def commands():
                    'help': 'Volume to set options for, specified as "volume@datastore".',
                    'required': True
                },
+                '--tenant': {
+                    'help': 'Name of the tenant which volume to set belongs to.',


Nit: Suggest to rephrase a little bit: "Name of the tenant the volume belongs to"

shaominchen · 2017-03-09T19:16:10Z

esx_service/utils/auth.py

@@ -37,6 +37,7 @@
 DEFAULT_TENANT_UUID = '11111111-1111-1111-1111-111111111111'
 DEFAULT_DS  = '_DEFAULT'
 DEFAULT_DS_URL = DEFAULT_DS + "_URL"
+ORPHAN_TENANT = "_ORPHAN"


Let's define this in auth_data_const.py instead.

lipingxue · 2017-03-10T23:20:14Z

After change the order/name of columns, the output looks like:

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls
Volume    Datastore   VM-Group  Capacity  Used  Filesystem  Policy  Disk Format  Attached-to  Access      Attach-as               Created By  Created Date              
--------  ----------  --------  --------  ----  ----------  ------  -----------  -----------  ----------  ----------------------  ----------  ------------------------  
vol1      datastore1  _DEFAULT  100MB     13MB  ext4        N/A     thin         detached     read-write  independent_persistent  photon-6    Sun Sep 11 07:30:47 2016  
new-vol1  datastore1  N/A       100MB     13MB  ext4        N/A     thin         detached     read-write  independent_persistent  photon-6    Sun Sep 11 08:03:56 2016

msterin · 2017-03-10T23:27:39Z

No more comments and looks good but - why the CI is failing ? Also , need conflict resolution.

shaominchen

LGTM.

shaominchen · 2017-03-11T01:58:50Z

esx_service/cli/vmdkops_admin.py

@@ -419,7 +419,7 @@ def commands():
                    'required': True
                },
                '--tenant': {
-                    'help': 'Name of the tenant which volume to set belongs to.',
+                    'help': 'Name of the tenant the volume belongs to.',


Please make sure to resolve the conflicts with the other change which renames tenant to vm-group.

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

shuklanirdesh82 suggested changes Mar 9, 2017

View reviewed changes

msterin reviewed Mar 9, 2017

View reviewed changes

shaominchen suggested changes Mar 9, 2017

View reviewed changes

lipingxue added 3 commits March 10, 2017 15:42

Fix set_vol_opt issue.

dbaa9ee

Add column "Tenant" in admincCLI ls output.

eec86ae

Address comments from Mark, Sam and Nirdesh.

c12f94d

lipingxue force-pushed the unable_to_modify_attach_as.liping branch from f242f22 to c12f94d Compare March 10, 2017 23:54

shaominchen approved these changes Mar 11, 2017

View reviewed changes

lipingxue merged commit 172687e into master Mar 12, 2017

This was referenced Mar 13, 2017

Regression in admin_cli with choices #1024

Closed

Need to fix admin-cli.md user guide for volume part #1026

Closed

Orphaned volume should not be viewable from any docker host (not belongs to any tenant) #1031

Closed

lipingxue mentioned this pull request Mar 14, 2017

AdminCLI set command need to take "--vm-group" arg instead of "--tenant" #1035

Closed

msterin deleted the unable_to_modify_attach_as.liping branch March 22, 2017 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Tenant" column in "vmdkops_admin ls" output and fix "vmdkops_admin set" command #1014

Add "Tenant" column in "vmdkops_admin ls" output and fix "vmdkops_admin set" command #1014

lipingxue commented Mar 8, 2017 •

edited

Loading

lipingxue commented Mar 9, 2017 •

edited

Loading

lipingxue commented Mar 9, 2017

shuklanirdesh82 Mar 9, 2017

msterin left a comment

shaominchen left a comment

shaominchen Mar 9, 2017

lipingxue Mar 10, 2017

shaominchen Mar 9, 2017

lipingxue Mar 10, 2017

lipingxue Mar 10, 2017

lipingxue commented Mar 10, 2017

msterin commented Mar 10, 2017

shaominchen left a comment

shaominchen Mar 11, 2017

Add "Tenant" column in "vmdkops_admin ls" output and fix "vmdkops_admin set" command #1014

Add "Tenant" column in "vmdkops_admin ls" output and fix "vmdkops_admin set" command #1014

Conversation

lipingxue commented Mar 8, 2017 • edited Loading

lipingxue commented Mar 9, 2017 • edited Loading

lipingxue commented Mar 9, 2017

shuklanirdesh82 Mar 9, 2017

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment

shaominchen left a comment

Choose a reason for hiding this comment

shaominchen Mar 9, 2017

Choose a reason for hiding this comment

lipingxue Mar 10, 2017

Choose a reason for hiding this comment

shaominchen Mar 9, 2017

Choose a reason for hiding this comment

lipingxue Mar 10, 2017

Choose a reason for hiding this comment

lipingxue Mar 10, 2017

Choose a reason for hiding this comment

lipingxue commented Mar 10, 2017

msterin commented Mar 10, 2017

shaominchen left a comment

Choose a reason for hiding this comment

shaominchen Mar 11, 2017

Choose a reason for hiding this comment

lipingxue commented Mar 8, 2017 •

edited

Loading

lipingxue commented Mar 9, 2017 •

edited

Loading