Handling tenant vm add, delete, replace corner cases (#992)

Handling corner cases to add,remove, replace vms in a tenant

1. Ensuring VM already in a tenant cannot be added to another tenant through tenant_create, tenant_vm_add and tenant_vm_replace methods
2. Ensuring VM list in tenant_vm_add, tenant_vm_replace, tenant_vm_rm cannot be empty and doesn’t contain duplicates
3. Ensuring existing VM in a tenant cannot be replaced back into same tenant
4. Sequenced parameter processing and error message simplification

Resolves: #930, #994

esx_service/cli/vmdkops_admin_test.py

@@ -28,6 +28,7 @@
 import log_config
 import logging
 import convert
+import error_code
 
 # Number of expected columns in ADMIN_CLI ls
 EXPECTED_COLUMN_COUNT = 12
@@ -639,6 +640,15 @@ def test_tenant_vm(self):
         actual_output = rows
         self.assertEqual(expected_output, actual_output)
 
+        # Trying to create tenant with duplicate vm names
+        error_info, tenant_dup = auth_api._tenant_create(
+                                                    name="tenant_add_dup_vms",
+                                                    description="Tenant with duplicate VMs",
+                                                    vm_list=[self.vm1_name, self.vm1_name],
+                                                    privileges=[])
+
+        self.assertEqual(error_code.ErrorCode.VM_DUPLICATE, error_info.code)
+
         # tenant vm add to add two VMs to the tenant
         error_info = auth_api._tenant_vm_add(
                                              name=self.tenant1_name,
@@ -648,6 +658,39 @@ def test_tenant_vm(self):
         error_info, vms = auth_api._tenant_vm_ls(self.tenant1_name)
         self.assertEqual(None, error_info)
 
+        # create tenant2 with vm1 a part of it. Should fail as VM can be a part
+        # of just one tenant
+        error_info, tenant2 = auth_api._tenant_create(
+                                                    name="Test_tenant2",
+                                                    description="Test_tenant2",
+                                                    vm_list=[self.vm1_name],
+                                                    privileges=[])
+        self.assertEqual(error_code.ErrorCode.VM_IN_ANOTHER_TENANT, error_info.code)
+
+        # create tenant3 and then try to add vm1 to it which is a part of
+        # another tenant. Should fail as VM can be a part of just one tenant
+        error_info, tenant3 = auth_api._tenant_create(
+                                                    name="Test_tenant3",
+                                                    description="Test_tenant3",
+                                                    vm_list=[],
+                                                    privileges=[])
+        self.assertEqual(None, error_info)
+
+        error_info  = auth_api._tenant_vm_add(
+                                              name=tenant3.name,
+                                              vm_list=[self.vm1_name])
+        self.assertEqual(error_code.ErrorCode.VM_IN_ANOTHER_TENANT, error_info.code)
+
+        # Replace should fail since vm1 is already a part of tenant1
+        error_info  = auth_api._tenant_vm_replace(
+                                              name=tenant3.name,
+                                              vm_list=[self.vm1_name])
+        self.assertEqual(error_code.ErrorCode.VM_IN_ANOTHER_TENANT, error_info.code)
+
+        # remove the tenant3
+        error_info = auth_api._tenant_rm(name=tenant3.name)
+        self.assertEqual(None, error_info)
+
         # There are 2 columns for each row, the name of the columns are
         # "Uuid", "Name"
         # Sample output of a row:

esx_service/utils/auth_api.py

@@ -276,25 +276,49 @@ def is_tenant_name_valid(name):
     else:
         return False
 
+def is_vm_duplicate(vm_list):
+    """
+    Check if vm names in vm_list contain duplicates
+    """
+
+    if len(vm_list) != len(set(vm_list)):
+        error_info = error_code.generate_error_info(ErrorCode.VM_DUPLICATE, vm_list)
+        logging.error(error_info.msg)
+        return error_info
+
+    return None
+
 def _tenant_create(name, description="", vm_list=None, privileges=None):
     """ API to create a tenant """
     logging.debug("_tenant_create: name=%s description=%s vm_list=%s privileges=%s", name, description, vm_list, privileges)
     if not is_tenant_name_valid(name):
         error_info = error_code.generate_error_info(ErrorCode.TENANT_NAME_INVALID, name, VALID_TENANT_NAME_REGEXP)
         return error_info, None
 
-    error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list)
-    if error_msg:
-        not_found_vm_list = ",".join(not_found_vms)
-        error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list)
-        return error_info, None
-
     # if param "description" is not set by caller, the default value is empty string
     if not description:
         description = ""
 
