Make change to authorize() function to take "datastore_url" and volume create fail after datastore rename #979

lipingxue · 2017-03-01T19:18:34Z

This PR is the second part of fix for #818. This PR includes:

make change to authorize() function to take "datastore_url" to avoid unnecessary conversion between datastore_url and datastore_name during the authorize process.
fix the issue that volume create failure after datastore rename by forcing datastore cache refresh in executeRequest() function. We use "os.path.exists("/vmfs/volumes/datastore_name")" to determine whether the datastore with given "datastore_name" exist or not. If the datastore does not exist, force refresh the datastore cache.
corresponding changes to test code.

lipingxue · 2017-03-01T19:39:16Z

Testing Done:

Configuration 1:
One VM "photon6" is created on "datastore1", and this VM is associated with _DEFAULT tenant

Case 1.1: after VM datastore "datastore1" rename, create volume with short name succeeded.

root@photon-KwqUODFXp [ ~ ]# docker volume create --driver=vsphere --name=vol1
vol1

---- rename "datastore1" to "new-datastore1"

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

Case1.2: after VM datastore "datastore1" rename, create volume with volname@old-datastore failed, and create volume with volname@new-datastore succeeded.

root@photon-KwqUODFXp [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             vol1@datastore1
vsphere             vol2@datastore1

---- rename "datastore1" to "new-datastore1"

root@photon-KwqUODFXp [ ~ ]# docker volume create --driver=vsphere --name="vol3@datastore1"
Error response from daemon: create vol3@datastore1: VolumeDriver.Create: Invalid datastore 'datastore1'.
Known datastores: new-datastore1, datastore2.
Default datastore: new-datastore1
root@photon-KwqUODFXp [ ~ ]# docker volume create --driver=vsphere --name="vol3@new-datastore1"
vol3@new-datastore1
root@photon-KwqUODFXp [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             vol1@new-datastore1
vsphere             vol2@new-datastore1
vsphere             vol3@new-datastore1

Configuration2:
One VM "photon7" is created on "datastore1", this VM is associated with tenant "tenant1". The default_datastore for "tenant1" is "datastore1"

Case2.1: after default datastore "datastore1", rename, create volume with short name succeeded.

Case3:
root@photon-P0itmvfOB [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             tenant1-vol1@datastore1

----rename "datastore1" to "new-datastore1"

root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol2
tenant1-vol2
root@photon-P0itmvfOB [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             tenant1-vol1@new-datastore1
vsphere             tenant1-vol2@new-datastore1

Case2.2: after non default datastore "datastore2" rename, create volume with full name volname@old-datastore failed, and create volume with volname@new-datastore succeeded.

root@photon-P0itmvfOB [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             tenant1-vol1@datastore1
vsphere             tenant1-vol2@datastore1

--- rename "datastore2" to "new-datastore2"

root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol3@datastore2
Error response from daemon: create tenant1-vol3@datastore2: VolumeDriver.Create: Invalid datastore 'datastore2'.
Known datastores: datastore1, new-datastore2.
Default datastore: datastore1
root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol3@new-datastore2
tenant1-vol3@new-datastore2
root@photon-P0itmvfOB [ ~ ]# docker volume ls
DRIVER              VOLUME NAME
vsphere             tenant1-vol1@datastore1
vsphere             tenant1-vol2@datastore1
vsphere             tenant1-vol3@new-datastore2

lipingxue · 2017-03-01T19:39:33Z

@msterin @shaominchen Please review it.

msterin

Good change. Comments inside - mainly about some refactoring and a few nits.
Also, did you test (manually or automated) setting limits on a tenant (default tenant is the best to test here) ?

msterin · 2017-03-01T19:41:59Z

esx_service/cli/vmdkops_admin_test.py

@@ -288,7 +288,7 @@ def setUp(self):
        """ Setup run before each test """
        self.vol_count = 0
        self.cleanup()
-        for (datastore, url_name, path) in vmdk_utils.get_datastores():
+        for (datastore, url_name, path, url) in vmdk_utils.get_datastores():


what's the difference between url_name and url ?

I have the same confusion. We should rename these 2 variables to clarify the difference.

url_name is part of url.
As we discussed offline, will remove "url_name" from vmdk_utils.get_datastores()

msterin · 2017-03-01T19:44:45Z

esx_service/utils/auth_api.py

@@ -250,7 +244,7 @@ def generate_privileges_dict(privileges):

 def get_default_datastore(name):


nit: better to rename to get_default_datastore_url.

Agree with Mark.

msterin · 2017-03-01T19:47:26Z

esx_service/utils/auth_data.py

@@ -227,7 +227,7 @@ def set_name(self, conn, name, new_name):
        # rename the old symbol link /vmfs/volumes/datastore_name/tenant_name
        # to a new name /vmfs/volumes/datastore_name/new_tenant_name
        # which still point to path /vmfs/volumes/datastore_name/tenant_uuid
-        for (datastore, url_name, path) in vmdk_utils.get_datastores():
+        for (datastore, url_name, path, url) in vmdk_utils.get_datastores():


same question - what's the difference between url_name and url ?

msterin · 2017-03-01T19:48:39Z

esx_service/utils/auth_data.py

                if err:
                    logging.error("remove vmdk %s failed with error %s", vmdk_path, err)
                    error_msg += str(err)

            VOL_RM_LOG_PREFIX = "Tenant <name> %s removal: "
            # delete the symlink /vmfs/volume/datastore_name/tenant_name
            # which point to /vmfs/volumes/datastore_name/tenant_uuid
-            for (datastore, url_name, path) in vmdk_utils.get_datastores():
+            for (datastore, url_name, path, url) in vmdk_utils.get_datastores():


nit - this section seems to be cut-n-pasted afew times, better to have a function

msterin · 2017-03-01T19:51:01Z

esx_service/vmdk_ops.py

+                  % (config_path, config_ds_url))
+    return config_ds_url
+
+def datastore_path_exist(datastore):


nit: either datastore_name or just name (instead of 'datastore) in params. Datastore is confusing

msterin · 2017-03-01T19:53:54Z

esx_service/vmdk_ops.py

@@ -764,24 +773,45 @@ def executeRequest(vm_uuid, vm_name, config_path, cmd, full_vol_name, opts):
    Returns None (if all OK) or error string
    """
    logging.debug("config_path=%s", config_path)
-    vm_datastore = get_datastore_name(config_path)
+    vm_datastore_url = get_datastore_url_from_config_path(config_path)
+    vm_datastore = vmdk_utils.get_datastore_name(vm_datastore_url)


this block spreads the knowledge about cache manipulation onto too many places.
I think it would be way better to hide it , say in get_datastore_name() ... it can check get_datastores() and reset the cache internally if needed

msterin · 2017-03-01T19:55:03Z

esx_service/vmdk_ops.py

            default_datastore = vm_datastore
+        else:
+             default_datastore = vmdk_utils.get_datastore_name(default_datastore_url)
+             if not datastore_path_exist(default_datastore):


same comment as above , plus copy-n-paste is evil :-) (makes it much harder to change later) .
Please consider hiding the path check and cache update inside of get_datastore_name()

Totally agree - cache manipulation (refresh on demand) is the API's responsibility, not the API user's.

msterin · 2017-03-01T19:56:42Z

esx_service/vmdk_ops.py


-    if not vmdk_utils.validate_datastore(datastore):
+    if datastore and not vmdk_utils.validate_datastore(datastore):


Maybe it's better to hide a check for if datastore inside of validate datastore, which should always return False if passed None ? (depends on other places in the code, so it's a nit)

Here "datastore==None" means the volume which user passed is a short name(no datastore is specified by user), so no need to validate "datastore" in this case.

msterin · 2017-03-01T19:57:33Z

esx_service/vmdk_ops.py

    if error_info:
        return err(error_info)

    # get /vmfs/volumes/<volid>/dockvols path on ESX:
+    datastore = vmdk_utils.get_datastore_name(datastore_url)


and another confirmation that we need to hide this in get_datastore_name

shaominchen

Overall looks great.

shaominchen · 2017-03-01T22:03:05Z

esx_service/cli/vmdkops_admin_test.py

@@ -288,7 +288,7 @@ def setUp(self):
        """ Setup run before each test """
        self.vol_count = 0
        self.cleanup()
-        for (datastore, url_name, path) in vmdk_utils.get_datastores():
+        for (datastore, url_name, path, url) in vmdk_utils.get_datastores():


I have the same confusion. We should rename these 2 variables to clarify the difference.

shaominchen · 2017-03-01T22:05:57Z

esx_service/utils/auth_api.py

@@ -250,7 +244,7 @@ def generate_privileges_dict(privileges):

 def get_default_datastore(name):


Agree with Mark.

shaominchen · 2017-03-01T22:11:51Z

esx_service/utils/auth_data.py


        Return value:
            error_msg: return None on success or error info on failure
-            datastore: return default_datastore name on success or None on failure
+            datastore: return default_datastore url on success or None on failure


Nit: datastore => datastore_url

shaominchen · 2017-03-01T22:24:50Z

esx_service/utils/vmdk_utils.py

@@ -74,7 +74,8 @@ def init_datastoreCache():
            continue
        datastores.append((datastore.info.name,
                           os.path.split(datastore.info.url)[1],


I doubt this statement may not really work. I tried to run this statement with a real datastore url, it always return empty:

import os
url =os.path.split("ds:///vmfs/volumes/vsan:525ab51e793147fd-c0f242a06e800fc2/")[1]
print(url)

If this field is incorrect and it's not being used anywhere, we should just get rid of it.

shaominchen · 2017-03-01T22:33:14Z

esx_service/utils/vmdk_utils.py

@@ -331,7 +336,7 @@ def get_datastore_name(datastore_url):
        return auth.DEFAULT_DS

    # Query datastore name from VIM API
-    res = [d.info.name for d in get_datastore_objects() if d.info.url == datastore_url]
+    res = [d[0] for d in get_datastores() if d[3] == datastore_url]


Is it possible to improve the readability to avoid the magic d[0] and d[3]?

Add some comments here.

shaominchen · 2017-03-01T22:43:11Z

esx_service/vmdk_ops.py

    else:
        logging.debug(error_code.error_code_to_message[ErrorCode.VM_NOT_BELONG_TO_TENANT].format(vm_name))


-def cloneVMDK(vm_name, vmdk_path, opts={}, vm_uuid=None, vm_datastore=None):
-    logging.info("*** cloneVMDK: %s opts = %s", vmdk_path, opts)
+def cloneVMDK(vm_name, vmdk_path, opts={}, vm_uuid=None, vm_datastore_url=None):


Just for consistency: rename vm_datastore_url to datastore_url.

shaominchen · 2017-03-01T22:49:45Z

esx_service/vmdk_ops.py

            default_datastore = vm_datastore
+        else:
+             default_datastore = vmdk_utils.get_datastore_name(default_datastore_url)
+             if not datastore_path_exist(default_datastore):


Totally agree - cache manipulation (refresh on demand) is the API's responsibility, not the API user's.

lipingxue · 2017-03-02T06:34:17Z

I also did the following test to make sure max_vol_size and total_size(quota) limit for tenant is not broken by this change.
Configuration:
Create a tenant "tenant1", add VM "photon7" to this tenant

/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant ls
Uuid                                  Name      Description               Default_datastore  VM_list 
------------------------------------  --------  ------------------------  -----------------  ------- 
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default tenant                             
e6a0cef4-22ce-48f6-a9ed-a28950c7277e  tenant1                             datastore1         photon7 


/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant access ls --name=tenant1
Datastore   Allow_create  Max_volume_size  Total_size 
----------  ------------  ---------------  ---------- 
datastore1  True          500.00MB         1.00GB

root@photon-P0itmvfOB [ ~ ]# docker volume ls
DRIVER              VOLUME NAME

root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol1 -o size=600MB
Error response from daemon: create tenant1-vol1: VolumeDriver.Create: volume size exceeds the max volume size limit
root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol1 -o size=500MB
tenant1-vol1
root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol2 -o size=500MB
tenant1-vol2
root@photon-P0itmvfOB [ ~ ]# docker volume create --driver=vsphere --name=tenant1-vol3 -o size=100MB
Error response from daemon: create tenant1-vol3: VolumeDriver.Create: The total volume size exceeds the usage quota
root@photon-P0itmvfOB [ ~ ]#

shuklanirdesh82 · 2017-03-02T06:46:18Z

esx_service/utils/auth_data.py

+
+        # dockvol_path has the format like "/vmfs/volumes/<datastore_name>"
+        # tenant_path has the format like "/vmfs/volumes/<datastore_name>/tenant_id"
+        dockvol_path = os.path.join("/vmfs/volumes", datastore_name, vmdk_ops.DOCK_VOLS_DIR)


minor: can we define /vmfs/volumes as constant?

agree with this but it should delay the merge

shuklanirdesh82 · 2017-03-02T06:49:24Z

esx_service/vmdk_ops.py

+    # path can be /vmfs/volumes/<datastore_url_name>/...
+    # or /vmfs/volumes/datastore_name/...
+    # so extract datastore_url_name:
+    config_ds_url = os.path.join("/vmfs/volumes/", os.path.realpath(config_path).split("/")[3])


seen multiple occurrence of /vmfs/volumes/; it is better to declare as constant. Please refactor for each occurrences.

I personally think "/vmfs/volumes" is more readable in the context of the code, so will drop this comment.

shuklanirdesh82 · 2017-03-02T06:59:06Z

esx_service/vmdk_ops.py

-        logging.error("get_datastore_name: found more than one match: %s" % ds_name)
-    logging.debug("get_datastore_name: path=%s name=%s" % (config_ds_name, ds_name))
-    return ds_name[0]
+def get_datastore_url_from_config_path(config_path):


can this be moved to vmdk_utils as part of this PR (seems the whole method is refactored)?

shuklanirdesh82 · 2017-03-02T06:59:48Z

esx_service/vmdk_ops.py

+                  % (config_path, config_ds_url))
+    return config_ds_url
+
+def datastore_path_exist(datastore_name):


same as above .. it is better to move to vmdk_utils.

I will keep it here for now since now no other callers need this function.

shuklanirdesh82

overall look good to me .. supplying some minor comments.

shaominchen · 2017-03-02T19:21:23Z

esx_service/utils/vmdk_utils.py

@@ -324,6 +324,8 @@ def get_datastore_url(datastore_name):
        return None

    # Query datastore URL from VIM API
+    # get_datastores() return a list of tuple
+    # each tuple has for format like (datastore_name, datastore_url, dockvol_path)


Nit: remove the redundant "for"

shaominchen · 2017-03-02T19:21:50Z

esx_service/utils/vmdk_utils.py

@@ -335,6 +337,8 @@ def get_datastore_name(datastore_url):
        return auth.DEFAULT_DS

    # Query datastore name from VIM API
+    # get_datastores() return a list of tuple
+    # each tuple has for format like (datastore_name, datastore_url, dockvol_path)


Nit: same as above.

shaominchen · 2017-03-02T19:36:01Z

esx_service/vmdk_ops.py

+    """ Get datastore_name with given datastore_url """
+    datastore_name = vmdk_utils.get_datastore_name(datastore_url)
+    datastore_path = os.path.join("/vmfs/volumes/", datastore_name)
+    if not datastore_path_exist(datastore_path):


Shouldn't we pass the datastore_name instead of datastore_path here? See the API signature at line 757.

Yes, good catch!

shaominchen

LGTM.

msterin

LGTM. Please do not forget to squash the commits

…f "datastore_name" Fix the issue that volume create fail after datastore rename.

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

msterin suggested changes Mar 1, 2017

View reviewed changes

shaominchen suggested changes Mar 1, 2017

View reviewed changes

lipingxue force-pushed the use_ds_url_in_authorize.liping branch from 454140c to 7a59ceb Compare March 2, 2017 00:35

shuklanirdesh82 suggested changes Mar 2, 2017

View reviewed changes

shuklanirdesh82 reviewed Mar 2, 2017

View reviewed changes

shuklanirdesh82 suggested changes Mar 2, 2017

View reviewed changes

shaominchen reviewed Mar 2, 2017

View reviewed changes

shaominchen suggested changes Mar 2, 2017

View reviewed changes

lipingxue force-pushed the use_ds_url_in_authorize.liping branch from fdfedd2 to 4b23184 Compare March 2, 2017 20:07

shaominchen approved these changes Mar 2, 2017

View reviewed changes

msterin approved these changes Mar 2, 2017

View reviewed changes

lipingxue added 5 commits March 3, 2017 10:15

Make change to authorize() function to take "datastore_url" instead o…

599f227

…f "datastore_name" Fix the issue that volume create fail after datastore rename.

Address comments from Mark.

d7fbe2c

Address comments from Sam.

89f0b9a

Address additional comments from Sam and Nirdesh.

25ea84a

Trigger CI run.

12b03d0

lipingxue force-pushed the use_ds_url_in_authorize.liping branch from 4b23184 to 12b03d0 Compare March 3, 2017 18:16

lipingxue merged commit 8ab5681 into master Mar 3, 2017

lipingxue deleted the use_ds_url_in_authorize.liping branch March 3, 2017 22:17

		@@ -250,7 +244,7 @@ def generate_privileges_dict(privileges):

		def get_default_datastore(name):


		if not vmdk_utils.validate_datastore(datastore):
		if datastore and not vmdk_utils.validate_datastore(datastore):

Make change to authorize() function to take "datastore_url" and volume create fail after datastore rename #979

Make change to authorize() function to take "datastore_url" and volume create fail after datastore rename #979

Conversation

lipingxue commented Mar 1, 2017

lipingxue commented Mar 1, 2017

lipingxue commented Mar 1, 2017

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

Choose a reason for hiding this comment

shaominchen 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

lipingxue commented Mar 2, 2017

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

shuklanirdesh82 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

shaominchen left a comment

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment