From 67fa2135df7d051bf518b61d76eef846b6b6270a Mon Sep 17 00:00:00 2001 From: pshahzeb Date: Fri, 3 Mar 2017 18:05:38 -0800 Subject: [PATCH 1/2] Admincli improvement 1. Printing info message for default tenant command instead of showing an empty list 2. Removing usage of --name option for tenant rm command Testing: Updated testcases --- esx_service/cli/vmdkops_admin.py | 10 ++++++++-- esx_service/cli/vmdkops_admin_test.py | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/esx_service/cli/vmdkops_admin.py b/esx_service/cli/vmdkops_admin.py index 02897065b..a7e5dc351 100644 --- a/esx_service/cli/vmdkops_admin.py +++ b/esx_service/cli/vmdkops_admin.py @@ -236,9 +236,8 @@ def commands(): 'func': tenant_rm, 'help': 'Delete a tenant', 'args': { - '--name': { + 'name': { 'help': 'The name of the tenant', - 'required': True }, '--remove-volumes': { 'help': 'BE CAREFUL: Removes this tenant volumes when removing a tenant', @@ -914,6 +913,13 @@ def generate_tenant_vm_ls_rows(vms): def tenant_vm_ls(args): """ Handle tenant vm ls command """ + + # Handling _DEFAULT tenant case separately to print info message + # instead of printing empty list + if (args.name == "_DEFAULT"): + print("All VMs that do not belong to any tenant belong to _DEFAULT tenant") + return + error_info, vms = auth_api._tenant_vm_ls(args.name) if error_info: return operation_fail(error_info.msg) diff --git a/esx_service/cli/vmdkops_admin_test.py b/esx_service/cli/vmdkops_admin_test.py index 11c57eef0..62d8f1e57 100644 --- a/esx_service/cli/vmdkops_admin_test.py +++ b/esx_service/cli/vmdkops_admin_test.py @@ -127,13 +127,13 @@ 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 --name=tenant1 --remove-volumes'.split()) + args = self.parser.parse_args('tenant rm tenant1 --remove-volumes'.split()) self.assertEqual(args.func, vmdkops_admin.tenant_rm) self.assertEqual(args.name, 'tenant1') self.assertEqual(args.remove_volumes, True) def test_tenant_rm_without_arg_remove_volumes(self): - args = self.parser.parse_args('tenant rm --name=tenant1'.split()) + args = self.parser.parse_args('tenant rm tenant1'.split()) self.assertEqual(args.func, vmdkops_admin.tenant_rm) self.assertEqual(args.name, 'tenant1') # If arg "remove_volumes" is not specified in the CLI, then args.remove_volumes From 5d3c474afc8dec94a37c97e752f099c45658f265 Mon Sep 17 00:00:00 2001 From: pshahzeb Date: Tue, 7 Mar 2017 13:57:29 -0800 Subject: [PATCH 2/2] Admincli improvement 1. Info message for default tenant vm ls command. 2. Moving default tenant and privilege constants to auth_data_const.py Related changes for using these constants 3. Making use of --name option for policy rm and reverting the tenant rm to also make use of --name option. Testing: Updated test cases --- esx_service/cli/vmdkops_admin.py | 16 ++++++++------- esx_service/cli/vmdkops_admin_test.py | 6 +++--- esx_service/utils/auth.py | 16 ++++++--------- esx_service/utils/auth_api.py | 2 +- esx_service/utils/auth_data.py | 20 +++++++++--------- esx_service/utils/auth_data_const.py | 6 ++++++ esx_service/utils/vmdk_utils.py | 9 +++++---- esx_service/vmdk_ops.py | 3 ++- esx_service/vmdk_ops_test.py | 29 ++++++++++++++------------- 9 files changed, 57 insertions(+), 50 deletions(-) diff --git a/esx_service/cli/vmdkops_admin.py b/esx_service/cli/vmdkops_admin.py index a7e5dc351..f09dbd2c1 100644 --- a/esx_service/cli/vmdkops_admin.py +++ b/esx_service/cli/vmdkops_admin.py @@ -159,8 +159,9 @@ def commands(): 'func': policy_rm, 'help': 'Remove a storage policy', 'args': { - 'name': { - 'help': 'Policy name' + '--name': { + 'help': 'Policy name', + 'required': True } } }, @@ -236,8 +237,9 @@ def commands(): 'func': tenant_rm, 'help': 'Delete a tenant', 'args': { - 'name': { + '--name': { 'help': 'The name of the tenant', + 'required': True }, '--remove-volumes': { 'help': 'BE CAREFUL: Removes this tenant volumes when removing a tenant', @@ -808,7 +810,7 @@ def generate_tenant_ls_rows(tenant_list): uuid = tenant.id name = tenant.name description = tenant.description - if not tenant.default_datastore_url or tenant.default_datastore_url == auth.DEFAULT_DS_URL: + if not tenant.default_datastore_url or tenant.default_datastore_url == auth_data_const.DEFAULT_DS_URL: default_datastore = "" else: default_datastore = vmdk_utils.get_datastore_name(tenant.default_datastore_url) @@ -916,8 +918,8 @@ def tenant_vm_ls(args): # Handling _DEFAULT tenant case separately to print info message # instead of printing empty list - if (args.name == "_DEFAULT"): - print("All VMs that do not belong to any tenant belong to _DEFAULT tenant") + if (args.name == auth_data_const.DEFAULT_TENANT): + print("{0} tenant contains all VMs which were not added to other tenants".format(auth_data_const.DEFAULT_TENANT)) return error_info, vms = auth_api._tenant_vm_ls(args.name) @@ -988,7 +990,7 @@ def generate_tenant_access_ls_rows(privileges): """ Generate output for tenant access ls command """ rows = [] for p in privileges: - if not p.datastore_url or p.datastore_url == auth.DEFAULT_DS_URL: + if not p.datastore_url or p.datastore_url == auth_data_const.DEFAULT_DS_URL: datastore = "" else: datastore = vmdk_utils.get_datastore_name(p.datastore_url) diff --git a/esx_service/cli/vmdkops_admin_test.py b/esx_service/cli/vmdkops_admin_test.py index 62d8f1e57..80dececeb 100644 --- a/esx_service/cli/vmdkops_admin_test.py +++ b/esx_service/cli/vmdkops_admin_test.py @@ -103,7 +103,7 @@ def test_policy_create(self): self.assertEqual(args.name, 'testPolicy') def test_policy_rm(self): - args = self.parser.parse_args('policy rm testPolicy'.split()) + args = self.parser.parse_args('policy rm --name=testPolicy'.split()) self.assertEqual(args.func, vmdkops_admin.policy_rm) self.assertEqual(args.name, 'testPolicy') @@ -127,13 +127,13 @@ 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'.split()) + args = self.parser.parse_args('tenant rm --name=tenant1 --remove-volumes'.split()) self.assertEqual(args.func, vmdkops_admin.tenant_rm) self.assertEqual(args.name, 'tenant1') self.assertEqual(args.remove_volumes, True) def test_tenant_rm_without_arg_remove_volumes(self): - args = self.parser.parse_args('tenant rm tenant1'.split()) + args = self.parser.parse_args('tenant rm --name=tenant1'.split()) self.assertEqual(args.func, vmdkops_admin.tenant_rm) self.assertEqual(args.name, 'tenant1') # If arg "remove_volumes" is not specified in the CLI, then args.remove_volumes diff --git a/esx_service/utils/auth.py b/esx_service/utils/auth.py index 9b121d4dd..0673b6691 100644 --- a/esx_service/utils/auth.py +++ b/esx_service/utils/auth.py @@ -33,10 +33,6 @@ CMD_DETACH = 'detach' SIZE = 'size' -DEFAULT_TENANT = '_DEFAULT' -DEFAULT_TENANT_UUID = '11111111-1111-1111-1111-111111111111' -DEFAULT_DS = '_DEFAULT' -DEFAULT_DS_URL = DEFAULT_DS + "_URL" # thread local storage in this module namespace thread_local = threadutils.get_local_storage() @@ -71,22 +67,22 @@ def get_default_tenant(): try: cur = _auth_mgr.conn.execute( "SELECT id FROM tenants WHERE name = ?", - (DEFAULT_TENANT, ) + (auth_data_const.DEFAULT_TENANT, ) ) result = cur.fetchone() except sqlite3.Error as e: - error_msg = "Error {0} when querying from tenants table for tenant {1}".format(e, DEFAULT_TENANT) + error_msg = "Error {0} when querying from tenants table for tenant {1}".format(e, auth_data_const.DEFAULT_TENANT) logging.error(error_msg) return str(e), None, None if result: # found DEFAULT tenant tenant_uuid = result[0] - logging.debug("Found DEFAULT tenant, tenant_uuid %s, tenant_name %s", tenant_uuid, DEFAULT_TENANT) - return None, tenant_uuid, DEFAULT_TENANT + logging.debug("Found DEFAULT tenant, tenant_uuid %s, tenant_name %s", tenant_uuid, auth_data_const.DEFAULT_TENANT) + return None, tenant_uuid, auth_data_const.DEFAULT_TENANT else: # cannot find DEFAULT tenant err_code = error_code.ErrorCode.TENANT_NOT_EXIST - err_msg = error_code.error_code_to_message[err_code].format(DEFAULT_TENANT) + err_msg = error_code.error_code_to_message[err_code].format(auth_data_const.DEFAULT_TENANT) logging.debug(err_msg) return None, None, None @@ -110,7 +106,7 @@ def get_default_privileges(): try: cur = _auth_mgr.conn.execute( "SELECT * FROM privileges WHERE datastore_url = ?", - (DEFAULT_DS_URL,) + (auth_data_const.DEFAULT_DS_URL,) ) privileges = cur.fetchone() except sqlite3.Error as e: diff --git a/esx_service/utils/auth_api.py b/esx_service/utils/auth_api.py index 3a169c5f5..ffec641f1 100644 --- a/esx_service/utils/auth_api.py +++ b/esx_service/utils/auth_api.py @@ -528,7 +528,7 @@ def _tenant_vm_replace(name, vm_list): def check_datastore(datastore_name): """ Check datastore with given name is a valid datastore or not """ - if datastore_name == auth.DEFAULT_DS: + if datastore_name == auth_data_const.DEFAULT_DS: return None if not vmdk_utils.validate_datastore(datastore_name): diff --git a/esx_service/utils/auth_data.py b/esx_service/utils/auth_data.py index 5887c56d1..845c7b9cf 100644 --- a/esx_service/utils/auth_data.py +++ b/esx_service/utils/auth_data.py @@ -433,12 +433,12 @@ def get_auth_db_path(self): def create_default_tenant(self): """ Create DEFAULT tenant """ error_msg, tenant = self.create_tenant( - name=auth.DEFAULT_TENANT, + name=auth_data_const.DEFAULT_TENANT, description="This is a default tenant", vms=[], privileges=[]) if error_msg: - err = error_code.error_code_to_message[ErrorCode.TENANT_CREATE_FAILED].format(auth.DEFAULT_TENANT, error_msg) + err = error_code.error_code_to_message[ErrorCode.TENANT_CREATE_FAILED].format(auth_data_const.DEFAULT_TENANT, error_msg) logging.warning(err) def create_default_privileges(self): @@ -449,20 +449,20 @@ def create_default_privileges(self): this privilege will have full permission (create, delete, and mount) and no max_volume_size and usage_quota limitation """ - privileges = [{'datastore_url': auth.DEFAULT_DS_URL, + privileges = [{'datastore_url': auth_data_const.DEFAULT_DS_URL, 'allow_create': 1, 'max_volume_size': 0, 'usage_quota': 0}] - error_msg, tenant = self.get_tenant(auth.DEFAULT_TENANT) + error_msg, tenant = self.get_tenant(auth_data_const.DEFAULT_TENANT) if error_msg: - err = error_code.error_code_to_message[ErrorCode.TENANT_NOT_EXIST].format(auth.DEFAULT_TENANT) + err = error_code.error_code_to_message[ErrorCode.TENANT_NOT_EXIST].format(auth_data_const.DEFAULT_TENANT) logging.warning(err) return error_msg = tenant.set_datastore_access_privileges(self.conn, privileges) if error_msg: - err = error_code.error_code_to_message[ErrorCode.TENANT_SET_ACCESS_PRIVILEGES_FAILED].format(auth.DEFAULT_TENANT, auth.DEFAULT_DS, error_msg) + err = error_code.error_code_to_message[ErrorCode.TENANT_SET_ACCESS_PRIVILEGES_FAILED].format(auth_data_const.DEFAULT_TENANT, auth_data_const.DEFAULT_DS, error_msg) logging.warning(err) def get_db_version(self): @@ -525,14 +525,14 @@ def handle_upgrade_db_from_ver_1_0_to_ver_1_1(self): """ # in DB version 1.0, _DEFAULT_TENANT was created using a random generated UUID # in DB version 1.1, _DEFAULT_TENANT will be created using a constant UUID - error_msg, tenant = self.get_tenant(auth.DEFAULT_TENANT) + error_msg, tenant = self.get_tenant(auth_data_const.DEFAULT_TENANT) if error_msg: raise DbAccessError(self.db_path, error_msg) error_msg = """ Your ESX installation seems to be using configuration DB created by previous version of vSphere Docker Volume Service, and requires upgrade. See {0} for more information. (_DEFAULT_UUID = {1}, expected = {2}) - """.format(UPGRADE_README, tenant.id, auth.DEFAULT_TENANT_UUID) + """.format(UPGRADE_README, tenant.id, auth_data_const.DEFAULT_TENANT_UUID) logging.error(error_msg) raise DbAccessError(self.db_path, error_msg) @@ -710,8 +710,8 @@ def create_tenant(self, name, description, vms, privileges): error_msg = "Not all columns are set in privileges" return error_msg, None - if name == auth.DEFAULT_DS: - tenant_uuid = auth.DEFAULT_TENANT_UUID + if name == auth_data_const.DEFAULT_DS: + tenant_uuid = auth_data_const.DEFAULT_TENANT_UUID else: tenant_uuid = None # Create the entry in the tenants table diff --git a/esx_service/utils/auth_data_const.py b/esx_service/utils/auth_data_const.py index 75b593ac1..db127a952 100644 --- a/esx_service/utils/auth_data_const.py +++ b/esx_service/utils/auth_data_const.py @@ -34,3 +34,9 @@ # column name in volume table COL_VOLUME_NAME = 'volume_name' COL_VOLUME_SIZE = 'volume_size' + +# default tenant constants +DEFAULT_TENANT = '_DEFAULT' +DEFAULT_TENANT_UUID = '11111111-1111-1111-1111-111111111111' +DEFAULT_DS = '_DEFAULT' +DEFAULT_DS_URL = DEFAULT_DS + "_URL" diff --git a/esx_service/utils/vmdk_utils.py b/esx_service/utils/vmdk_utils.py index 38611ef46..64f1dd63e 100644 --- a/esx_service/utils/vmdk_utils.py +++ b/esx_service/utils/vmdk_utils.py @@ -23,6 +23,7 @@ import fnmatch import vmdk_ops import auth +import auth_data_const import auth_api import log_config import auth @@ -316,8 +317,8 @@ def get_datastore_url(datastore_name): """ return datastore url for given datastore name """ # Return default datastore URL for default datastore name - if datastore_name == auth.DEFAULT_DS: - return auth.DEFAULT_DS_URL + if datastore_name == auth_data_const.DEFAULT_DS: + return auth_data_const.DEFAULT_DS_URL # validate_datastore will refresh the cache if datastore_name is not in cache if not validate_datastore(datastore_name): @@ -333,8 +334,8 @@ def get_datastore_name(datastore_url): """ return datastore name for given datastore url """ # Return default datastore name for default datastore URL - if datastore_url == auth.DEFAULT_DS_URL: - return auth.DEFAULT_DS + if datastore_url == auth_data_const.DEFAULT_DS_URL: + return auth_data_const.DEFAULT_DS # Query datastore name from VIM API # get_datastores() return a list of tuple diff --git a/esx_service/vmdk_ops.py b/esx_service/vmdk_ops.py index 9cbd3c1f8..1a0d456f0 100755 --- a/esx_service/vmdk_ops.py +++ b/esx_service/vmdk_ops.py @@ -85,6 +85,7 @@ import sqlite3 import convert import error_code +import auth_data_const import auth_api from error_code import ErrorCode import re @@ -780,7 +781,7 @@ def executeRequest(vm_uuid, vm_name, config_path, cmd, full_vol_name, opts): error_info, tenant_uuid, tenant_name = auth.get_tenant(vm_uuid) if error_info: return err(error_info) - if tenant_name == auth.DEFAULT_TENANT: + if tenant_name == auth_data_const.DEFAULT_TENANT: # for DEFAULT tenant, set default_datastore to vm_datastore default_datastore_url = vm_datastore_url default_datastore = vm_datastore diff --git a/esx_service/vmdk_ops_test.py b/esx_service/vmdk_ops_test.py index 5977a8e31..bb579f6a4 100644 --- a/esx_service/vmdk_ops_test.py +++ b/esx_service/vmdk_ops_test.py @@ -36,6 +36,7 @@ import auth import auth_data import auth_api +import auth_data_const import error_code import vmdk_utils import random @@ -267,7 +268,7 @@ def setUp(self): self.vm_datastore = datastore[0] self.vm_datastore_url = datastore[1] - path, err = vmdk_ops.get_vol_path(self.vm_datastore, auth.DEFAULT_TENANT) + path, err = vmdk_ops.get_vol_path(self.vm_datastore, auth_data_const.DEFAULT_TENANT) self.assertEqual(err, None, err) self.name = vmdk_utils.get_vmdk_path(path, self.volName) @@ -714,18 +715,18 @@ def create_default_tenant_and_privileges(self): logging.debug("create_default_tenant_and_privileges: create DEFAULT tenant") error_info, auth_mgr = auth_api.get_auth_mgr_object() if error_info: - err = error_code.TENANT_CREATE_FAILED.format(auth.DEFAULT_TENANT, error_info.msg) + err = error_code.TENANT_CREATE_FAILED.format(auth_data_const.DEFAULT_TENANT, error_info.msg) logging.warning(err) self.assertEqual(error_info, None) error_msg, tenant = auth_mgr.create_tenant( - name=auth.DEFAULT_TENANT, + name=auth_data_const.DEFAULT_TENANT, description="This is a default tenant", vms=[], privileges=[]) if error_msg: - err = error_code.TENANT_CREATE_FAILED.format(auth.DEFAULT_TENANT, error_msg) + err = error_code.TENANT_CREATE_FAILED.format(auth_data_const.DEFAULT_TENANT, error_msg) logging.warning(err) self.assertEqual(error_msg, None) @@ -737,8 +738,8 @@ def create_default_tenant_and_privileges(self): if not privileges: logging.debug("create_default_tenant_and_privileges: create DEFAULT privileges") - error_info = auth_api._tenant_access_add(name=auth.DEFAULT_TENANT, - datastore=auth.DEFAULT_DS, + error_info = auth_api._tenant_access_add(name=auth_data_const.DEFAULT_TENANT, + datastore=auth_data_const.DEFAULT_DS, allow_create=True, default_datastore=False, volume_maxsize_in_MB=0, @@ -827,7 +828,7 @@ def cleanup(self): # cleanup existing volume under DEFAULT tenant logging.info("VMDKTenantTest cleanup") if self.datastore_path: - default_tenant_path = os.path.join(self.datastore_path, auth.DEFAULT_TENANT) + default_tenant_path = os.path.join(self.datastore_path, auth_data_const.DEFAULT_TENANT) for vol in self.default_tenant_vols: vmdk_path = vmdk_utils.get_vmdk_path(default_tenant_path, vol) response = vmdk_ops.getVMDK(vmdk_path, vol, self.datastore_name) @@ -885,7 +886,7 @@ def test_vmdkops_on_default_tenant_vm(self): self.assertEqual(None, error_info) # remove DEFAULT privileges, and run create command, which should fail - error_info = auth_api._tenant_access_rm(auth.DEFAULT_TENANT, auth.DEFAULT_DS) + error_info = auth_api._tenant_access_rm(auth_data_const.DEFAULT_TENANT, auth_data_const.DEFAULT_DS) self.assertEqual(None, error_info) opts={u'size': u'100MB', u'fstype': u'ext4'} @@ -893,7 +894,7 @@ def test_vmdkops_on_default_tenant_vm(self): self.assertNotEqual(None, error_info) # remove DEFAULT tenant, and run create command, which should fail - error_info = auth_api._tenant_rm(auth.DEFAULT_TENANT, False) + error_info = auth_api._tenant_rm(auth_data_const.DEFAULT_TENANT, False) self.assertEqual(None, error_info) opts={u'size': u'100MB', u'fstype': u'ext4'} @@ -1232,7 +1233,7 @@ def create_default_tenant_and_privileges(self): if not tenant_uuid: logging.debug("create_default_tenant_and_privileges: create DEFAULT tenant") error_info, tenant = auth_api._tenant_create( - name=auth.DEFAULT_TENANT, + name=auth_data_const.DEFAULT_TENANT, description="This is a default tenant", vm_list=[], privileges=[]) @@ -1244,14 +1245,14 @@ def create_default_tenant_and_privileges(self): error_info, privileges = auth.get_default_privileges() if not privileges: logging.debug("create_default_tenant_and_privileges: create DEFAULT privileges") - error_info = auth_api._tenant_access_add(name=auth.DEFAULT_TENANT, - datastore=auth.DEFAULT_DS_URL, + error_info = auth_api._tenant_access_add(name=auth_data_const.DEFAULT_TENANT, + datastore=auth_data_const.DEFAULT_DS_URL, allow_create=True, default_datastore=False, volume_maxsize_in_MB=0, volume_totalsize_in_MB=0) if error_info: - err = error_code.TENANT_SET_ACCESS_PRIVILEGES_FAILED.format(auth.DEFAULT_TENANT, auth.DEFAULT_DS, error_info) + err = error_code.TENANT_SET_ACCESS_PRIVILEGES_FAILED.format(auth_data_const.DEFAULT_TENANT, auth_data_const.DEFAULT_DS, error_info) logging.warning(err) @unittest.skipIf(not vsan_info.get_vsan_datastore(), @@ -1292,7 +1293,7 @@ def cleanup(self): # cleanup existing volume under DEFAULT tenant logging.info("VmdkTenantPolicyUsageTestCase cleanup") if self.datastore_path: - default_tenant_path = os.path.join(self.datastore_path, auth.DEFAULT_TENANT) + default_tenant_path = os.path.join(self.datastore_path, auth_data_const.DEFAULT_TENANT) for vol in self.default_tenant_vols: vmdk_path = vmdk_utils.get_vmdk_path(default_tenant_path, vol) response = vmdk_ops.getVMDK(vmdk_path, vol, self.datastore_name)