-    logging.debug("_tenant_create: vms=%s", vms)
-    vms_uuid_list = [(vm_id) for (vm_id, vm_name) in vms]
+    # VM list can be empty during tenant create. Validate only if it exists
+    vms_uuid_list = []
+    if vm_list:
+        error_info = is_vm_duplicate(vm_list)
+        if error_info:
+            return error_info, None
+
+        error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list)
+        if error_msg:
+            not_found_vm_list = ",".join(not_found_vms)
+            error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list)
+            return error_info, None
+
+        error_info = vm_in_any_tenant(vms)
+        if error_info:
+            return error_info, None
+
+        logging.debug("_tenant_create: vms=%s", vms)
+        vms_uuid_list = [(vm_id) for (vm_id, vm_name) in vms]
+
     error_info, tenant = create_tenant_in_db(
                                              name=name,
                                              description=description,
@@ -384,26 +408,57 @@ def _tenant_ls(name=None):
     error_info, tenant_list = get_tenant_list_from_db(name)
     return error_info, tenant_list
 
-def any_vm_already_exist(existing_vms, vms):
+def vm_already_in_tenant(name, vms):
     """
-        Check whehter any vm in @param "vms" already exist in @param "existing_vms"
+        Check whether any vm in @param "vms" already exists in tenant @param "name"
     """
-    for vm in vms:
-        if vm[0] in existing_vms:
-            return True
+    error_info, existing_vms = _tenant_vm_ls(name)
+    if error_info:
+        return error_info
 
-    return False
+    for vm_id, vm_name in vms:
+        if vm_id in existing_vms:
+            error_info = error_code.generate_error_info(ErrorCode.VM_ALREADY_IN_TENANT,
+                                                        vm_name, name)
+            logging.error(error_info.msg)
+            return error_info
+
+    return None
 
-def any_vm_not_exist(existing_vms, vms):
+def vm_not_exist(name, vms):
     """
-        Check whehter any vm in @param "vms" does not exist in @param "existing_vms"
+        Check whether any vm in @param "vms" does not exist in tenant @param "name"
     """
-    for vm in vms:
-        if not vm[0] in existing_vms:
-            return True
+    error_info, existing_vms = _tenant_vm_ls(name)
+    if error_info:
+        return error_info
+
+    for vm_id, vm_name in vms:
+        if not vm_id in existing_vms:
+            error_info = error_code.generate_error_info(ErrorCode.VM_NOT_IN_TENANT, vm_name, name)
+            logging.error(error_info.msg)
+            return error_info
+
+    return None
 
-    return False
 
+def vm_in_any_tenant(vms):
+    """
+        Check if any vm in @param "vms" is a part of another tenant
+    """
+    error_info, tenant_list = get_tenant_list_from_db()
+    if error_info:
+        return error_info
+
+    for tenant in tenant_list:
+        for vm_id, vm_name in vms:
+            if vm_id in tenant.vms:
+                error_info = error_code.generate_error_info(ErrorCode.VM_IN_ANOTHER_TENANT,
+                                                            vm_name, tenant.name)
+                logging.error(error_info.msg)
+                return error_info
+
+    return None
 
 def _tenant_vm_add(name, vm_list):
     """ API to add vms for a tenant """
@@ -416,18 +471,26 @@ def _tenant_vm_add(name, vm_list):
         error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name)
         return error_info
 
+    if not vm_list:
+        error_info = error_code.generate_error_info(ErrorCode.VM_LIST_EMPTY)
+        return error_info
+
+    error_info = is_vm_duplicate(vm_list)
+    if error_info:
+        return error_info
+
     error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list)
     if error_msg:
         not_found_vm_list = ",".join(not_found_vms)
         error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list)
         return error_info
 
-    error_info, existing_vms = _tenant_vm_ls(name)
+    error_info = vm_already_in_tenant(name, vms)
     if error_info:
         return error_info
 
-    if any_vm_already_exist(existing_vms, vms):
-        error_info = error_code.generate_error_info(ErrorCode.VM_ALREADY_IN_TENANT, vm_list, name)
+    error_info = vm_in_any_tenant(vms)
+    if error_info:
         return error_info
 
     error_info, auth_mgr = get_auth_mgr_object()
@@ -453,6 +516,14 @@ def _tenant_vm_rm(name, vm_list):
         error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name)
         return error_info
 
