Multi-tenancy VMODL implementation #859
Conversation
shaominchen
force-pushed
the
vmodl_multi_tenancy_impl.shchen
branch
from
3d0fddf
to
e543571
Compare
|
@msterin @lipingxue Can you please review this change? You can focus on the VMODL changes. I will review the backend changes done by Liping. |
msterin
force-pushed
the
vmodl_multi_tenancy_impl.shchen
branch
from
e543571
to
712620f
Compare
There was a problem hiding this comment.
@lipingxue The backend changes mostly look straightforward. I just have a few trivial comments.
| @@ -941,14 +980,28 @@ def tenant_access_ls_headers(): | |||
| def generate_tenant_access_ls_rows(privileges): | |||
| """ Generate output for tenant access ls command """ | |||
| rows = [] | |||
| # for p in privileges: | |||
There was a problem hiding this comment.
@lipingxue let's remove these unused code.
| @@ -193,11 +193,20 @@ def test_tenant_access_add_invalid_option_fails(self): | |||
| self.assert_parse_error('tenant access add --name=tenant1 --datastore=datastore1 --rights=create mount') | |||
|
|
|||
| def test_tenant_accss_set(self): | |||
There was a problem hiding this comment.
@lipingxue typo: "accss" => "access"
| from error_code import ErrorCode | ||
| from error_code import ErrorInfo | ||
|
|
||
| def generate_error_info_for_internal_error(secondary_err_msg): |
There was a problem hiding this comment.
@lipingxue Can we consider to introduce a generic API (in error_code.py) to generate error info? This API may look like:
generate_error_info(err_code, err_msg)
There was a problem hiding this comment.
I don't think we need this API. We already has the _init function for class "ErrorInfo" which does the same thing.
There was a problem hiding this comment.
It's not the same thing. If you look at these 3 new utility methods, they are doing the same thing:
- Get the error_code
- Call error_code.error_codes_to_messages() API to get a user-friendly error message
- Contruct an ErrorInfo object and return
My suggestion is to add a new API in error_code to do these common logic so that we don't need to introduce these 3 methods which contains duplicate logic.
There was a problem hiding this comment.
I don't think it will work. The reason is that parameter needed to generate error message from error code is not common for different error_code.
There was a problem hiding this comment.
Python does support variable argument with special syntax *args and **kwargs in function definitions.
| INTERNAL_ERROR = 501 | ||
|
|
||
|
|
||
| error_codes_to_messages = { |
There was a problem hiding this comment.
Nit: suggest to use singular format, i.e. error_code_to_msg()
| # _VERSION = newestVersions.Get("vim") | ||
| _VERSION = 'vim.version.version10' | ||
| _VERSION = newestVersions.Get("vim") | ||
| #_VERSION = "vim.version.version10" |
There was a problem hiding this comment.
remove this unused line.
| @return New tenant for the given name and description | ||
| @throws vmodl.fault.InvalidArgument If the given name or description exceeds maximum length. | ||
| @throws vim.fault.AlreadyExists If the given tenant name already exists. | ||
| @throws vim.fault.VcsFault If an internal server error occurred. |
| @param tenant A tenant instance to be added VMs | ||
| @param vms VMs to be added to this tenant. | ||
| @throws vim.fault.VcsFault If an internal server error occurred. | ||
| """ |
There was a problem hiding this comment.
This function should also throws a fault if tenant does not exist.
There was a problem hiding this comment.
Good catch! Fixed.
| @param vms VMs to be added to this tenant replacing existing VMs. | ||
| @throws vim.fault.VcsFault If an internal server error occurred. |
There was a problem hiding this comment.
Should also throw fault when tenant does not exist.
| error_info = ErrorCode.VMODL_TENANT_NAME_EMPTY.format(name); | ||
| return error_info | ||
|
|
||
| if len(name) > 64: |
There was a problem hiding this comment.
Define a constant for MAX_NAME_LEN, instead of using a hard coded "64"
| error_info = ErrorCode.VMODL_TENANT_NAME_TOO_LONG.format(name); | ||
| return error_info | ||
|
|
||
| if description and len(description) > 256: |
| logging.error("Failed to create tenant: name=%s, description=%s", name, description) | ||
| raise vim.fault.VcsFault(msg=error_info.msg) | ||
|
|
||
| result = vim.vcs.Tenant() |
There was a problem hiding this comment.
Define a function which can construct a vim.vcs.Tenant instance from a tenant instance returned by API layer.
There was a problem hiding this comment.
Please either address this comments or give the reason why you don't want to address it.
|
|
||
| result = [] | ||
| for t in tenant_list: | ||
| tenant = vim.vcs.Tenant() |
|
|
||
| def ReplaceVMs(self, tenant, vms): | ||
| if len(vms) == 0: | ||
| logging.warn("Replace VMs: the VM list is empty") |
There was a problem hiding this comment.
We should return error to user in this case.
There was a problem hiding this comment.
I guess you meant to return error for all 3 APIs: Add VMs, RemoveVMs and ReplaceVMs.
I'm fine with this, will fix.
There was a problem hiding this comment.
I plan to throw InvalidArgument in this case. Let me know if you don't agree with this.
| # the return value is a string like this vm1,vm2 | ||
| res = "" | ||
| for vm in vms: | ||
| # vm[0] is vm_uuid, vm has format (vm_uuid) | ||
| vm_name = vmdk_utils.get_vm_name_by_uuid(vm[0]) | ||
| # vms ia a list of vm_uuid |
| # vm[0] is vm_uuid, vm has format (vm_uuid) | ||
| vm_name = vmdk_utils.get_vm_name_by_uuid(vm[0]) | ||
| # vms ia a list of vm_uuid | ||
| vm_name = vmdk_utils.get_vm_name_by_uuid(vm) |
There was a problem hiding this comment.
Nit - can the "vm" param be named to "vm_uuid" the function takes a VM uuid.
| datastore_url = p[auth_data_const.COL_DATASTORE_URL] | ||
| datastore = vmdk_utils.get_datastore_name(datastore_url) | ||
| allow_create = ("False", "True")[p[auth_data_const.COL_ALLOW_CREATE]] | ||
| datastore_url = p.datastore_url |
There was a problem hiding this comment.
Is datastore_url used anywhere
| """ | ||
| Connect to VCS - currently utilizing VSAN mgmt service on ESX (/vsan) - and return SOAP stub | ||
| """ | ||
|
|
||
| si = vmdk_ops.get_si() | ||
| #pylint: disable=no-member |
There was a problem hiding this comment.
This line is commented out? Do we need it for any reason? If it is, please add some comments for this line.
There was a problem hiding this comment.
No, it's not commented out. This is the standard way to disable a pylint warning: http://stackoverflow.com/questions/4341746/how-do-i-disable-a-pylint-warning
| @@ -78,3 +78,16 @@ class ErrorInfo: | |||
| def __init__(self, code, msg): | |||
| self.code = code | |||
| self.msg = msg | |||
|
|
|||
| def join_args(fmstr, *args): | |||
| #print fmstr | |||
There was a problem hiding this comment.
Let's remove the debugging statements.
| @@ -67,7 +67,7 @@ def get_tenant_from_db(name): | |||
|
|
|||
| error_msg, tenant = auth_mgr.get_tenant(name) | |||
| if error_msg: | |||
| error_info = generate_error_info_for_internal_error(error_msg) | |||
| error_info = error_info.generate_error_info(error_msg) | |||
There was a problem hiding this comment.
I don't think this will compile - it should be error_code.xxx instead of error_info.xxx
There was a problem hiding this comment.
I have fixed this in my change, since this is blocking my build.
|
The back-end changes done by Liping look good to me. @govint Can you please review the VMODL part (esx_services/vmodl/...)? Thanks! |
| @@ -679,17 +689,19 @@ def test_tenant_access(self): | |||
| vm_list=[self.vm1_name], | |||
| privileges=[]) | |||
| self.assertEqual(None, error_info) | |||
|
|
|||
| # add first access privilege for tenant | ||
| # allow_create = False | ||
| # max_volume size = 600MB | ||
| # total_volume size = 1GB | ||
| volume_maxsize_in_MB = convert.convert_to_MB("600MB") | ||
| volume_totalsize_in_MB = convert.convert_to_MB("1GB") |
There was a problem hiding this comment.
Could make these values globals
There was a problem hiding this comment.
Why? This only used in this function.
| @@ -721,11 +733,13 @@ def test_tenant_access(self): | |||
| # change allow_create to True | |||
| # change max_volume size to 1000MB | |||
| # change total_volume size to 2GB | |||
| volume_maxsize_in_MB = convert.convert_to_MB("1000MB") | |||
| volume_totalsize_in_MB = convert.convert_to_MB("3GB") | |||
There was a problem hiding this comment.
Same comment on making test values global or even place all these test values in a separate file thats can be changed by user to configure the test as needed.
There was a problem hiding this comment.
Maybe we can take this as a further enhancement, but I have no plan to make change in this PR.
| err_msg = error_code.error_code_to_message[err_code].format(not_found_vm_list) | ||
| error_info = ErrorInfo(err_code, err_msg) | ||
| return error_info | ||
|
|
There was a problem hiding this comment.
Nit - since these functions are generating the error info could rename so that the action is implicit like vm_not_found(), internal_error(), etc..
| return error_info, tenant | ||
|
|
||
| def get_tenant_name(tenant_uuid): | ||
| """ | ||
| Get tenant name with given tenant_uuid | ||
| Return value: | ||
| -- error_info: return None on success or error info on failure | ||
| -- error_code: return None on success or error code on failure |
There was a problem hiding this comment.
Should this remain error_info as thats returned below.
| err_code = ErrorCode.TENANT_ALREADY_EXIST | ||
| err_msg = error_code.error_code_to_message[err_code].format(name) | ||
| error_info = ErrorInfo(err_code, err_msg) | ||
| return error_info, None |
There was a problem hiding this comment.
Can this be handled by the new error_info generation interface.
| if error_info: | ||
| return error_info, vms, not_found_vms | ||
| if error_msg: | ||
| return error_msg, vms, not_found_vms |
There was a problem hiding this comment.
Just for uniformity can all functions return error_info or error_msg vs. a mix of both.
There was a problem hiding this comment.
Function "generate_tuple_from_vm_list" is not a API, it is only used by other API function in the same file. Only the API function will return error_info. error_info is a struct defined in error_code.py, which includes both field "code" and "msg".
|
|
||
| logging.debug("generate_privileges: privileges=%s", privileges) | ||
| logging.debug("generate_privileges: privileges=%s", privileges) |
| else: | ||
| set_privileges(allow_create, privileges, 0) | ||
| privileges = set_privileges(allow_create, privileges, 0) |
There was a problem hiding this comment.
If allow_create is None then privileges is unset but may be accessed below?
There was a problem hiding this comment.
This function is used to modify an existing privilege. "allow_create" is passed by user to specify if he/she wants to modify this field in the passed-in "privileges". "allow_create" is None just means user does not want to change the "allow_create" filed in the passed-in "privileges".
| size_in_MB = convert.convert_to_MB(volume_totalsize) | ||
| privileges[auth_data_const.COL_USAGE_QUOTA] = size_in_MB | ||
| if volume_totalsize_in_MB: | ||
| privileges[auth_data_const.COL_USAGE_QUOTA] = volume_totalsize_in_MB | ||
|
|
||
| return privileges |
There was a problem hiding this comment.
This can return None depending on the passed in values?
There was a problem hiding this comment.
It will not return None, unless the passed-in "privileges" is None.
| @@ -203,25 +248,27 @@ def get_default_datastore(name): | |||
| return error_info, None | |||
|
|
|||
| if not tenant: | |||
| error_info = error_code.TENANT_NOT_EXIST.format(name) | |||
| error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) | |||
| not_found_vm_list = ",".join(not_found_vms) | ||
| logging.warning(error_code.VM_NOT_FOUND.format(not_found_vm_list)) | ||
| return error_info, None | ||
| error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list) |
| logging.warning(error_code.VM_NOT_FOUND.format(not_found_vm_list)) | ||
| return error_info, None | ||
| error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list) | ||
| return error_info, None |
| @@ -246,24 +293,28 @@ def _tenant_update(name, new_name=None, description=None, default_datastore=None | |||
| return error_info | |||
|
|
|||
| if not tenant: | |||
| error_info = error_code.TENANT_NOT_EXIST.format(name) | |||
| return error_info | |||
| error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) | |||
| error_info = error_code.TENANT_NOT_EXIST.format(name) | ||
| return error_info | ||
| error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) | ||
| return error_info, None |
There was a problem hiding this comment.
Only one return value, other places below only error_info is being returned.
| error_info = error_code.TENANT_NOT_EXIST.format(name) | ||
| return error_info | ||
| error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name) | ||
| return error_info, None |
There was a problem hiding this comment.
Only single value is being returned above and below.
Based on VSAN VMODL extension support and uses vsanmgmtd which needs to be started Also includes backed changes (auth_api, etc...) needed to support the model Note: current Makefile brute-force restarts vsanmgmtd on every start of our daemon. It also assumes vsan service running - i.e. 'esxcli vsan cluster new' is the minimal requirement on 6.0 U2 (in U3 it does not require this command)
running on single node ESX 2. Added tenant CRUD related negative test cases, and fixed some issues detected by these negative tests 3. Fixed an issue in auth_api
Note: VSAN mgmt server may use different certificate depending on the ESXi version and whether it's a single-node or part of VC cluster. Even in a similar deployment, the ESXi certificate may use different SAN: ip, host name or simply "localhost". So to make sure VMODL test client can connect to VSAN mgmt server in all different scenarios, we need to disable certificate check during SSL communication. For testing purpose, we don't really need strong communication safety anyway. Also addressed a few comments from Govindan in this commit.
shaominchen
force-pushed
the
vmodl_multi_tenancy_impl.shchen
branch
from
1f7583d
to
d01c80a
Compare
This PR mainly includes 5 parts: