tenant modify #781

lipingxue · 2016-11-28T22:26:10Z

This PR includes:

AdminCLI to support "tenant modify"
API to support "tenant modify"
corresponding vmdkops changes. Before this change, a directory /vmfs/volumes/datastore_name/dockvol/tenant_name will be created to store all volumes created by a tenant. In order to support tenant rename, we decide to create the directory with "/vmfs/volumes/datastore_name/dockvol/tenant_uuid, and then create a symlink like "/vmfs/volumes/datastore_name/dockvol/tenant_name" which points to that directory.
Then during tenant rename, we don't delete the directory itself, we just unlink the old symlink and recreate the new symlink.
extend unit test in vmdk_ops_test.py to test vmdk ops after tenant rename.
Add code to enable logging in auth*.py.

shaominchen · 2016-11-28T23:56:51Z

esx_service/cli/vmdkops_admin.py

+                            'help': 'The new name of the tenant',
+                            'required': False
+                        },
+                        '--description': {


The existing "tenant create" command does not support "description" argument. If it's not trivial to fix it in the current change, can you please file a tracking issue?

shaominchen · 2016-11-29T00:32:24Z

esx_service/utils/auth_api.py

+    if error_info:
+        return error_info
+    if new_name:    
+        error_info = tenant.set_name(auth_mgr.conn, name, new_name)


It looks to me unnatural to pass auth_mrg.conn parameter here. Why can't we get the DB connection in the backend (inside the tenant class or its dependents) to free up the API user?

API user does not need to worry about the connection. API user only need to call _tenant_modify API. The DB connection is generated inside AuthorizationDataManager class. We can make the change to get the DB connection in the backend, but that will need a lot of code refactoring. I will not do it for now.

It is true the _tenant_update API user doesn't need to worry about the connection. However, in my opinion the user of DockerVolumeTenant.set_name() API should not worry about the connection either. Basically we should isolate the DB operation to any upper-level business logic.

Anyway, I agree this will need a lot of code refactoring. We can address this later.

shaominchen · 2016-11-29T00:50:11Z

esx_service/utils/auth_api.py

@@ -196,6 +211,29 @@ def _tenant_create(name, description, default_datastore, default_privileges, vm_

    return None, tenant

+def _tenant_modify(name, new_name=None, description=None):


It looks like all API methods are following a different naming convention:

Start with understore "_"

A noun followed by a verb

Other non-API methods in this module are not following this convention. Since Python doesn't provide access modifiers to differentiate public and private methods, maybe we should consider refactoring the module (split it into two?) so that all methods in one module follow the same naming convention?

I will file a issue to track this code refactoring.

shaominchen · 2016-11-29T00:51:11Z

esx_service/utils/auth_api.py

+        if error_info:
+            return error_info
+    if description:
+        error_info = tenant.set_description(auth_mgr.conn, description)


Same as above.

shaominchen · 2016-11-29T01:11:14Z

esx_service/utils/auth_data.py

+        # unlink the old symbol link /vmfs/volumes/datastore_name/tenant_name
+        # create the new symbol link /vmfs/volumes/datastore_name/new_tenant_name 
+        # which point to path /vmfs/volumes/datastore_name/tenant_uuid
+        lock = threadutils.get_lock()


There are some duplicate logic in this paragraph (line 135-156) and the other one (line 695-709). Can we refactor the code to extract them into separate methods and eliminate the duplication?

Can you please address this comment?

lipingxue · 2016-12-06T17:29:30Z

@msterin Please review this change.

pdhamdhere

Some clarifying questions below.

pdhamdhere · 2016-12-06T19:49:04Z

esx_service/cli/vmdkops_admin.py

@@ -204,6 +205,24 @@ def commands():
                        }
                    }
                },
+                'modify': {


Change modify => update?

pdhamdhere · 2016-12-06T19:53:07Z

esx_service/utils/auth_data.py

+            logging.debug("set_name: try to update the symlink to path %s", tenant_path)
+
+            if os.path.isdir(tenant_path):
+                lock.acquire() 


What exactly this lock is protecting?

Is there anything that synchronizes docker volume create and admin_cli tenant update happening at the same time?

The lock is to make sure if two users (from CLI and UI) try to modify the tenant, for example, CLI start first, it wants to rename "tenant" to "tenant1" and UI wants to rename "tenant" to "tenant2", then the final result should be "tenant" is renamed to "tenant2". Without this lock, it is quite possible that the result is "tenant" is renamed to "tenant1".
I think we can use os.rename here.

Made the change to use os.rename.

pdhamdhere · 2016-12-06T19:55:50Z

esx_service/utils/auth_data.py

+                    os.remove(exist_symlink_path)
+                    logging.debug("Remove the symbol link %s", exist_symlink_path)
+
+                new_symlink_path = os.path.join(dock_vol_path, new_name)


Is "renaming" (mv oldSymLink newSymLink) not an option?

I think "renaming" should work here.

lipingxue · 2016-12-07T00:42:10Z

I have addressed comments from Sam and Prashant. Please review it @shaominchen @pdhamdhere

shaominchen

LGTM. As I mentioned in one comment, the DB connections are exposed in many classes. We should refactor this code to have a persistence layer (or DAO layer) to handle DB operations. Other core business logic should not handle DB connections. This way different layers can evolve separately.

msterin · 2016-12-07T18:54:32Z

+1 on @shaominchen comment wrt DAO / DB connection consolidation. Please file an issue. The rest is LGTM too.

msterin

a few format / message changes are needed - pls take a look before the merge. other than that LGTM

msterin · 2016-12-07T18:56:01Z

esx_service/utils/auth_data.py

+                exist_symlink_path = os.path.join(dock_vol_path, name)
+                new_symlink_path = os.path.join(dock_vol_path, new_name)
+                if os.path.isdir(exist_symlink_path):
+                    logging.info("Rename the symlink %s to %s", exist_symlink_path, new_symlink_path)


nit: Renaming

msterin · 2016-12-07T19:03:58Z

esx_service/utils/auth_data.py

+            for (datastore, url_name, path) in vmdk_utils.get_datastores():
+                dock_vol_path = os.path.join("/vmfs/volumes", datastore, vmdk_ops.DOCK_VOLS_DIR)
+                tenant_path = os.path.join(dock_vol_path, tenant_id)
+                logging.debug("remove volumes for tenant: try to remove the symlink to path %s", tenant_path)


message is a bit unclear. Maybe "Tenant removal: removing symlink " ?
And the rest of the messages should be similar. Also please avoid copy-n-paste for logging format,
instead use something like

VOL_RM_LOG_PREFIX = ""Tenant <name> removal: " logging.debug(VOL_RM_LOG_PREFIX + "Removing symlink to %s", tenant_id, tenant_path)

(or any other reusabe code)

msterin · 2016-12-07T19:04:31Z

esx_service/utils/auth_data.py

+                    exist_symlink_path = os.path.join(dock_vol_path, tenant_name)
+                    if os.path.isdir(exist_symlink_path):
+                        os.remove(exist_symlink_path)
+                        logging.debug("remove volumes for tenant: Remove the symbol link %s", exist_symlink_path)


"removing symlink %s"

lipingxue · 2016-12-07T19:13:00Z

Thanks @shaominchen @msterin

Issue #803 is filed to address your DAO layer related comments.

Turn on log configure in vmdkops_admin.py, auth.py, auth_data.py.

Fix one small bug in function "_tenant_access_ls".

lipingxue · 2016-12-08T17:32:49Z

Fixes #715

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

shaominchen reviewed Nov 29, 2016

View reviewed changes

pdhamdhere reviewed Dec 6, 2016

View reviewed changes

lipingxue force-pushed the issue_715.liping branch from 372ce97 to a5a9d0a Compare December 7, 2016 00:11

shaominchen approved these changes Dec 7, 2016

View reviewed changes

lipingxue mentioned this pull request Dec 7, 2016

Tenancy: code refactor is needed to have a persistence layer (or DAO layer) to handle DB operations. #803

Closed

msterin approved these changes Dec 7, 2016

View reviewed changes

lipingxue added 6 commits December 7, 2016 14:58

First checkpoint.

589735a

Add code to support tenant_modify.

7b35dc1

Turn on log configure in vmdkops_admin.py, auth.py, auth_data.py.

Add unit test to test vmdkops after tenant rename.

2e6fead

Fix a typo in "tenant modify" command description and some comments.

1a6f1b3

Address comments from Sam and Prashant.

bb97934

Address more comments from Mark.

6a6262d

Fix one small bug in function "_tenant_access_ls".

lipingxue force-pushed the issue_715.liping branch from a5a9d0a to 6a6262d Compare December 7, 2016 22:59

lipingxue merged commit ff71815 into master Dec 8, 2016

lipingxue deleted the issue_715.liping branch December 8, 2016 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tenant modify #781

tenant modify #781

lipingxue commented Nov 28, 2016

shaominchen Nov 28, 2016

shaominchen Nov 29, 2016

lipingxue Dec 6, 2016

shaominchen Dec 7, 2016

shaominchen Nov 29, 2016

lipingxue Dec 7, 2016

shaominchen Nov 29, 2016

lipingxue Dec 6, 2016

shaominchen Nov 29, 2016

shaominchen Dec 7, 2016

lipingxue commented Dec 6, 2016

pdhamdhere left a comment

pdhamdhere Dec 6, 2016

lipingxue Dec 7, 2016

pdhamdhere Dec 6, 2016

lipingxue Dec 6, 2016

lipingxue Dec 7, 2016

pdhamdhere Dec 6, 2016

lipingxue Dec 6, 2016

lipingxue Dec 7, 2016

lipingxue commented Dec 7, 2016

shaominchen left a comment

msterin commented Dec 7, 2016

msterin left a comment

msterin Dec 7, 2016

lipingxue Dec 7, 2016

msterin Dec 7, 2016

lipingxue Dec 7, 2016

msterin Dec 7, 2016

lipingxue Dec 7, 2016

lipingxue commented Dec 7, 2016

lipingxue commented Dec 8, 2016

		@@ -196,6 +211,29 @@ def _tenant_create(name, description, default_datastore, default_privileges, vm_

		return None, tenant

		def _tenant_modify(name, new_name=None, description=None):

tenant modify #781

tenant modify #781

Conversation

lipingxue commented Nov 28, 2016

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 6, 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

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 7, 2016

shaominchen left a comment

Choose a reason for hiding this comment

msterin commented Dec 7, 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

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 7, 2016

lipingxue commented Dec 8, 2016