Tenancy: Implement FALLBACK tenant and FALLBACK privilege #788

lipingxue · 2016-12-02T00:17:41Z

This change solved two problems in this issue #621.

support __FALLBACK tenant
support __FALLBACK privilege
The remain problem "support default_datastore" will be addressed in my next PR. In my next PR, I plan to address "support default_datastore" and privilege changes (Tenancy: Creating volume fails in absence of "mount" privilage #709)

This change also include necessary unit test to support __FALLBACK tenant and __FALLBACK privilege.

lipingxue · 2016-12-02T00:19:04Z

shaominchen · 2016-12-02T23:34:44Z

esx_service/utils/auth.py

+    if result:
+        # found __FALLBACK tenant
+        tenant_uuid = result[0]
+        logging.debug("Found __FALLBACK tenant, tenant_uuid %s, tenant_name %s", tenant_uuid, FALLBACK_TENANT)


Nit: Since the last parameter tenant_name is "__FALLBACK", I think we can log the message "Found fallback tenant..." instead of "Found __FALLBACK tenant...".

Seems like it's still the same?

shaominchen · 2016-12-02T23:35:56Z

esx_service/utils/auth.py

+        return None, None, None
+
+
+def get_fallback_privileges():


There will be one and only one fallback privilege, correct? Shouldn't we use singular form "privilege" instead of "privileges" everywhere?

Yes. This should just be one entry for fallback privilege. But in this entry, they have different privileges (create, remove, and mount). That is why I use "privileges".

I think we should differentiate the term "privilege" and "right". IIUC, in our service the term "privilege" indicates some rights (create, remove, mount, etc.) on one datastore. One tenant can have multiple "privileges" on different datastores, but there will be one and only one privilege (which includes multiple "rights") on one single datastore.

shaominchen · 2016-12-02T23:39:16Z

esx_service/utils/auth.py

+            )
+        privileges = cur.fetchone()
+    except sqlite3.Error as e:
+        error_info = "Error {0} when querying privileges table for __FALLBACK privilege".format(e)


Nit: Same as above - I suggest to use "fallback" instead of "__FALLBACK" in the log messages. We should always use above constants (FALLBACK_TENANT or FALLBACK_DS) to reference "__FALLBACK" strings.

Seems like it's still the same?

shaominchen · 2016-12-02T23:44:50Z

esx_service/utils/auth_data.py

@@ -308,6 +309,35 @@ def get_auth_db_path(self):

        """
        return AUTH_DB_PATH
+
+    def create_fallback_tenant_and_privileges(self):


I suggest to split this method into two: create_fallback_tenant() and create_fallback_privilege(). One method does one thing only.

+1 on splitting into 2 separate methods.

shaominchen · 2016-12-02T23:53:48Z

esx_service/utils/auth.py

+        error_info, tenant_uuid, tenant_name = get_fallback_tenant()
+        if error_info:
+            return error_info, None, None
+        if not tenant_uuid:


Nit: I'd prefer "if" than "if not" as the former is slightly easier to understand:

if tenant_uuid:
....
else:
....

I will keep it as is.

Can you please explain a little bit?

I think it is different coding style. No major advantage to make the change.

shaominchen · 2016-12-02T23:58:59Z

esx_service/utils/error_code.py

@@ -18,6 +18,8 @@
 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"
+TENANT_CREATE_FAILED = "Tenant {0} create failed with err:{1}"


Nit: add a space after "err:"

shaominchen · 2016-12-03T00:01:26Z

esx_service/utils/error_code.py

@@ -18,6 +18,8 @@
 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"
+TENANT_CREATE_FAILED = "Tenant {0} create failed with err:{1}"
+TENANT_SET_ACCESS_PRIVILEGES_FAILED = "Tenant {0} set access privileges on datastore {1} failed with {2}"


Nit: Let's keep the error messages consistent: either "failed with {}" or "failed with error: {}"

shaominchen · 2016-12-03T00:05:12Z

esx_service/vmdk_ops.py

@@ -252,6 +252,7 @@ def cloneVMDK(vm_name, vmdk_path, opts={}, vm_uuid=None, vm_datastore=None):
        return err("Failed to initialize source volume path {0}: {1}".format(src_path, errMsg))

    src_vmdk_path = vmdk_utils.get_vmdk_path(src_path, src_volume)
+    logging.debug("cloneVMDK: src_path=%s, src_volume=%s, src_vmdk_path=%s", src_path, src_volume, src_vmdk_path)


The change in this file doesn't seem to be related to __FALLBACK feature. If you want to include these changes, please add some description in the review.

Yes. This is not related to FALLBACK feature. The reason I add this debug log is I found it is very difficult when I debug FALLBACK feature, the unit test failed in cloneVMDK related test. I think the log is helpful for others to debug if they hit errors in this part.

shaominchen · 2016-12-03T00:20:35Z

esx_service/utils/auth_data.py

@@ -336,6 +366,7 @@ def connect(self):

        if need_create_table:
            self.create_tables()
+            self.create_fallback_tenant_and_privileges()


This doesn't seem to be the right place to create fallbacks. I think we should add a flag to check if need_crreate_fallback. This way, we can handle the "upgrade" more smoothly. Also in the unit test we shouldn't have the logic to create fallbacks.

fallback has nothing to do with upgrade.
fallback tenant and fallback privilege will be created automatically when table was first created.
upgrade happens when we detect that there exist a DB file, but we found the version is not the latest, then we should have some code to upgrade the DB table to the latest. Those part of code is not here yet.

Since user can remove the fallback tenant and fallback privilege, in unit test, we have some test to test the behavior when fallback tenant and fallback privileges are removed. And after our test, we want to return back to the status that fallback tenant and fallback privileges are still in the DB table.

shaominchen · 2016-12-03T00:22:55Z

esx_service/vmdk_ops_test.py

@@ -685,7 +689,38 @@ class VmdkTenantTestCase(unittest.TestCase):
    datastore_name = None
    datastore_path = None

-
+    def create_fallback_tenant_and_privileges(self):


As mentioned in above comment, we should check whether fallbacks need to be created even if table already exists. This way we don't need to do this in unit tests.

Same as above.

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

@msterin Please review this change.

pdhamdhere

Overall LGTM.

_FALLBACK tenant will show up in admin tenant ls command rt? Also I assume Admin can override default privileges using Admin CLI. Please confirm

pdhamdhere · 2016-12-06T21:16:50Z

esx_service/utils/auth.py

@@ -30,6 +32,8 @@
 CMD_DETACH = 'detach'

 SIZE = 'size'
+FALLBACK_TENANT = '__FALLBACK'


FALLBACK => _DEFAULT or _SYSTEM?

FWIW I do not see much difference. 'fallback' is semantically more accurate. But "__DEFAULT" sounds good too.

I will keep the name as __FALLBACK.

pdhamdhere · 2016-12-06T21:35:52Z

esx_service/utils/auth_data.py

+        # create a __FALLBACK tenant
+        error_info, tenant = self.create_tenant(
+                                           name=auth.FALLBACK_TENANT, 
+                                           description="This is a fallback tenant", 


"This is a system _default tenant"?

I will keep it as FALLBACK.

pdhamdhere · 2016-12-06T21:42:51Z

esx_service/utils/auth_data.py

@@ -308,6 +309,35 @@ def get_auth_db_path(self):

        """
        return AUTH_DB_PATH
+
+    def create_fallback_tenant_and_privileges(self):


+1 on splitting into 2 separate methods.

lipingxue · 2016-12-06T22:05:47Z

@pdhamdhere
__FALLBACK tenant and __FALLBACK privilege are created by system. Admin can remove the __FALLBACK tenant. Admin can also remove or modify __FALLBACK privilege.

msterin

Good change ! Some comments inline. Please add the test description to the PR description. I will be looking forward for you addressing Sam's and Prashant;s comments (mine are mainly nits)

msterin · 2016-12-06T22:38:05Z

esx_service/utils/auth.py

@@ -30,6 +32,8 @@
 CMD_DETACH = 'detach'

 SIZE = 'size'
+FALLBACK_TENANT = '__FALLBACK'


FWIW I do not see much difference. 'fallback' is semantically more accurate. But "__DEFAULT" sounds good too.

msterin · 2016-12-06T22:38:40Z

esx_service/utils/auth.py