+    if not vm_list:
+        error_info = error_code.generate_error_info(ErrorCode.VM_LIST_EMPTY)
+        return error_info
+
+    error_info = is_vm_duplicate(vm_list)
+    if error_info:
+        return error_info
+
     error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list)
     if error_msg:
         not_found_vm_list = ",".join(not_found_vms)
@@ -461,14 +532,10 @@ def _tenant_vm_rm(name, vm_list):
 
     logging.debug("_tenant_vm_rm: vms=%s", vms)
 
-    error_info, existing_vms = _tenant_vm_ls(name)
+    error_info = vm_not_exist(name, vms)
     if error_info:
         return error_info
 
-    if any_vm_not_exist(existing_vms, vms):
-        error_info = error_code.generate_error_info(ErrorCode.VM_NOT_IN_TENANT, vm_list, name)
-        return error_info
-
     error_info, auth_mgr = get_auth_mgr_object()
     if error_info:
         return error_info
@@ -496,10 +563,6 @@ def _tenant_vm_ls(name):
 def _tenant_vm_replace(name, vm_list):
     """ API to replace vms for a tenant """
     logging.debug("_tenant_vm_replace: name=%s vm_list=%s", name, vm_list)
-    if not vm_list:
-        error_info = error_code.generate_error_info(ErrorCode.REPLACE_VM_EMPTY)
-        return error_info
-
     error_info, tenant = get_tenant_from_db(name)
     if error_info:
         return error_info
@@ -508,13 +571,29 @@ def _tenant_vm_replace(name, vm_list):
         error_info = error_code.generate_error_info(ErrorCode.TENANT_NOT_EXIST, name)
         return error_info
 
+    if not vm_list:
+        error_info = error_code.generate_error_info(ErrorCode.REPLACE_VM_EMPTY)
+        return error_info
+
+    error_info = is_vm_duplicate(vm_list)
+    if error_info:
+        return error_info
+
     error_msg, vms, not_found_vms = generate_tuple_from_vm_list(vm_list)
 
     if error_msg:
         not_found_vm_list = ",".join(not_found_vms)
         error_info = error_code.generate_error_info(ErrorCode.VM_NOT_FOUND, not_found_vm_list)
         return error_info
 
+    error_info = vm_already_in_tenant(name, vms)
+    if error_info:
+        return error_info
+
+    error_info = vm_in_any_tenant(vms)
+    if error_info:
+        return error_info
+
     logging.debug("_tenant_vm_replace: vms=%s", vms)
     error_info, auth_mgr = get_auth_mgr_object()
     if error_info:

esx_service/utils/error_code.py

@@ -30,6 +30,9 @@ class ErrorCode:
     REPLACE_VM_EMPTY = 102
     VM_ALREADY_IN_TENANT = 103
     VM_NOT_IN_TENANT = 104
+    VM_IN_ANOTHER_TENANT = 105
+    VM_LIST_EMPTY = 106
+    VM_DUPLICATE = 107
     # VM related error code end
 
     # Privilege related error code start
@@ -67,8 +70,11 @@ class ErrorCode:
 
     ErrorCode.VM_NOT_FOUND : "Cannot find vm {0}",
     ErrorCode.REPLACE_VM_EMPTY : "Replace VM cannot be empty",
-    ErrorCode.VM_ALREADY_IN_TENANT : "Some VMs in {0} have already been associated with tenant '{1}', cannot add them again",
-    ErrorCode.VM_NOT_IN_TENANT : "Some VMs in {0} have not been associated with tenant '{1}', cannot remove them",
+    ErrorCode.VM_ALREADY_IN_TENANT : "VM '{0}' has already been associated with tenant '{1}', cannot add it again",
+    ErrorCode.VM_NOT_IN_TENANT : "VM '{0}' has not been associated with tenant '{1}', cannot remove it",
+    ErrorCode.VM_IN_ANOTHER_TENANT : "VM '{0}' has already been associated with tenant '{1}', can't add it",
+    ErrorCode.VM_LIST_EMPTY : "VM list cannot be empty",
+    ErrorCode.VM_DUPLICATE : "VMs {0} contain duplicates, they should be unique",
 
     ErrorCode.PRIVILEGE_NOT_FOUND : "No privilege exists for ({0}, {1})",
     ErrorCode.PRIVILEGE_ALREADY_EXIST : "Privilege for ({0}, {1}) already exists",