Print tenant name in logs instead of tenant ID #1387

shivanshu21 · 2017-06-11T02:54:20Z

Description

Changed approach after discussion with Mark. Closing previous pull request.
This fixes #955

If the request comes with a tenant_name already, I return a path with symlink name instead of tenant UUID. The OS will handle resolving symlinks.

Testing done:

Manual tests:

Create VMDK
Remove VMDK

Ran multiple runs of CI automation.
After multiple ESX server operations, logs do not show tenant UUID in vmdk_path anymore.
Here is a CI test I scheduled on build server: https://ci.vmware.run/vmware/docker-volume-vsphere/458

Log Excerpts

06/09/17 20:57:14 5055174 [photon-VM0.2-sharedVmfs-0._DEFAULT.option_volume_3132227180552437724] [INFO ] *** createVMDK: /vmfs/volumes/sharedVmfs-0/dockvols/_DEFAULT/option_volume_3132227180552437724.vmdk opts = {'fstype': 'ext4', 'size': '10gb'} vm_name=photon-VM0.2 tenant_uuid=11111111-1111-1111-1111-111111111111 datastore_url=/vmfs/volumes/591cfe9a-12f9884b-340d-020037ced216

06/09/17 20:57:27 5055174 [photon-VM0.2-sharedVmfs-0._DEFAULT.option_volume_8185258463521160967] [INFO ] *** removeVMDK: /vmfs/volumes/sharedVmfs-0/dockvols/_DEFAULT/option_volume_8185258463521160967.vmdk

06/09/17 20:57:18 5055174 [photon-VM0.2-sharedVmfs-0._DEFAULT.option_volume_182702735967048418] [INFO ] *** attachVMDK: /vmfs/volumes/sharedVmfs-0/dockvols/_DEFAULT/option_volume_182702735967048418.vmdk to photon-VM0.2 VM uuid = 564d5cac-bc2b-bfde-45ca-14f6f8437476

vmwclabot · 2017-06-11T02:54:25Z

@shivanshu21, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

msterin

Overall looking good. A few comments inside - please take a look

msterin · 2017-06-12T07:31:17Z

esx_service/vmdk_ops.py


-    path = dock_vol_path = os.path.join("/vmfs/volumes", datastore, DOCK_VOLS_DIR)
+    rpath = path = dock_vol_path = os.path.join("/vmfs/volumes", datastore, DOCK_VOLS_DIR)


what does "r" stands for in "rpath" ? It would be better to use full names, whatever it is .... - easier to read

msterin · 2017-06-12T07:32:07Z

esx_service/vmdk_ops.py

@@ -696,20 +696,24 @@ def get_vol_path(datastore, tenant_name=None):
    # is created on <datastore>/DOCK_VOLS_DIR/tenant_uuid
    # a symlink <datastore>/DOCK_VOLS_DIR/tenant_name will be created to point to
    # path <datastore>/DOCK_VOLS_DIR/tenant_uuid
+    # If the command is under a tenant then the path returned contains tenant


I am not sure what "command is under a tenant" mean. Can you elaborate ?

This is just an extension of the already present comment at the start of this function mentioned below:

"If the command is NOT running under a tenant, the folder for Docker volumes is created on /DOCK_VOLS_DIR"

By this the original commenter means that tenant_name is not none in the args.

then let's change the comment (including the original one) so it is clear

msterin · 2017-06-12T07:34:35Z

esx_service/vmdk_ops.py


    if tenant_name:
        error_info, tenant = auth_api.get_tenant_from_db(tenant_name)
        if error_info:
            logging.error("get_vol_path: failed to find tenant info for tenant %s", tenant_name)
            path = dock_vol_path
        path = os.path.join(dock_vol_path, tenant.id)
+        rpath = os.path.join(dock_vol_path, tenant_name)

    if os.path.isdir(path):


should not it be if os.path.ispath(rpath) ?

msterin · 2017-06-12T07:36:27Z

esx_service/vmdk_ops.py


    if os.path.isdir(path):
        # If the path exists then return it as is.
-        logging.debug("Found %s, returning", path)
-        return path, None
+        logging.debug("Found %s, returning", rpath)


You also should check (out of healty paranoia) if os.path.exists(os.path.realpath(rpath)) and thow or print a warning if it does not

luomiao · 2017-06-12T07:45:09Z

esx_service/vmdk_ops.py


    if tenant_name:
        error_info, tenant = auth_api.get_tenant_from_db(tenant_name)
        if error_info:
            logging.error("get_vol_path: failed to find tenant info for tenant %s", tenant_name)
            path = dock_vol_path
        path = os.path.join(dock_vol_path, tenant.id)
+        rpath = os.path.join(dock_vol_path, tenant_name)


why rpath? (in the sense, what's "r" stands for?)
update: oops just saw this one is duplicated with Mark's review above :)

And it looks like rpath is in fact the same thing as the symlink_path on Line 739. Can you just use symlink_path here?

luomiao · 2017-06-12T21:02:11Z

esx_service/vmdk_ops.py

-        # If the path exists then return it as is.
-        logging.debug("Found %s, returning", path)
-        return path, None
+    if os.path.isdir(readable_path) or os.path.isdir(path):


I just feel the new logic here is different from old ones...
Before this fix, if path is not a dir, it will go ahead to the following steps to create it or run necessary things.
But the new logic is, as long as the readable_path is there (even path is not a dir), it will never go to the rest of the code.
Is it possible that, the original path is getting deleted but the symlink path is still existing. Then the original path will never be created again?
I am not sure if this is a possible situation. Just raise the concern.
So I think it's better to keep this check as it was "if os.path.isdir(path):", but add your following checks.

luomiao · 2017-06-12T21:18:12Z

And sorry I just noticed here: line 751
I guess you may want to add the return of readable_path here too (with the checks).

And btw for line 734, it looks like the second half "not os.path.isdir(path)" check is not necessary.

shivanshu21 · 2017-06-12T21:49:22Z

@luomiao We can leave line 734 for abundance of caution. The original writer might have put it for some corner case. Fixed line 751.

luomiao · 2017-06-12T21:53:23Z

LGTM but need Mark's final call :)

msterin

One small request (change logging.debug to logging,warning) and it's ready to merge when CI passes.
Thanks for taking care of this !

msterin · 2017-06-12T22:54:08Z

esx_service/vmdk_ops.py

+            logging.debug("Found %s, returning", readable_path)
+            return readable_path, None
+        else:
+            logging.debug("Tenant name symlink not found for path %s", readable_path)


this should be logging.warning("internal: Tenant name symlink not found for path %s. Using UUID path instead") - it is not a normal situation

Addressing Marks and Miaos comments Fixing comment Addressing Miaos comments Changing no symlink case to warning

pshahzeb · 2017-06-14T09:15:18Z

Merging this. Comments have been addressed by @shivanshu21 .

msterin reviewed Jun 12, 2017

View reviewed changes

luomiao reviewed Jun 12, 2017

View reviewed changes

shivanshu21 force-pushed the vmdkpath.shivanshug branch from dcfa72d to dd8fa74 Compare June 12, 2017 22:38

msterin approved these changes Jun 12, 2017

View reviewed changes

vmdk path without tenant id

9bf8993

Addressing Marks and Miaos comments Fixing comment Addressing Miaos comments Changing no symlink case to warning

shivanshu21 force-pushed the vmdkpath.shivanshug branch from dd8fa74 to 9bf8993 Compare June 13, 2017 04:31

pshahzeb merged commit 0c6cda5 into vmware-archive:master Jun 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print tenant name in logs instead of tenant ID #1387

Print tenant name in logs instead of tenant ID #1387

shivanshu21 commented Jun 11, 2017

vmwclabot commented Jun 11, 2017

msterin left a comment

msterin Jun 12, 2017

msterin Jun 12, 2017

shivanshu21 Jun 12, 2017 •

edited

Loading

msterin Jun 12, 2017

msterin Jun 12, 2017

msterin Jun 12, 2017

luomiao Jun 12, 2017 •

edited

Loading

luomiao Jun 12, 2017

luomiao commented Jun 12, 2017

shivanshu21 commented Jun 12, 2017

luomiao commented Jun 12, 2017

msterin left a comment

msterin Jun 12, 2017

pshahzeb commented Jun 14, 2017


		path = dock_vol_path = os.path.join("/vmfs/volumes", datastore, DOCK_VOLS_DIR)
		rpath = path = dock_vol_path = os.path.join("/vmfs/volumes", datastore, DOCK_VOLS_DIR)

Print tenant name in logs instead of tenant ID #1387

Print tenant name in logs instead of tenant ID #1387

Conversation

shivanshu21 commented Jun 11, 2017

Description

Testing done:

Log Excerpts

vmwclabot commented Jun 11, 2017

msterin left a comment

Choose a reason for hiding this comment

msterin Jun 12, 2017

Choose a reason for hiding this comment

msterin Jun 12, 2017

Choose a reason for hiding this comment

shivanshu21 Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

msterin Jun 12, 2017

Choose a reason for hiding this comment

msterin Jun 12, 2017

Choose a reason for hiding this comment

msterin Jun 12, 2017

Choose a reason for hiding this comment

luomiao Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

luomiao Jun 12, 2017

Choose a reason for hiding this comment

luomiao commented Jun 12, 2017

shivanshu21 commented Jun 12, 2017

luomiao commented Jun 12, 2017

msterin left a comment

Choose a reason for hiding this comment

msterin Jun 12, 2017

Choose a reason for hiding this comment

pshahzeb commented Jun 14, 2017

shivanshu21 Jun 12, 2017 •

edited

Loading

luomiao Jun 12, 2017 •

edited

Loading