Support for default_datastore and data privileges change #828

lipingxue · 2016-12-16T21:31:24Z

This change includes:

support for default_datastore (including APIs to manage default_datastore, and corresponding VMDK op changes)
change privileges from three privileges(create/remove/mount) to two ("mount_only" and "full_access")
auth DB schema change to reflect the privilege change
auth DB schema change: in "tenants" and "volumes" table, store datastore url instead of datastore name since datastore name may change; don't store vm name in table "vms" since vm name may change
add table "versions" to record auth-db schema version and corresponding logic to check it upgrade is needed or not
add unit test and change some existing unit tests to test support for default_datastore
remove some unused code in vmdkops_admin.py. Functions which have been removed have been put in auth_api.py.

lipingxue · 2016-12-16T21:32:15Z

Unit test for new feature has been included in this PR. "make test-esx" passed.

lipingxue · 2016-12-16T21:32:35Z

Please review @msterin @shaominchen.

shaominchen

Overall looks good. Please check the CI test failure.

This PR is too big to give a thorough review - at least the versions related logic should have been done in a separate PR.

shaominchen · 2016-12-16T22:46:05Z

esx_service/cli/vmdkops_admin.py

+                                },                            
+                                '--allow-create': {
+                                    'help': 'Allow create and delete on datastore if set to True',
+                                    'action': 'store_true'


I guess the most common case when user creates a privilege is to assign full permissions, so probably we should set this to "store_false" instead?

Let us keep it as is for now. We can make the change later if user complains about it.

What is default? "allow-create = true"?

shaominchen · 2016-12-16T22:47:30Z

esx_service/cli/vmdkops_admin.py

-                                    'metavar': 'create,delete,mount,all'
+                                '--allow-create': {
+                                    'help': 'Allow create and delete on datastore if set to True',
+                                    'action': 'store_true'


Same as above.

Same as above

shaominchen · 2016-12-16T22:57:59Z

esx_service/cli/vmdkops_admin.py

-        vms.append((vm_uuid, vm_name))
-
-    return None, vms
-
 def tenant_create(args):
    """ Handle tenant create command """
    error_info, tenant = auth_api._tenant_create(
                                                 name=args.name , 


Nit: remove the space before ","

shaominchen · 2016-12-16T23:05:55Z

esx_service/utils/auth.py

@@ -35,6 +35,7 @@
 SIZE = 'size'
 DEFAULT_TENANT = '_DEFAULT'
 DEFAULT_DS  = '_DEFAULT'
+DEFAULT_DS_URL = DEFAULT_DS + "_url"


Nit: let's use capital letters consistently for these constants, i.e. "_URL"

shaominchen · 2016-12-16T23:07:40Z

esx_service/utils/auth.py

+def has_privilege(privileges, type=None):
+    """ Check the privileges has the specific type of privilege set.
+        if param "allow_create" is None or False, which means check
+        the input "privileges" has "mount_only" privileges or not


Nit: "mount_only "privileges => "mount_only" privilege

shaominchen · 2016-12-16T23:34:00Z

esx_service/utils/auth_api.py

+        List all tenants or tenant with the name specified
+        Params:
+        -- name: if "name" is specified, return a list of one tenant with name specified
+        if "name" is not specified, return a list of all tenant


Nit: all tenant => all tenants

shaominchen · 2016-12-16T23:42:03Z

esx_service/utils/auth_api.py

@@ -115,84 +124,99 @@ def generate_tuple_from_vm_list(vm_list):
    return None, vms, not_found_vms

 def default_privileges():
-    """ return a privilege object with default value """


Why do we remove this comment? It seems useful to me.

Add it back.

shaominchen · 2016-12-16T23:55:22Z

esx_service/utils/auth_data.py

@@ -374,6 +376,58 @@ def create_default_privileges(self):
            err = error_code.TENANT_SET_ACCESS_PRIVILEGES_FAILED.format(auth.DEFAULT_TENANT, auth.DEFAULT_DS, error_info)
            logging.warning(err)

+    def get_db_version(self):
+        """
+            Get DB schema version from auth-db


Nit: keep the indent consistent

shaominchen · 2016-12-16T23:59:09Z

esx_service/utils/auth_data.py

+
+    def need_upgrade_db(self):
+        """
+            Check auth-db schema need to be upgraded or not


Nit: Check if auth-db ...

shaominchen · 2016-12-17T00:03:36Z

esx_service/utils/auth_data.py

+                tenant = DockerVolumeTenant(name=name,
+                                            description=description,
+                                            vms=vms,
+                                            privileges=privileges,


The current privileges field contains specific DB column information. As we discussed offline, the API layer should return a pure data object so that above layer (such as VMODL) can get the data directly without having dependency on DB specifics.

We can address this in a separate PR.

kerneltime · 2016-12-20T18:30:12Z

esx_service/cli/vmdkops_admin.py

@@ -221,6 +221,9 @@ def commands():
                        },
                        '--description': {
                            'help': 'The new description of the tenant',
+                        },
+                        '--default-datastore': {
+                            'help': 'The name of new default_datastore',


The name of the new default_datastore

another nit - the flag is default-datastore (with -) and the help says default_datastore (with underscore), the help should say "the name of the datastore to be used by default for volumes placement"

kerneltime · 2016-12-20T18:33:08Z

What's the plan to add documentation for the default datastore?

msterin

It's a big change and I have a set of nits (inside) but nothing major. I'd suggest adding to the PR description a few examples of setting the defaut_datastore and privileges, and running docker volume create and docker volume ls.

LGTM after addressing (fixing or commenting) on the nits.

msterin · 2016-12-20T18:58:36Z

esx_service/cli/vmdkops_admin.py

@@ -221,6 +221,9 @@ def commands():
                        },
                        '--description': {
                            'help': 'The new description of the tenant',
+                        },
+                        '--default-datastore': {
+                            'help': 'The name of new default_datastore',


another nit - the flag is default-datastore (with -) and the help says default_datastore (with underscore), the help should say "the name of the datastore to be used by default for volumes placement"

msterin · 2016-12-20T18:59:39Z

esx_service/cli/vmdkops_admin.py

-                                    'choices': ['create', 'delete', 'mount', 'all'],
-                                    'metavar': 'create,delete,mount'
+                                '--switch-default-datastore': {
+                                    'help': "Switch default datastore for this tenant",


why not simply 'default-datastore' ? How the command would look like ?

Command is like this:

The datastore in the first privilege added for a tenant will be set to "default_datastore" automatically even if --switch-default-datastore is not specified

Example:

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access add --name=tenant1 --datastore=datastore1 --volume-maxsize=500MB --volume-totalsize=1GB tenant access add succeeded [root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant ls Uuid Name Description Default_datastore VM_list ------------------------------------ -------- ------------------------ ----------------- ------- 93377b1e-af74-47ca-888e-75fd0a2a4aba _DEFAULT This is a default tenant 948f0edc-3a06-4d95-8b8c-8cf278154fc3 tenant1 datastore1 photon4

when you add the second privilege for a tenant, and if "--switch-default-datastore" is specified, "default_datastore" will be switch to the datastore used in the second privilege, see the following example:

/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access add --name=tenant1 --datastore=datastore2 --allow-create --switch-default-datastore --volume-maxsize=500MB --volume-tot alsize=1GB tenant access add succeeded [root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant ls Uuid Name Description Default_datastore VM_list ------------------------------------ -------- ------------------------ ----------------- ------- 93377b1e-af74-47ca-888e-75fd0a2a4aba _DEFAULT This is a default tenant 948f0edc-3a06-4d95-8b8c-8cf278154fc3 tenant1 datastore2 photon4

That is why the param is called "--switch-default-datastore".

It does not really answer the questions. Why does it need "switch" in the name ? It is confusing, inconsistent and does not really add anything. --default-datastore should be perfectly fine.

OK. I change the param to "--default-datastore".

msterin · 2016-12-20T19:12:14Z

esx_service/cli/vmdkops_admin.py


 def tenant_access_add(args):
    """ Handle tenant access command """
    error_info = auth_api._tenant_access_add(name=args.name,
                                             datastore=args.datastore,
-                                             rights=args.rights,
+                                             switch_default_datastore=args.switch_default_datastore,
+                                             allow_create=args.allow_create,


nit: the rest of params do not say "switch_allow_create", why default_datastore does ?

msterin · 2016-12-20T19:19:31Z

esx_service/utils/auth_data.py

+        if major_ver != DB_MAJOR_VER or minor_ver != DB_MINOR_VER:
+            auth_db_ver = get_version_str(major_ver, minor_ver)
+            curr_db_ver = get_version_str(DB_MAJOR_VER, DB_MINOR_VER)
+            logging.error("version %s in auth-db does not match latest DB version %s",


need some info on what to do next :-).
E.g. "DB upgrade is not supported. Please remove the DB and create new configuration using "

msterin · 2016-12-20T19:20:45Z

esx_service/utils/vmdk_utils.py

@@ -305,6 +305,16 @@ def log_volume_lsof(vol_name):
                cartel, name, ftype, fd, desc)
            logging.info("Volume open descriptor: %s", msg)

+def get_datastore_url(datastore):
+    si = vmdk_ops.get_si()
+    res = [d.info.url for d in si.content.rootFolder.childEntity[0].datastore if d.info.name == datastore]


pls wrap si.content.rootFolder.childEntity[0].datastore in some named function and reuse in line 315 as well

msterin · 2016-12-20T19:23:56Z

esx_service/vmdk_ops_test.py

@@ -885,22 +878,29 @@ def test_vmdkops_on_tenant_vm(self):
                                         vm_list=[self.vm1_name]) 
        self.assertEqual(None, error_info)

-        # run create command, which should succeed since we have DEFAULT privilege set
+        # run create command, which should fail since "default_datastore" is not set for tenant1


why create() should fail without default_datastore ? I thought we wanted to keep the current behavior (i.e. use VM datastore) ? If this is not the case then we need to make setting default_datastore mandatory on tenant creation

OK. I agree with you and make the change.
Now the logic is if "default_datastore" is not specified for a tenant, a volume will be created in the datastore where the VM lives in if the user specify the name with the short notion.

msterin · 2016-12-20T19:25:09Z

esx_service/vmdk_ops_test.py

+        # run create command again, which should succeed since "default_datastore" is set for tenant1 now
+        opts={u'fstype': u'ext4'}
+        error_info = vmdk_ops.executeRequest(vm1_uuid, self.vm1_name, self.vm1_config_path, auth.CMD_CREATE, self.tenant1_vol1_name, opts)
+        self.assertEqual(None, error_info)


we also need to check that the volume is created in correct place.
We also need to check that if we 'ls' volume it's name does not have datastore if it's on default datastore

Done. Change the listVMDK logic as we discussed. Now listVMDK will show volume with format like vol_name@datastore

msterin · 2016-12-20T19:26:21Z

esx_service/vmdk_ops_test.py

@@ -1082,6 +1084,87 @@ def test_vmdkops_on_different_tenants(self):
        self.assertEqual("tenant2_vol1", result[0]['Name'])
        self.assertEqual("tenant2_vol2", result[1]['Name'])
        self.assertEqual("tenant2_vol3", result[2]['Name'])
+
+    @unittest.skipIf(len(vmdk_utils.get_datastores()) < 2,
+                     "second datastore is not found - skipping this test")


I think that If we need 2 DS for a test, then we need to request it and make sure test bed has 2 datastores - instead of skipping the test.
@kerneltime - comments ?

I agree with you on this and will wait for Ritesh's comments on how to do it.

As we discussed yesterday, I will keep this code as it is.
I will file a new issue to track:
-- testbed in CI need to make sure it have 2 datastores in order to run this test successfully
-- contributing.md need to be updated to let user know they need to have 2 datastores to run the test

msterin · 2016-12-20T19:29:55Z

esx_service/vmdk_ops_test.py

+        # there should be two volumes shown as "tenant1_vol1"" and "tenant1_vol2"
+        self.assertEqual(len(result), 2)
+        self.assertEqual("tenant1_vol1", result[0]['Name'])
+        self.assertEqual("tenant1_vol2", result[1]['Name'])


Is it checking that @default_datastore is stripped from the name?
Then pls ad this to the comments

Since listVMDK now return volume with format like vol_name@datastore, this test has been changed accordingly.

lipingxue · 2016-12-21T21:25:02Z

Hi Mark,

This are the steps before I run "docker volume ls"

volume tenant1-vol1 is created on datastore1 from a VM which belongs to "tenant1"
I change the "default_datastore" of "tenant1" from "datastore1" to "datastore2", and then create a volume "tenant1-vol2" on the default_datastore, which is on "datastore2".
I rename "tenant1" to "new-tenant1", and change the "default_datastore" back to "datastore1", then create a volume "new-tenant1-vol3" on the default_datastore, which is "datastore1"
I run "docker volume ls", and this is the output

root@photon-JQQBWNwG6 [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vmdk                new-tenant1-vol3@datastore1
vmdk                tenant1-vol1@datastore1
vmdk                tenant1-vol2@datastore2

lipingxue · 2016-12-21T21:26:10Z

Btw: What do you mean "Good to go, not good to fi"? I assume the PR is approved by you and I can merge it after the CI failure is resolved.

pdhamdhere

LGTM

pdhamdhere · 2016-12-21T23:18:14Z

esx_service/utils/auth.py

+    """ Check whether the param "privileges" has the specific type of privilege set.
+        There are two types of privilege: 
+        - "mount_only" which means only mount and unmount are allowed
+        - "fully_access" which means create, remove, mount, unmount are


fully_access => full_access here and other places.

pdhamdhere · 2016-12-21T23:21:47Z

esx_service/cli/vmdkops_admin.py

+                                },                            
+                                '--allow-create': {
+                                    'help': 'Allow create and delete on datastore if set to True',
+                                    'action': 'store_true'


What is default? "allow-create = true"?

…eturns volume with format like "vol_name@datastore". Need to ignore the "@datastore" part when check whether the volume exists or not.

vmwclabot added the cla-not-required label Dec 16, 2016

shaominchen reviewed Dec 17, 2016

View reviewed changes

kerneltime reviewed Dec 20, 2016

View reviewed changes

msterin reviewed Dec 20, 2016

View reviewed changes

lipingxue force-pushed the issue_709.liping branch 2 times, most recently from c20a4d1 to 644c3d1 Compare December 21, 2016 00:35

lipingxue mentioned this pull request Dec 21, 2016

Testbed need to have at least two datastores to run unit test for tenancy #834

Closed

lipingxue force-pushed the issue_709.liping branch from ae7132a to 8106310 Compare December 21, 2016 18:35

pdhamdhere approved these changes Dec 21, 2016

View reviewed changes

lipingxue added 4 commits December 21, 2016 15:58

Support for default_datastore and data privileges change.

c1a08cf

Address comments from Sam and Ritesh.

82f639f

Address comments from Mark.

bd9df39

Change the logic in function "volumeVmdkExists". "docker volume ls" r…

cd6d93d

…eturns volume with format like "vol_name@datastore". Need to ignore the "@datastore" part when check whether the volume exists or not.

lipingxue force-pushed the issue_709.liping branch from 8106310 to cd6d93d Compare December 22, 2016 00:01

lipingxue added 2 commits December 21, 2016 18:27

Fix CI test failure with removing unused package.

4898ab9

Address comments from Prashant.

1efb6ca

lipingxue merged commit 7618485 into master Dec 22, 2016

msterin deleted the issue_709.liping branch January 10, 2017 07:29

shaominchen mentioned this pull request Feb 11, 2017

Tenant Admin CLI may return obsolete VM name #783

Closed

shaominchen mentioned this pull request Feb 18, 2017

Using immutable datastore identifier to define & check privileges #782

Closed

Support for default_datastore and data privileges change #828

Support for default_datastore and data privileges change #828

Conversation

lipingxue commented Dec 16, 2016

lipingxue commented Dec 16, 2016

lipingxue commented Dec 16, 2016

shaominchen left a comment • edited Loading

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

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

shaominchen Dec 17, 2016 • edited Loading

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

kerneltime commented Dec 20, 2016

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

lipingxue Dec 20, 2016 • edited Loading

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

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

lipingxue commented Dec 21, 2016

lipingxue commented Dec 21, 2016

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen left a comment •

edited

Loading

shaominchen Dec 17, 2016 •

edited

Loading

lipingxue Dec 20, 2016 •

edited

Loading