code refactoring for tenant management API #729

lipingxue · 2016-11-12T01:18:53Z

This PR includes:

Refactor code to add an abstract layer provide APIs for tenant management
add vmdk_ops unit test for multi-tenency
make "name" unique in "tenants" table

lipingxue · 2016-11-12T01:19:11Z

Please review it @msterin @shaominchen

pdhamdhere · 2016-11-14T16:47:26Z

@lipingxue CI failing. Please fix.

pdhamdhere · 2016-11-14T18:57:26Z

esx_service/cli/vmdkops_admin.py

-        return operation_fail(error_info)
-
-    name = args.name 
+    name = args.name


Currently we do not validate any parameters. We should. Separate PR is fine with me.

Issue #734 is filed to track this.

pdhamdhere

Did a quick first pass. See comments below.

pdhamdhere · 2016-11-14T19:00:20Z

esx_service/utils/auth.py

@@ -324,6 +326,25 @@ def add_volume_to_volumes_table(tenant_uuid, datastore, vol_name, vol_size_in_MB

    return None

+def remove_volume_from_volumes_table(tenant_uuid, datastore, vol_name):
+    """ Remove volume from volumes table. """


Can we add a line about return code e.g. "Returns None on success or error string" to all functions?

pdhamdhere · 2016-11-14T19:03:07Z

esx_service/utils/auth_api.py

+        vm_uuid = vmdk_utils.get_vm_uuid_by_name(vm_name)
+        if not vm_uuid:
+            error_info =  "Cannot find vm_uuid for vm {0}, tenant_create failed".format(vm_name)
+            return error_info, None


I think I had commented on this last PR. We should not terminate loop on first bad VM name. Rather iterate through entire list and return VMs that are successfully assigned and VMs that couldn't be found.

pdhamdhere · 2016-11-14T19:03:52Z

esx_service/utils/auth_api.py

+                     'global_visibility': 0,
+                      'create_volume': 0,
+                      'delete_volume': 0,
+                     'mount_volume': 0,


nit: Looks off by a space.

pdhamdhere · 2016-11-14T19:14:28Z

esx_service/utils/auth_api.py

+def default_privileges():
+     privileges = [{'datastore': 'datastore1',
+                     'global_visibility': 0,
+                      'create_volume': 0,


Are we going to group privileges as discussed in #709

I think this is a refactoring of existing code, changing/ grouping provileges should not be in this PR IMO.

Sure. Fine with separate PR.

Existing issue #709 tracked this.

pdhamdhere · 2016-11-14T19:15:55Z

esx_service/utils/auth.py

@@ -28,6 +28,7 @@
 CMD_REMOVE = 'remove'
 CMD_ATTACH = 'attach'
 CMD_DETACH = 'detach'
+CMD_LIST = 'list'


I had removed this in my last PR. Any reason we are re-introducing?
See executeRequest. Auth checks are bypassed for 'list' command.

In my unit test code, I need to test 'list' command. It seems that your change assume we only put commands which need authorize in auth.py. I will remove 'list' command from this file and update the comment.

pdhamdhere · 2016-11-14T19:16:39Z

esx_service/utils/auth_api.py

+    # mount_volume, max_volume_size, usage_quota)
+    privileges_dict = {}
+    privileges_dict[auth_data_const.COL_DATASTORE] = privileges[1]
+    privileges_dict[auth_data_const.COL_GLOBAL_VISIBILITY] = privileges[2]


Didn't we get rid of GLOBAL_VISIBILITY?

+1 on this one - there are too many open issues on this one , we need to drop it.

Done. Remove "global_visibility" from the code.

pdhamdhere · 2016-11-14T19:17:34Z

esx_service/utils/auth_data.py

@@ -359,15 +359,16 @@ def create_tables(self):
                '''
                    CREATE TABLE tenants(
                    -- uuid for the tenant, which is generated by create_tenant() API
-                    id TEXT PRIMARY KEY NOT NULL,
+                    id TEXT NOT NULL,


If name is unique then why do we need ID?

so we can rename without moving volumes, and have something invariant no matter what the ever-changing name is :-)
Still ma consider using it as a key, just need to be careful on rename

PR ##711 tracks "tenant rename".

pdhamdhere · 2016-11-14T19:22:27Z

esx_service/vmdk_ops.py

+        vol_size_in_MB = convert.convert_to_MB(auth.get_vol_size(opts))
+        auth.add_volume_to_volumes_table(tenant_uuid, datastore, vol_name, vol_size_in_MB)
+    else:
+        logging.warning(" VM %s does not belong to any tenant", vm_name)


warnings can flood logs. How about changing it to debug?

I think it's not a big deal now - we should not have this warning once "_fallback" is implemented - in which case "not belonging to any tenant" is a warning and the access for VM is denied

However given the costs, it's easier to change to debug than discuss :-)

Change the log to "DEBUG".

pdhamdhere · 2016-11-14T19:23:17Z

esx_service/vmdk_ops.py

+                return error_info
+            else:
+                if not vm_name:
+                    logging.warning("VM %s does not belong to any tenant", vm_name)


Same here. These warnings can flood logs. Change it to debug?

Please try to avoid copy-n-paste of messages. It is usually an indication that you need to either introduce a global for format string, or a simple function to return formatted string

In this PR, I just define a global for format string. But in the long run, I think we need to have a file which defines all the error code and corresponding error string for all the error message we used in our code.

msterin

Comments inline. All in all good change , the majority of comments are format / readability issues (e.g. docstrings or copy-n-paste of messages).

msterin · 2016-11-14T19:22:35Z

esx_service/cli/vmdkops_admin.py

-                                             privileges = privileges)
+
+    error_info, tenant = auth_api._tenant_create(
+                                                 name = name, 


Applies to multiple places:

PEP8 https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements:

Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value. Yes: def complex(real, imag=0.0): return magic(r=real, i=imag) No: def complex(real, imag = 0.0): return magic(r = real, i = imag)

(note that spaces around assignment in other cases are still needed)

msterin · 2016-11-14T19:27:07Z

esx_service/cli/vmdkops_admin.py

+    datastore = args.datastore
+    rights = args.rights
+    volume_maxsize = args.volume_maxsize
+    volume_totalsize = args.volume_totalsize


are those assignment really needed ? What prevents us from simply callng

error_info = auth_api._tenant_access_add( name=args.name, datastore=args.datastore, privileges=generate_privileges(args), ...

msterin · 2016-11-14T19:28:01Z

esx_service/cli/vmdkops_admin.py

-    privileges_dict = modify_privileges(privileges_dict, args)
-
-    error_info = tenant.set_datastore_access_privileges(_auth_mgr.conn, [privileges_dict])
+    error_info = auth_api._tenant_access_set(name, datastore, add_rights, rm_rights, volume_maxsize, volume_totalsize)


same question - assignment above do not not seem to be needed, you can use param=args.field in the call to _tenant_access_set

the comment applies to the rest of the wrappers too

msterin · 2016-11-14T19:31:04Z

esx_service/utils/auth_api.py

+import auth_data
+import vmdk_utils
+
+def get_auth_mgr():


dosctrings are missing, please add - that applies to all new methods/functions

msterin · 2016-11-14T19:31:54Z

esx_service/utils/auth_api.py

+def get_auth_mgr():
+    try:
+        auth_mgr = auth.get_auth_mgr()
+    except auth_data.DbConnectionError, e:


If it's new code, please use 'except as . If this is just code move, its OK to leave it as is

msterin · 2016-11-14T20:00:26Z

esx_service/vmdk_ops_test.py

+        if (not self.datastore_name):
+            datastores = vmdk_utils.get_datastores()
+            datastore = datastores[0]
+            if (not datastore):


this looks suspicious. Is there a case where non-empty array with None as the first element is returned by get_datastores() ?

msterin · 2016-11-14T20:07:34Z

esx_service/vmdk_ops_test.py

+
+    def tearDown(self):
+        """ Cleanup after each test """
+        logging.debug("VMDKTenantTest  tearDown path")


logging.info ? we are in tests, it's important to have traces

msterin · 2016-11-14T20:08:09Z

esx_service/vmdk_ops_test.py

+                vmdk_ops.removeVMDK(vmdk_path)
+
+        # cleanup existing tenant
+        error_info = auth_api._tenant_rm(


nit: PEP8 (just a reminder). no spaces around = in param assignment

msterin · 2016-11-14T20:12:24Z

esx_service/vmdk_ops_test.py

+        self.assertEqual("tenant2_vol2", result[1]['Name'])
+        self.assertEqual("tenant2_vol3", result[2]['Name'])
+
+


A general comment on tests: it is better to have them data drivel . I.e. instead of using tenantx_volx ad opts and other stuff in the code, ALL that should be in data structures and the test should just taverse the structures. See example (testInfo traversing) in vmdk_ops_test.py.

It's OK to leave it as is for this PR but please do open an issue for test rearrangement and making them data driven - that allows to understand tests better and to add to the test set easily

Issue #740 is filed to track this

msterin · 2016-11-14T20:13:52Z

esx_service/vsan_policy_test.py

@@ -62,6 +62,7 @@ def test_create_delete(self):

    def test_delete_nonexistent_policy_fails(self):
        self.assertNotEqual(None, vsan_policy.delete(self.name))
+        print ("The test itself is expected to fail, and please ignore the errors printed above.")


does not it have to be stderr ? Or log ? (just asking since I do not see this message in the log

put it in logging.info.

msterin

Looks good, almost there. A couple of files are missing standard headers ,and I am not sure why ' name' is a part of keys in Tenant. When this is resolved, ready to merge

msterin · 2016-11-18T01:53:58Z

esx_service/utils/auth_api.py

@@ -0,0 +1,336 @@
+import auth


Please add standard file header

I thought it was auto-generated? CC/ @kerneltime

It was auto-generated when we did a pass of adding headers :-). Now it is copy-n-paste before push.

msterin · 2016-11-18T01:56:01Z

esx_service/utils/auth_data.py

                    -- brief description of the tenant, which is specified by user when creating the tenant
                    -- this field can be changed laster by using set_description API
                    description TEXT,
                    -- not use currently
-                    default_datastore TEXT
+                    default_datastore TEXT,
+                    PRIMARY KEY(id, name)


why do we need a key with both id, name ? does it mean the key changes when name changes ?
I thought ID would be the key and name would be just a text field (unique is good, but just text, not key) ?

Done. Make "name" to be UNIQUE, but not the key.

msterin · 2016-11-18T01:56:15Z

esx_service/utils/auth_data.py

@@ -446,6 +442,12 @@ def create_tenant(self, name, description, default_datastore, default_privileges

        """
        logging.debug ("create_tenant name=%s", name)
+        error_info, exist_tenant = self.get_tenant(name)
+        if exist_tenant:
+            error_info = error_code.TENANT_ALREADY_EXIST.format(name)


msterin · 2016-11-18T01:56:42Z

esx_service/utils/error_code.py

@@ -0,0 +1,7 @@
+# Tenant related error message


Please add standard file header

msterin · 2016-11-18T01:57:26Z

esx_service/utils/vmdk_utils.py

@@ -214,4 +214,38 @@ def get_vm_uuid_by_name(vm_name):
        return vm[0].config.uuid
    except:
        return None
+def get_datastore_path(ds_name):
+    for (datastore, url_name, path) in get_datastores():


dosctring (or comment) is missing. Also, an empty line before def is needed

pdhamdhere

Took quick look. It's huge change so couldn't go through logic again. Overall LGTM. Please address Mark's review comments and I have some minor comments below. Once those are addressed, it's good to go from my side.

pdhamdhere · 2016-11-18T16:54:54Z

esx_service/utils/auth_api.py

+                                             privileges=privileges)
+    if error_info:
+        return error_info, None
+


Print/ Log not_found_vms informing user?

pdhamdhere · 2016-11-18T16:58:09Z

esx_service/utils/auth_data.py

+                                          vol_name = vmdk_utils.strip_vmdk_extension(vmdk['filename']),
+                                          vm_name = None,
+                                          tenant_uuid = tenant_id,
+                                          datastore = vmdk['datastore'])


Spaces still exist. Forgot to push?

pdhamdhere · 2016-11-18T16:59:18Z

esx_service/utils/error_code.py

+VM_NOT_BELONG_TO_TENANT = "VM {0} does not belong to any tenant"
+TENANT_NOT_EXIST = "Tenant {0} does not exist"
+TENANT_ALREADY_EXIST = "Tenant {0} already exists"
+


pdhamdhere · 2016-11-18T16:59:57Z

esx_service/vmdk_ops.py

-        return err(msg)
-
+        error_info = err(msg)
+        remove_err = removeVMDK(vmdk_path = vmdk_path, 


remove space?

pdhamdhere · 2016-11-18T17:00:26Z

esx_service/vmdk_ops.py

-                logging.warning(" VM %s does not belong to any tenant", vm_name)
-
+    elif cmd == "create":          
+        response = createVMDK(vmdk_path = vmdk_path, 


remove spaces?

pdhamdhere · 2016-11-18T17:00:32Z

esx_service/vmdk_ops.py

    elif cmd == "remove":
-        response = removeVMDK(vmdk_path)
+        response = removeVMDK(vmdk_path = vmdk_path, 


remove spaces?

msterin · 2016-11-18T20:22:11Z

@lipingxue - it looks like there are some conflicts to be resolved. and the latest changes (marked as "done" are still not in. Looking forward...

lipingxue · 2016-11-18T22:34:58Z

Yes. After rebase with the master, there are some conflict with latest change in master(Bruno's change). I am addressing those conflict and rerun the test. Will do the push when it is done.

lipingxue · 2016-11-18T23:09:48Z

@msterin I have resolved conflict and push the change. Please take another look.

msterin · 2016-11-18T23:12:23Z

esx_service/vmdk_ops_test.py

@@ -438,52 +438,6 @@ def remove_vm(si, vm_name):
        task = vm.Destroy_Task()
        vmdk_ops.wait_for_tasks(si, [task])

-class ValidationTestCase(unittest.TestCase):


why this test is removed?

lipingxue · 2016-11-21T18:07:34Z

@msterin
As we discussed last Friday, CI test failed due to DB schema existed in the testbed.
In order to make the test always start from a clean state, we decided to add code to remove auth_DB at each test. I added code for that, but it does not work.
The CI test failed with "disk I/O error" when accessing auth DB

I checked the log and also looked at our code, and I figured out this is reason why I see the above error. We call function "get_auth_mgr()" to get a connection to the DB. This function will call "thread_local._auth_mgr.connect()" ONLY if it does not have a cached value. In function"AuthorizationDataManager.connect()", we have the logic to decide whether we need to create table or not (check whether DB file exists or not).

Assume we start from the state that we have pre-exist DB in the testbed.

first time call "get_auth_mgr" to get a DB connection. Since this is the first time, it will further call AuthorizationDataManager.connect() to check whether create table is needed or not. Here
we don't need create the table since we already has the DB file in the test bed. The function
return a connection for the DB.
remove the auth DB file in the test code
second time call "get_auth_mgr" to get a DB connection. This time, it will not call AuthorizationDataManager.connect() since it already has the cached value. Therefore, it returned
a connection to staled DB file.

I think we only need to remove the old DB one time. This logic also apply to the possible DB migration.
Now I change the logic as follows:

At the beginning of AuthorizationDataManager.connect(), check if we have DB file, it we have, and we haven't removed it (upgraded), do remove the DB.
After that, existing code will check whether we have the DB file (we don't have now since we just remove it), and if we don't have, it will create the DB file and return the connection to the DB.

Please review this newly added code.

msterin

file removal should be a part of the esx_service/Makefile not .py code

msterin · 2016-11-21T18:44:28Z

esx_service/utils/auth_data.py

+        logging.debug("auth DB %s exist, try to remove the file", AUTH_DB_PATH)
+        os.remove(AUTH_DB_PATH)
+        # insert some delay 
+        time.sleep(60)


why the delay ?

msterin · 2016-11-21T18:49:47Z

esx_service/utils/auth_data.py

+
+        if os.path.isfile(self.db_path):
+            # already exist
+            if not has_migrate_auth_db and self.need_migrate_auth_db():


self.need_migrate_auth_db() need to check for 'has already migrated' (what you cann has_migrate_auth_db ) for better encapsulation

msterin · 2016-11-21T18:50:14Z

esx_service/utils/auth_data.py

+            # already exist
+            if not has_migrate_auth_db and self.need_migrate_auth_db():
+                self.migrate_auth_db()
+                has_migrate_auth_db = True


well, since it's global it's "test and set" should be atomic , so I suggest a lock here

msterin · 2016-11-21T18:51:35Z

misc/scripts/deploy-tools.sh

@@ -117,9 +117,11 @@ function deployesx {
    do
        TARGET=root@$ip
        log "Deploying to ESX $TARGET"
+        $SSH $TARGET  rm -f /etc/vmware/vmdkops/log_config.json 


This one should be a different commit - it's a useful fix

…get a connection to auth DB.

…ent. Those APIs can be called by both AdminCLI and VMODL code.

…sfully. 2. make change to make "tenant name" unique.

1) attachVMDK, detachVMDK, listVMDK 2) test to make sure isolation between tenants

… in vmdkops test for multi-tenancy. Fix small bugs in auth_api.py.

…ipingXue)

…e testbed." This reverts commit 3f8b606.

…deployesx".

Fix a bug in function "get_vm_config_path".

lipingxue · 2016-11-21T22:35:52Z

@msterin I have addressed your comments, please review.

msterin

Contingent on passing CI

* Refactor code to add an abstract layer to provide APIs for tenant management. Those APIs can be called by both AdminCLI and VMODL code. * Add unit test for multi-tenancy in "vmdk_ops_test.py" * remove volume from "volumes" table when removing the volume successfully. * make change to make "tenant name" unique in DB schema. * Force remove file "/etc/vmware/vmdkops/log_config.json" in function "deployesx". * Add code to force removing auth DB file before running unit test.

lipingxue · 2017-02-01T18:21:34Z

Fixed #710

vmwclabot added the cla-not-required label Nov 12, 2016

pdhamdhere reviewed Nov 14, 2016

View reviewed changes

msterin suggested changes Nov 14, 2016

View reviewed changes

lipingxue mentioned this pull request Nov 15, 2016

Tenancy: need add code to validate input parameters for all AdminCLI for tenant management #734

Closed

msterin mentioned this pull request Nov 15, 2016

Add some parameter checks for multi-tenancy CLI commands #735

Closed

lipingxue mentioned this pull request Nov 16, 2016

unit test for multi-tenancy need to be data-driven #740

Closed

lipingxue force-pushed the auth_refactor.liping branch from 68c5e04 to fd1fa8b Compare November 18, 2016 00:58

msterin reviewed Nov 18, 2016

View reviewed changes

pdhamdhere reviewed Nov 18, 2016

View reviewed changes

lipingxue force-pushed the auth_refactor.liping branch from fd1fa8b to f77514a Compare November 18, 2016 23:08

msterin approved these changes Nov 18, 2016

View reviewed changes

lipingxue force-pushed the auth_refactor.liping branch 3 times, most recently from 910a9db to 3f8b606 Compare November 21, 2016 17:39

msterin suggested changes Nov 21, 2016

View reviewed changes

lipingxue force-pushed the auth_refactor.liping branch from 3f8b606 to 563c044 Compare November 21, 2016 20:03

lipingxue added 7 commits November 21, 2016 14:23

Remove function connect_auth_db, using new interface get_auth_mgr to …

15fd87b

…get a connection to auth DB.

More changes to remove usage of function "connect_auth_db".

107e596

Refactor code to add an abstace layer provide APIs for tenant managem…

8ee5e28

…ent. Those APIs can be called by both AdminCLI and VMODL code.

Add vmdkops_test to use "executeRequest" provided by vmdk_ops.

cd6b4dc

Add vmdkop_test for tenant.

08b53aa

1. remove volume from "volumes" table when removing the volume succes…

d25dd73

…sfully. 2. make change to make "tenant name" unique.

Add more vmdkops test for multi-tenency.

50a7f2b

lipingxue and others added 13 commits November 21, 2016 14:23

Add vmdkops test for tenant to include:

a61d4ac

1) attachVMDK, detachVMDK, listVMDK 2) test to make sure isolation between tenants

Fixes error in vmdk_ops unit test for multi-tenency.

e09492e

Remove unsed comment.

42659fb

Add cleanup logic to cleanup the possible left over of previous tests…

531f09c

… in vmdkops test for multi-tenancy. Fix small bugs in auth_api.py.

Address comments from Mark and Prashant.

0056570

Fix lint errors.

a1079ec

Address more comments from Mark and Prashant.

a7dbc35

Rebase with master, remove lint error caused by rebase.

c8b87ba

Replaced get_datastore_path_by_name with os.path.join (on behalf on L…

3f6b0c3

…ipingXue)

Fix the CI test failure caused by old DB schema exsited in the testbed.

3ea987e

Revert "Fix the CI test failure caused by old DB schema exsited in th…

55ea631

…e testbed." This reverts commit 3f8b606.

Force remove file "/etc/vmware/vmdkops/log_config.json" in function "…

1f26b27

…deployesx".

Add code to force removing auth DB file before running unit test.

84f013f

Fix a bug in function "get_vm_config_path".

lipingxue force-pushed the auth_refactor.liping branch from 563c044 to 84f013f Compare November 21, 2016 22:23

msterin approved these changes Nov 21, 2016

View reviewed changes

lipingxue merged commit 8717d75 into master Nov 22, 2016

This was referenced Nov 22, 2016

The "--volume-maxcount" option is confusing #676

Closed

Need unit test coverage for admin CLI tenant commands #668

Closed

Cleanup work for "AdminCLI change to support multitenancy" #653

Closed

msterin deleted the auth_refactor.liping branch November 23, 2016 22:54

kerneltime mentioned this pull request Nov 30, 2016

Address regression in ESX 6.5 support. #786

Closed

		self.assertEqual("tenant2_vol2", result[1]['Name'])
		self.assertEqual("tenant2_vol3", result[2]['Name'])

code refactoring for tenant management API #729

code refactoring for tenant management API #729

Conversation

lipingxue commented Nov 12, 2016

lipingxue commented Nov 12, 2016

pdhamdhere commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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