+def get_fallback_tenant():
+    """ 
+        Get __FALLBACK tenant by querying the auth DB.
+        VMs which does not belong to any tenant explicitly will 


nit: "VMs which do not belong" or "VM which does not belong"

msterin · 2016-12-06T23:25:47Z

esx_service/utils/auth.py

+        be assigned to __FALLBACK tenant if __FALLBACK tenant exists
+        Return value:
+        -- error_info: return None on success or error info on failure
+        -- tenant_uuid: return __FALLBACK tenant uuid on success,  


Nit: please use FALLBACK_TENANT in comments, instead of the variable value :-)

msterin · 2016-12-06T23:27:53Z

esx_service/utils/auth.py

+    tenant_uuid = None
+    _auth_mgr = get_auth_mgr()
+    try:
+        cur = _auth_mgr.conn.execute(


so this code is going to be executed on each VMDK request ? Any way to cache it for say 1 sec ? (not a issue, just musing)

I am not going to address it in this PR, but will file an issue to track this enhancement.

msterin · 2016-12-07T00:03:41Z

esx_service/utils/auth.py

-    return None, tenant_uuid, tenant_name
+        return None, tenant_uuid, tenant_name
+    else:
+        error_info, tenant_uuid, tenant_name = get_fallback_tenant()


get_fallback_tenant() seems to be doing pretty much the dame as the code above (line 136-151). I think it would be better wrap it in a function get_tenant_from_db(where_clause, key) , and use it with something like.

get_tenant_from_db("id = ?", (tenant_uuid, ))

Basically I think copy-n-paste is evil :-)

These two places are different. For normal tenant, from vm_uuid, we query vms table to find the tenant_uuid of the tenant which own this VM. Then we query the tenant table by id to get tenant_name.

For FALLBACK tenant, we know the tenant name is "__FALLBACK". We then query the tenant table by name to get tenant_uuid.

msterin · 2016-12-07T00:05:26Z

esx_service/utils/auth.py

+            (FALLBACK_DS,)
+            )
+        privileges = cur.fetchone()
+    except sqlite3.Error as e:


It feels the exceptions are handled too often. I think yu should handle them much higher up in the stack. Let's discuss offline

We have discussed offline.

msterin · 2016-12-07T00:07:53Z

esx_service/utils/auth_data_test.py

+                                 tenants_list[tenant2_idx].default_privileges[0][auth_data_const.COL_DELETE_VOLUME],
+                                 tenants_list[tenant2_idx].default_privileges[0][auth_data_const.COL_MOUNT_VOLUME],
+                                 tenants_list[tenant2_idx].default_privileges[0][auth_data_const.COL_MAX_VOLUME_SIZE],
+                                 tenants_list[tenant2_idx].default_privileges[0][auth_data_const.COL_USAGE_QUOTA]


Nice seeing unit tests in the PR !

A suggestion: I think you need to check for fallback tenant and priviledge being created in the DB as a separate test. And I think there should be a test with deleting the fallback and making sure ops from nontenant fails

"test_vmdkops_on_fallback_tenant_vm" did this.

It test the create, attach, detach, remove command when FALLBACK tenant and FALLBACK privileges are present

It then test the vmdk ops after removing FALLBACK tenant and FALLBACK privilege

What do you mean "you need to check for fallback tenant and privileges being created in the DB as a separate test"?
FALLBACK tenant and privileges are created when table is created.

What do you mean "you need to check for fallback tenant and privileges being created in the DB as a separate test"?
FALLBACK tenant and privileges are created when table is created.

is there a sanity check that validates it ? (low priority)

In the setUp() function of class "VmdkTenantTestCase", I have the code to check whether FALLBACK tenant and FALLBACK privilege are missing. It they are missing, the code will try to create them. See function "create_fallback_tenant_and_privileges".

msterin · 2016-12-07T00:08:30Z

esx_service/utils/vmdk_utils.py

+    si = vmdk_ops.get_si()
+    try:
+        vm = [d for d in si.content.rootFolder.childEntity[0].vmFolder.childEntity if d.config.uuid == vm_uuid]
+        return vm[0].config.name


can it be more that 1 element in the list ?

I don't think so. I think the vm name should be unique?

I think the vm name should be unique?

no they are not

Then how do we handle this? Assume we already has an VM named "vm1", then I try to create another vm also named "vm1". Then how can I tell which VM is the one I just created?

I think in one esx, the vm name should be unique, right?

Made change to "create_vm" and "remove_vm" to avoid using functino "find_vm_by_name".

msterin · 2016-12-07T00:08:56Z

esx_service/vmdk_ops_test.py

@@ -251,6 +251,7 @@ def testPolicy(self):
            err = vmdk_ops.removeVMDK(vmdk_path)
            self.assertEqual(err == None, unit[2], err)

+@unittest.skip("Liping remove")


did you want to remove it :-) ?

lipingxue · 2016-12-07T21:36:14Z

I have adressed all comments. Please take a look.

lipingxue · 2016-12-08T18:50:36Z

CI test failed in "VmdkClone" test. I have added more debug info in the change to debug the CI failure.

Please ignore the change related to that.
All other comments have been addressed. Please review it.
@msterin @pdhamdhere

msterin

Please
(1) revert DEBUG log level back to INFO in log_config.py
(2) add a few words for to description about WHAT cases are covered by tests and
(3) CI should pass (need int() in kvESX.py:convert() per our discussion.

After that it's LGTM.

msterin · 2016-12-08T21:36:39Z

esx_service/utils/log_config.py

@@ -30,7 +30,7 @@
 # since we rely on it to locate log file name after config is loaded.
 LOG_CONFIG_FILE = "/etc/vmware/vmdkops/log_config.json"

-LOG_LEVEL_DEFAULT = 'INFO'
+LOG_LEVEL_DEFAULT = 'DEBUG'


this should be changed back before merge

Add unit test to test vmdkops on VM which belongs to __FALLBACK tenant.

…moving old volumes which are left from previous test run.

… "find_vm_by_name". Remove function "find_vm_by_name" since vm name is not unique and we cannot find vm by name.

There is a python2 and python 3 compitible issue with division. In python 2, 1 / 2 will return 0 (an integer value), but in python 3, 1/2 will return 0.5(a floating value).

"_DEFAULT" tenant and "_DEFAULT" privilege.

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

shaominchen reviewed Dec 3, 2016

View reviewed changes

pdhamdhere reviewed Dec 6, 2016

View reviewed changes

msterin reviewed Dec 7, 2016

View reviewed changes

lipingxue force-pushed the issue_621.liping branch from 18599d4 to 2f3838c Compare December 7, 2016 21:34

lipingxue force-pushed the issue_621.liping branch from 1d9a423 to d43b42f Compare December 8, 2016 18:10

msterin reviewed Dec 8, 2016

View reviewed changes

lipingxue added 6 commits December 8, 2016 13:40

Implement __FALLBACK tenant and __FALLBACK privileges.

2eb1034

Add unit test to test vmdkops on VM which belongs to __FALLBACK tenant.

Fix a bug in "cleanup" function of class "VmdkTenantTestCase" when re…

3d2f3ea

…moving old volumes which are left from previous test run.

Address comments from Prashant, Mark and Sam.

d02ebfa

Modify function "create_vm" and "remove_vm" to avoid calling function…

101b90f

… "find_vm_by_name". Remove function "find_vm_by_name" since vm name is not unique and we cannot find vm by name.

Add debug log in function "cloneVMDK".

4242aef

Fix a bug in function "convert" in kvESX.py.

58da306

There is a python2 and python 3 compitible issue with division. In python 2, 1 / 2 will return 0 (an integer value), but in python 3, 1/2 will return 0.5(a floating value).

lipingxue force-pushed the issue_621.liping branch from d43b42f to 58da306 Compare December 8, 2016 21:41

Rename "__FALLBACK" tenant and "__FALLBACK" privilege to

9d0463d

"_DEFAULT" tenant and "_DEFAULT" privilege.

lipingxue merged commit 0e08691 into master Dec 10, 2016

kerneltime deleted the issue_621.liping branch December 12, 2016 18:20

Tenancy: Implement __FALLBACK tenant and __FALLBACK privilege #788

Tenancy: Implement __FALLBACK tenant and __FALLBACK privilege #788

Conversation

lipingxue commented Dec 2, 2016

lipingxue commented Dec 2, 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

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

lipingxue commented Dec 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipingxue Dec 7, 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

lipingxue commented Dec 7, 2016

lipingxue commented Dec 8, 2016

msterin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tenancy: Implement FALLBACK tenant and FALLBACK privilege #788

Tenancy: Implement FALLBACK tenant and FALLBACK privilege #788

lipingxue Dec 7, 2016 •

edited

Loading

msterin left a comment •

edited

Loading