Create volume on default datastore if default_ds is set for _DEFAULT tenant #1095

lipingxue · 2017-03-27T18:39:35Z

Fixed #1044
This PR includes the fix for:

Datastore used for the first "user created" privilege will be set to "default_ds" for that tenant.
When creating Docker volume with short name from a VM in __DEFAULT tenant, use "default_ds" (if set) , not the vm_datastore

…tenant.

lipingxue · 2017-03-29T18:20:18Z

Per offline discussion with Mark and Sam, I reverted the change in _tenant_access_add.
User need to specify --default-datastore flag when running vm-group access add to
set the default datastore for a vm-group. After setting, user can run vm-group ls to list the vm-group and the column Default_datastore will show the default datastore of that vm-group.

lipingxue · 2017-03-29T18:26:23Z

Run the following test manually.
Configuration: VM "photon-6" is created on "datastore1". This VM belongs _DEFAULT tenant.

step1: Add a privilege for _DEFAULT vm-group with "--default-datastore" flag specified to set the default datastore of _DEFAULT vm-group to "datastore2"

[root@localhost:~] /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            

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group access add --name=_DEFAULT --datastore=datastore2 --allow-create --default-datastore
vm-group access add succeeded
[root@localhost:~] 
[root@localhost:~] 
[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vm-group access ls --name=_DEFAULT
Datastore   Allow_create  Max_volume_size  Total_size  
----------  ------------  ---------------  ----------  
datastore2  True          Unset            Unset       
            True          Unset            Unset

step2: check default_datastore of vm-group _DEFAULT

[root@localhost:~] /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  datastore2

step3: create a volume with short name, make sure this volume is created on the default datastore "datastore2" instead of vm datastore "datastore1".

root@photon-KwqUODFXp [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
root@photon-KwqUODFXp [ ~ ]# 
root@photon-KwqUODFXp [ ~ ]# docker volume create --driver=vsphere --name=vol1
vol1
root@photon-KwqUODFXp [ ~ ]# 
root@photon-KwqUODFXp [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             vol1@datastore2

lipingxue · 2017-03-29T18:28:13Z

@msterin @shaominchen I have changed this PR per yesterday's offline discussion. Please review it, thanks!

shuklanirdesh82

@lipingxue

Your changes looks OK to me. Can you please add some basic automated tests with your proposed code?

Thanks!

shaominchen

LGTM.

msterin

So this fixes the main issue - "when default_ds for named tenant is set, use it instead of VM DS", right ?

The code LGTM - please make sure CI passes before merge

msterin · 2017-03-27T19:54:57Z

esx_service/utils/auth_api.py

+    """
+        Check if need update default_datastore for this tenant
+    """
+    # In _tenant_access_add, we decide to automatically set the datastore for the first user added privilege


Please run this comment by someone in the office - the comment looks very confusing to me but maybe I am reading it wrong

msterin · 2017-03-27T19:58:02Z

esx_service/utils/auth_api.py

@@ -654,6 +654,35 @@ def check_privilege_parameters(privilege):

    return None

+def need_update_default_ds(conn, tenant_id):
+    """
+        Check if need update default_datastore for this tenant


so what does it return ? Why a tuple ?

msterin · 2017-03-27T19:59:05Z

esx_service/utils/auth_api.py

@@ -654,6 +654,35 @@ def check_privilege_parameters(privilege):

    return None

+def need_update_default_ds(conn, tenant_id):


the name is confusing. I can't suggest a better name because I don't understand what this is doing. Can you elaborate ?

msterin · 2017-03-29T19:29:44Z

esx_service/vmdk_ops.py

+
+    # if default_datastore is not set for tenant,
+    # default_datastore_url will be set to None
+    error_info, default_datastore_url = auth_api.get_default_datastore_url(tenant_name)


please update the admin command line message on group creation to something like "vm-group xxx is created. Do not forget to vm-group vm add and vm-group privilege add to enable better access control"
(adjust to reflect the correct commands please)

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

@msterin @shuklanirdesh82 I have addressed your comments. Please review it.

msterin

LGTM with 1 request to change the message text before merge

msterin · 2017-03-30T00:49:54Z

esx_service/cli/vmdkops_admin.py

@@ -854,7 +854,8 @@ def tenant_create(args):
    if error_info:
        return operation_fail(error_info.msg)
    else:
-        print("vm-group create succeeded")
+        print("vm-group create succeeded. vm-group '{}' is created. Do not forget to run "
+              "command 'vm-group vm add'  and 'vm-group access add' to enable better access control.".format(args.name))


Suggestion:
"vm-group '{}' is created. Do not forget to run 'vm-group vm add' and 'vm-group access add' commands to enable access control.".format(args.name))

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

lipingxue added 2 commits March 29, 2017 10:50

Create volume on default datastore if default_ds is set for _DEFAULT …

818a75b

…tenant.

Revert changes in _tenant_access_add().

a65fb75

lipingxue force-pushed the create_vol_on_default_ds.liping branch from 0c9e3ba to a65fb75 Compare March 29, 2017 17:52

shuklanirdesh82 reviewed Mar 29, 2017

View reviewed changes

shaominchen approved these changes Mar 29, 2017

View reviewed changes

msterin reviewed Mar 29, 2017

View reviewed changes

lipingxue added 2 commits March 29, 2017 16:01

Address comments from Mark and Nirdesh.

e009d93

Minor comment fix.

1e2e54c

msterin approved these changes Mar 30, 2017

View reviewed changes

Address one more comment from Mark.

ea27638

lipingxue merged commit 67bf73c into master Mar 30, 2017

shuklanirdesh82 deleted the create_vol_on_default_ds.liping branch March 31, 2017 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create volume on default datastore if default_ds is set for _DEFAULT tenant #1095

Create volume on default datastore if default_ds is set for _DEFAULT tenant #1095

lipingxue commented Mar 27, 2017 •

edited by msterin

lipingxue commented Mar 29, 2017

lipingxue commented Mar 29, 2017

lipingxue commented Mar 29, 2017

shuklanirdesh82 left a comment

shaominchen left a comment

msterin left a comment

msterin Mar 27, 2017

msterin Mar 27, 2017

msterin Mar 27, 2017

msterin Mar 29, 2017

lipingxue commented Mar 29, 2017

msterin left a comment

msterin Mar 30, 2017

		@@ -654,6 +654,35 @@ def check_privilege_parameters(privilege):

		return None

		def need_update_default_ds(conn, tenant_id):

Create volume on default datastore if default_ds is set for _DEFAULT tenant #1095

Create volume on default datastore if default_ds is set for _DEFAULT tenant #1095

Conversation

lipingxue commented Mar 27, 2017 • edited by msterin

lipingxue commented Mar 29, 2017

lipingxue commented Mar 29, 2017

lipingxue commented Mar 29, 2017

shuklanirdesh82 left a comment

Choose a reason for hiding this comment

shaominchen left a comment

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment

msterin Mar 27, 2017

Choose a reason for hiding this comment

msterin Mar 27, 2017

Choose a reason for hiding this comment

msterin Mar 27, 2017

Choose a reason for hiding this comment

msterin Mar 29, 2017

Choose a reason for hiding this comment

lipingxue commented Mar 29, 2017

msterin left a comment

Choose a reason for hiding this comment

msterin Mar 30, 2017

Choose a reason for hiding this comment

lipingxue commented Mar 27, 2017 •

edited by msterin