Confusing error message from volume.create when datastore is not writ… #718

BaluDontu · 2016-11-08T01:36:56Z

Handled both scenarios as outlined below:

datastore is not writable
permission denied to create dockvols directory in datastore.
This is extension to #705 and is part of fix for #568. Addressed Mark's comments over here.

+CC @pdhamdhere @msterin

osfs-mkdir -n returns different error codes when it is not able to create the dockvols directory in /vmfs/volumes/. It returns error codes 30 and 13 for scenarios 1 and 2 described above. These error codes are used to give a proper errorMsg to the user.

…able - Addressed Mark's comments

BaluDontu · 2016-11-08T23:13:57Z

@pdhamdhere : Can you verify this. All comments have been addressed.

msterin

The code seems to print the error message even if the RunCommand is successfull. Please take a look. Also - please mention how you tested it and cut--n-paste log fragment resulting from testing

msterin · 2016-11-08T23:47:32Z

esx_service/vmdk_ops.py

+        else:
+            logging.warning("Failed to initialize volume path %s", path)
+            errMsg = "Failed to initialize volume path {0}".format(path)
+        #creation of dockvols directory failed.


There is no need to duplicate the message. You can simply use

errMsg = "Failed to initialize volume path {0}".format(path) logging.warning(errMsg)

msterin · 2016-11-08T23:49:33Z

esx_service/vmdk_ops.py

-           logging.warning("Failed to create %s", path)
-           return None
+           logging.warning("Failed to initialize volume path %s", path)
+           errMsg = "Failed to initialize volume path {0}".format(path)


There is no need to duplicate the message. You can simply use

errMsg = "Failed to initialize volume path {0}".format(path) logging.warning(errMsg)

nit: Besides, I recall seeing the same message a few lines ago, You may want to reuse the format string (you can define it once in the beginning of the function)

msterin · 2016-11-08T23:50:58Z

esx_service/vmdk_ops.py

+            errMsg = "{0} creation failed - Permission denied on {1}".format(DOCK_VOLS_DIR, datastore)
+        else:
+            logging.warning("Failed to initialize volume path %s", path)
+            errMsg = "Failed to initialize volume path {0}".format(path)


won't it hit the 'else' clause and print this error message even when RunCommand was successful ? I think you need to have

elif rc == 0: pass

somewhere

Sorry, I missed out on this. Only tested wrong scenarios.

pdhamdhere · 2016-11-08T23:59:40Z

esx_service/vmdk_ops.py

-            logging.warning("Failed to create %s", dock_vol_path)
-            return None
+        errMsg = None
+        if rc == ERR_OSFS_MKDIR_READONLY:


Rather than special casing error codes, why not use existing library function to translate error code to string? I am not a python expert but tried following and seems to be fine;

os.strerror(13)
'Permission denied'
os.strerror(30)
'Read-only file system'

good point. It also removes the need to have special case per failure

Great point. Modified the code according to your suggestion. Using this, no special checks are required for error codes.

…nction

BaluDontu · 2016-11-09T01:50:14Z

In the below example nfsstore-baominw1 is a read-only nfs datastore mounted on ESX host, so creation failed.

root@photon-a4ZgLxlWh [ ~/CNA/docker-volume-vsphere ]# docker volume create -d=vmdk --name=hello@nfsstore-baominw1
**Error response from daemon: create hello@nfsstore-baominw1: VolumeDriver.Create: dockvols creation failed - Read-only file system on nfsstore-baominw1**

In the below example nfsstore-brandonn is a nfs datastore with no access permissions, so creation failed.

root@photon-a4ZgLxlWh [ ~/CNA/docker-volume-vsphere ]# docker volume create -d=vmdk --name=hello@nfsstore-brandonn
**Error response from daemon: create hello@nfsstore-brandonn: VolumeDriver.Create: dockvols creation failed - Permission denied on nfsstore-brandonn**

In the below example nfsstore-bdontu2 is a nfs datastore with full access, so it created successfully.

root@photon-a4ZgLxlWh [ ~/CNA/docker-volume-vsphere ]# docker volume create -d=vmdk --name=hello@nfsstore-bdontu2 
**hello@nfsstore-bdontu2**

On ESX host, I can see the VMDK created.

[root@promc-2n-dhcp105-41:/vmfs/volumes] cd nfsstore-bdontu2/dockvols/
**[root@promc-2n-dhcp105-41:/vmfs/volumes/6e5cea93-c7ca09c0/dockvols] ls
hello-a863f5c847c54e2d.vmfd  hello-flat.vmdk              hello.vmdk**

pdhamdhere · 2016-11-09T04:36:33Z

esx_service/vmdk_ops.py

    if not os.path.isdir(dock_vol_path):
        # The osfs tools are usable for DOCK_VOLS_DIR on all datastores
        cmd = "{0} {1}".format(OSFS_MKDIR_CMD, dock_vol_path)
-        rc, out = RunCommand(cmd)


Why did you delete this code? This need to exist. See #687
OSFS_MKDIR_CMD is used to create dock_vol_path and MKDIR_CMD is used to create Tenant folder.

I didn't delete any code. It achieves the same functionality as before. I just remodified the code so that it looks simpler now.

BaluDontu · 2016-11-09T20:35:42Z

esx_service/vmdk_ops.py

    if not os.path.isdir(dock_vol_path):
        # The osfs tools are usable for DOCK_VOLS_DIR on all datastores
        cmd = "{0} {1}".format(OSFS_MKDIR_CMD, dock_vol_path)
-        rc, out = RunCommand(cmd)


I didn't delete any code. It achieves the same functionality as before. I just remodified the code so that it looks simpler now.

BaluDontu · 2016-11-09T20:35:44Z

esx_service/vmdk_ops.py

-        if rc != 0:
-           logging.warning("Failed to create %s", path)
-           return None
+    rc, out = RunCommand(cmd)


If it is a dockvols creation, it sets the command to OSFS_MKDIR_CMD and if it is tenant creation, it sets the command to MKDIR_CMD. Once it exits the if condition, it will take the command and execute it.

BaluDontu · 2016-11-09T23:03:53Z

My previous review, doesn't create execute OSFS command to execute dockvols directory. It uses mkdir instead. Fixed the code to make sure dockvols is created using OSFS and tenant directory is created using mkdir.

Test steps:

First created a tenant using admin CLI
[root@promc-2n-dhcp105-41:/usr/lib/vmware/vmdkops/bin] ./vmdkops_admin.py tenant create --name T11 --vm-list photon-new-1
tenant create succeeded

[root@promc-2n-dhcp105-41:/usr/lib/vmware/vmdkops/bin] ./vmdkops_admin.py tenant access add --name T11 --datastore nfsstore-final --rights create,delete,mou
nt
{'mount_volume': 1, 'global_visibility': 0, 'max_volume_size': 0, 'create_volume': 1, 'delete_volume': 1, 'datastore': 'nfsstore-final', 'usage_quota': 0}
tenant access add succeeded

On docker host, executed the following command
root@photon-a4ZgLxlWh [ ~ ]# docker volume create -d=vmdk --name=helloT11@nfsstore-final
helloT11@nfsstore-final

It created the volume successfully. Verifying the logs I was able to confirm that first it created dockvols using OSFS command and created a tenant using mkdir command.

pdhamdhere

Minor change needed below.

pdhamdhere · 2016-11-10T00:13:39Z

esx_service/vmdk_ops.py

        cmd = "{0} {1}".format(MKDIR_CMD, path)
        rc, out = RunCommand(cmd)
        if rc != 0:
-           logging.warning("Failed to create %s", path)
-           return None
+            errMsg = "Failed to initialize volume path {0}".format(path)


Why not include error string here?
errMsg = "Failed to initialize volume path {0} : {1}".format(path, os.strerror(rc))

The problem is mkdir is that it returns either 0 or 1. 0 on success and 1 on failure for any reason.

Inorder to simulate this condition, where it fails on creating a tenant and is successful on dockvol creation. I have changed the access permissions on dockvol.

For example, when I printed the rc, out values to log
[photon-new-1-type@nfsstore-thanks] [DEBUG ] ****rc: 1, out: mkdir: can't create directory '/vmfs/volumes/nfsstore-thanks/dockvols/T12': Permission denied

Here, it clearly shows the error statement but return code is 1 which stands for "Operation not permitted".

os.strerror(1)
'Operation not permitted'

