Tenancy bug fixes #706

pdhamdhere · 2016-11-05T01:43:23Z

Fixed following issues;

Creating volume with default size fails for tenant VM.
If VM doesn't have access VM Home datastore then docker volume ls fails with list vmdk: VolumeDriver.List: No mount privilege error. Fixes Tenancy: Better error message for docker commands #686
If VM doesn't have access to VM Home datastore but have access to nonVMDatastore, user still can't create volumes on nonVMDatastore. Fixes Multitenancy - Unable to create a volume on a datastore which is not default #703
Removed auth checks for get/list command.
"--remove-volume: true" replaced with "—remove-volume"
"--remove-volume" if specified was deleting all volumes but was not deleting tenant directory.
Print human readable default values Fixes Admin CLI tenant help should print default values #683 Fixes Improve the format of "tenant access ls" output #682
tenant rm <T_Name> changed to tenant rm --name=<T_Name> Fixes Tenancy: Tenant rm command needs updates #688 Tenant cli improvement #675

Testing Done:

docker volume create -d vmdk --name=vol1 from tenant VM.
Grant access to nonVMDatastore. Do not grant access to VM Home datastore.
- docker volume ls succeeds
- docker volume create -d vmdk --name=vol2@nonVMDatastore succeeds
For sanity checking, confirmed docker run with and without mount privilege.
Print human readable default values;

vmdkops_admin tenant access ls --name=T2
Datastore     Create_volume  Delete_volume  Mount_volume  Max_volume_size  Total_size  
------------  -------------  -------------  ------------  ---------------  ----------  
ssdDatastore  True           True           False         2.00TB           Unset

#668 Unit test should take into account updated behavior in this change.

lipingxue · 2016-11-06T00:56:11Z

esx_service/vmdk_ops.py