For that reason, I just settled with a normal message without the reason.

Why not then log "out" which already has error string?

BaluDontu · 2016-11-11T23:27:48Z

Previously specific error message was not displayed on failure of tenant creation. This commit will fix it.

Test steps:

Create a tenant on a datastore which doesn't have access permissions. You can now see specific error message now.

root@photon-a4ZgLxlWh [ ~ ]# docker volume create -d=vmdk --name=vol1@nfsstore-new
Error response from daemon: create vol1@nfsstore-new: VolumeDriver.Create: Failed to initialize volume path /vmfs/volumes/nfsstore-new/dockvols/T15 - mkdir: can't create directory '/vmfs/volumes/nfsstore-new/dockvols/T15': Permission denied

This is similar to one in https://github.com/vmware/docker-volume-vsphere/blob/master/esx_service/vmdk_ops.py#L175 where the whole error message is displayed. So followed the same style.

I can also rip out the just the error reason. for example in mkdir: can't create directory '/vmfs/volumes/nfsstore-new/dockvols/T15': Permission denied, I can just print the error reason - Permission denied by using code out.split(':')[-1] but sometimes it might lead to index out of range errors when error reason is not specified. So went ahead with printing the whole error message which can be easy understood by the user.

pdhamdhere

LGTM

Confusing error message from volume.create when datastore is not writ…

646bd04

…able - Addressed Mark's comments

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

msterin reviewed Nov 8, 2016

View reviewed changes

pdhamdhere reviewed Nov 8, 2016

View reviewed changes

Balu Dontu added 2 commits November 8, 2016 17:37

Addressed review comments from Mark and Prashanth - Use OS errcode fu…

56fe3be

…nction

Removed OSFS error code global variables

832fbdf

msterin approved these changes Nov 9, 2016

View reviewed changes

pdhamdhere suggested changes Nov 9, 2016

View reviewed changes

BaluDontu commented Nov 9, 2016

View reviewed changes

Balu Dontu added 2 commits November 9, 2016 13:33

Merge branch 'master' into Bug_CreateVolumeOnReadOnlyDatastore3

02d74b2

Fix to make sure dockvols is created before tenant directory is created

a528ed6

pdhamdhere suggested changes Nov 10, 2016

View reviewed changes

Added specific error message during tenane creation failure

11631ab

pdhamdhere approved these changes Nov 11, 2016

View reviewed changes

pdhamdhere merged commit f39df6e into vmware-archive:master Nov 12, 2016

pdhamdhere mentioned this pull request Nov 12, 2016

Confusing error message from volume.create when datastore is not writable #568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confusing error message from volume.create when datastore is not writ… #718

Confusing error message from volume.create when datastore is not writ… #718

BaluDontu commented Nov 8, 2016

BaluDontu commented Nov 8, 2016

msterin left a comment

msterin Nov 8, 2016

msterin Nov 8, 2016

msterin Nov 8, 2016

BaluDontu Nov 9, 2016

pdhamdhere Nov 8, 2016

msterin Nov 9, 2016

BaluDontu Nov 9, 2016

BaluDontu commented Nov 9, 2016 •

edited by msterin

Loading

pdhamdhere Nov 9, 2016

BaluDontu Nov 9, 2016

BaluDontu Nov 9, 2016

BaluDontu Nov 9, 2016

BaluDontu commented Nov 9, 2016

pdhamdhere left a comment

pdhamdhere Nov 10, 2016

BaluDontu Nov 10, 2016 •

edited

Loading

pdhamdhere Nov 11, 2016

BaluDontu commented Nov 11, 2016

pdhamdhere left a comment

Confusing error message from volume.create when datastore is not writ… #718

Confusing error message from volume.create when datastore is not writ… #718

Conversation

BaluDontu commented Nov 8, 2016

BaluDontu commented Nov 8, 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

BaluDontu commented Nov 9, 2016 • edited by msterin 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

BaluDontu commented Nov 9, 2016

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BaluDontu Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BaluDontu commented Nov 11, 2016

pdhamdhere left a comment

Choose a reason for hiding this comment

BaluDontu commented Nov 9, 2016 •

edited by msterin

Loading

BaluDontu Nov 10, 2016 •

edited

Loading