@@ -565,7 +565,7 @@ def executeRequest(vm_uuid, vm_name, config_path, cmd, full_vol_name, opts):
    Returns None (if all OK) or error string
    """
    vm_datastore = get_datastore_name(config_path)
-    error_info, tenant_uuid, tenant_name = auth.authorize(vm_uuid, vm_datastore, cmd, opts)
+    error_info, tenant_uuid, tenant_name = auth.get_tenant(vm_uuid)


Why do we need to call auth.get_tenant here. I think later auth.authorize will call this function inernally?

With this PR we skip authorization checks for list and get command. However, we do need tenantName to list volumes and thus call to get_tenant

lipingxue · 2016-11-06T00:59:26Z

esx_service/utils/auth_data.py

@@ -667,12 +666,12 @@ def _remove_volumes_for_tenant(self, tenant_id):
                        return err


If we hit error when deleting one volume, I think we should continue to delete other volumes. We should return error after go through all volumes.

Good point. Will fix this.

lipingxue

LGTM after fix the minor change I mentioned.

lipingxue · 2016-11-06T05:57:21Z

esx_service/cli/vmdkops_admin_test.py

@@ -85,7 +85,7 @@ def test_tenant_create_missing_option_fails(self):
        self.assert_parse_error('tenant create')

    def test_tenant_rm(self):
-        args = self.parser.parse_args('tenant rm tenant1 --remove-volumes=True'.split())
+        args = self.parser.parse_args('tenant rm tenant1 --remove-volumes'.split())


It should be 'tenant rm --name tenant1..." since "name" is not a positional argument.

Yes, looks like I forgot to save file before commit. Will push an update.

msterin

a couple of format comments below. bad hour to review the content :-)

msterin · 2016-11-06T06:37:05Z

docs/user-guide/admin-cli.md

@@ -317,22 +317,20 @@ Remove a tenant, optionally all volumes for a tenant can be removed as well.

 Sample:
 ```
-/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant rm tenant1 --remove-volumes True
+/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py tenant rm --name=tenant1 --remove-volumes


I guess our attempt to follow docker volume command backfired. In docker, it is volume create --name=volname but volume rm name. @kernetime was also commenting on this. So I guess it is confusing. But if we do it consistent and deviate from docker volume command, I'd prefer 'tenant create ' and 'tenant rm '

Do you mean

vmdkops_admin tenant create T1 vmdkops_admin tenant rm T1

Or

vmdkops_admin tenant create --name=T1 vmdkops_admin tenant rm --name=T1

Most of tenant commands accept tenant name in the form of "--name=T1" for e.g. vmdkops_admin tenant access ls --name=T1 So, I changed rm to accept "--name=T1"

makes sense

From a usability stand point

vmdkops_admin tenant create T1 vmdkops_admin tenant rm T1

is a lot nicer, since all the operations are done on a per tenant basis maybe we should change all

vmdkops_admin tenant T1 access add --rights all

@kerneltime If we decide to change, let's make it in separate PR.

@kerneltime - vmdkops_admin tenant T1 access add --rights all is not trivial with argparse package. @andrewjstone - can you check if the above is true :-) ?

I suggest leaving it as is

I'm not really sure what is being asked about here. The way we do choice parsing is by comma seperated values with no spaces. It should be feasible to have --rights all since it's a single value. See here

msterin · 2016-11-06T06:37:58Z

esx_service/cli/vmdkops_admin.py

@@ -857,8 +859,8 @@ def tenant_rm(args):
    # If args "remove_volumes" is not specified in CLI
    # args.remove_volumes will be None
    if args.remove_volumes:
-        if args.remove_volumes == 'True':
-            remove_volumes = True
+        print "All Volumes will be removed"


this is not Python v3 compatible. Please use function print("msg"), not language construct print msg

Done. Looks like all "print" in this file are incompatible with Python 3. I don't want to pollute change so only changed newly added print @ L862.

it's OK. I will change others in one pass for P3 move (using 2x3 tool). I just want to avoid new ones popping up

Ok, PR #479 did handle 3.5 compat.

Yes, but it deviated from the working state (on print, and on list iterators, and probably more) so another pass is required. And. more importantly, we need a CI with 6/5, and pylint as a part of the build. And coverage.py in test runs. So for now I will re-fix on Py3 (or you can do it if you feel like it) and work with @kernetime to add pylinit to the build

govint · 2016-11-07T03:40:46Z

I do remember fixing this file to be Python 3.x compliant. If adding new
code then can we make it compliant?

On Mon, Nov 7, 2016 at 4:43 AM, Prashant Dhamdhere <notifications@github.com

wrote:

@pdhamdhere commented on this pull request.

In esx_service/cli/vmdkops_admin.py
#706:
@@ -857,8 +859,8 @@ def tenant_rm(args):
# If args "remove_volumes" is not specified in CLI
# args.remove_volumes will be None
if args.remove_volumes:
   if args.remove_volumes == 'True':
       remove_volumes = True
   print "All Volumes will be removed"
Done. Looks like all "print" in this file are incompatible with Python 3.
I don't want to pollute change so only changed newly added print @ L862.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#706, or mute the
thread
https://github.com/notifications/unsubscribe-auth/APHseBixQjoASdJaH5NXRa0BA0lhK0ysks5q7l8ugaJpZM4KqIUN
.

govint · 2016-11-07T04:27:18Z

esx_service/vmdk_ops.py

+    error_info, tenant_uuid, tenant_name = auth.authorize(vm_uuid, datastore, cmd, opts)
+    if error_info:
+        return err(error_info)
+


won't this authorize for Get also (called below)

No, Removed CMD_LIST & CMD_GET from check_privileges_for_command.

@govint - we do not have pylint in the build and no Py3 in CI test bed, so the py3 compatibility can rot. I will add pylint and @kerneltime - can we add py3 (latest ESX GA) to the CI ?

govint

Can admin CLI sanity tests be added for the create and ls use cases.

msterin

LGTM with 2 conditionals :-)

CI pass
Please add an issue for relevant Unit Tests - @lipingxue already has some unit tests on the list, they should also cover this PR

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

lipingxue reviewed Nov 6, 2016

View reviewed changes

lipingxue approved these changes Nov 6, 2016

View reviewed changes

msterin reviewed Nov 6, 2016

View reviewed changes

govint reviewed Nov 7, 2016

View reviewed changes

govint approved these changes Nov 7, 2016

View reviewed changes

kerneltime approved these changes Nov 7, 2016

View reviewed changes

msterin approved these changes Nov 7, 2016

View reviewed changes

administrator added 7 commits November 7, 2016 14:02

Minor bug fixes in multi-tenancy

ca9d5fa

remove extra newline

d3e6a98

No auth check needed for list command

e408e24

No auth check needed for GET command

c96568d

Admin CLI usability improvements

cff0b63

Address Liping's review comments and also fixed #675 & #688

4ebd378

Addressed review comments (nits)

2d4e401

pdhamdhere force-pushed the tenancy-bug-fixes branch from 995e482 to 2d4e401 Compare November 7, 2016 22:03

Fix CLI unit tests

44e4f0b

pdhamdhere merged commit 14dcd21 into master Nov 8, 2016

pdhamdhere deleted the tenancy-bug-fixes branch November 8, 2016 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tenancy bug fixes #706

Tenancy bug fixes #706

pdhamdhere commented Nov 5, 2016 •

edited

Loading

lipingxue Nov 6, 2016

pdhamdhere Nov 6, 2016

lipingxue Nov 6, 2016

pdhamdhere Nov 6, 2016

lipingxue left a comment

lipingxue Nov 6, 2016

pdhamdhere Nov 6, 2016

msterin left a comment

msterin Nov 6, 2016

pdhamdhere Nov 6, 2016

msterin Nov 7, 2016

kerneltime Nov 7, 2016

pdhamdhere Nov 7, 2016

msterin Nov 7, 2016

andrewjstone Nov 7, 2016

msterin Nov 6, 2016

pdhamdhere Nov 6, 2016

msterin Nov 7, 2016

govint Nov 7, 2016

msterin Nov 7, 2016

govint commented Nov 7, 2016

@pdhamdhere commented on this pull request.

govint Nov 7, 2016

pdhamdhere Nov 7, 2016

msterin Nov 7, 2016

govint left a comment

msterin left a comment

		@@ -667,12 +666,12 @@ def _remove_volumes_for_tenant(self, tenant_id):
		return err

Tenancy bug fixes #706

Tenancy bug fixes #706

Conversation

pdhamdhere commented Nov 5, 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

lipingxue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

govint commented Nov 7, 2016

@pdhamdhere commented on this pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint left a comment

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment

pdhamdhere commented Nov 5, 2016 •

edited

Loading