Cloudstack 10170: Fix resource tags security bugs and add account tags support (#2350)

This PR introduces several features and fixes some bugs:
- account tags feature
- fixed resource tags bugs which happened during tags search (found wrong entries because of mysql string to number translation - see #905, but this PR does more and fixes also resource access - vulnerability during list resource tags)
- some marvin improvements (speed, sanity)

Improved resource tags code:
1. Enhanced listTags security
2. Added support for account tags (account tags are required to support tags common for all users of an account)
3. Improved the tag management code (refactoring and cleanup)

Marvin:
1. Fixed Marvin wait timeout between async pools. To decrease polling interval and improve CI speed.
2. Fixed /tmp/ to /tmp in zone configuration files.
3. Fixed + to os.path.join in log class.
4. Fixed + to os.path.join in deployDataCenter class.
5. Fixed typos in tag tests.
6. Modified Tags base class delete method.

Deploy Datacenter script:
1. Improved deployDatacenter. Added option logdir to specify where script places results of evaluation.

ConfigurationManagerImpl:
1. Added logging to ConfigurationManagerImpl to log when vlan is not found. Added test stubs for tags. Found accidental exception during simulator running after CI.

tests_tags.py:
1. Fixed stale undeleted tags.
2. Changed region:India to scope:TestName.

api/src/com/cloud/server/ResourceTag.java

@@ -38,6 +38,7 @@ public interface ResourceTag extends ControlledEntity, Identity, InternalIdentit
         SecurityGroupRule(true, false),
         PublicIpAddress(true, true),
         Project(true, false),
+        Account(true, false),
         Vpc(true, true),
         NetworkACL(true, true),
         StaticRoute(true, false),

server/src/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -3804,6 +3804,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
     @DB
     public boolean releasePublicIpRange(final long vlanDbId, final long userId, final Account caller) {
         VlanVO vlan = _vlanDao.findById(vlanDbId);
+        if(vlan == null) {
+            s_logger.warn("VLAN information for Account '" + caller + "', User '" + userId + "' VLAN '" + vlanDbId + "' is null. This is NPE situation.");
+        }
 
         // Verify range is dedicated
         boolean isAccountSpecific = false;

server/src/com/cloud/tags/TaggedResourceManagerImpl.java

@@ -16,28 +16,13 @@
 // under the License.
 package com.cloud.tags;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-import javax.inject.Inject;
-import javax.naming.ConfigurationException;
-
-import org.apache.cloudstack.api.Identity;
-import org.apache.cloudstack.api.InternalIdentity;
-import org.apache.cloudstack.context.CallContext;
-import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
-import org.apache.commons.lang.StringUtils;
-import org.apache.log4j.Logger;
-
-import com.cloud.api.query.dao.ResourceTagJoinDao;
 import com.cloud.dc.DataCenterVO;
 import com.cloud.domain.PartOf;
 import com.cloud.event.ActionEvent;
 import com.cloud.event.EventTypes;
 import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.exception.PermissionDeniedException;
+import com.cloud.offerings.NetworkOfferingVO;
 import com.cloud.network.LBHealthCheckPolicyVO;
 import com.cloud.network.as.AutoScaleVmGroupVO;
 import com.cloud.network.as.AutoScaleVmProfileVO;
@@ -58,7 +43,6 @@ import com.cloud.network.vpc.NetworkACLVO;
 import com.cloud.network.vpc.StaticRouteVO;
 import com.cloud.network.vpc.VpcOfferingVO;
 import com.cloud.network.vpc.VpcVO;
-import com.cloud.offerings.NetworkOfferingVO;
 import com.cloud.projects.ProjectVO;
 import com.cloud.server.ResourceTag;
 import com.cloud.server.ResourceTag.ResourceObjectType;
@@ -72,6 +56,7 @@ import com.cloud.storage.VolumeVO;
 import com.cloud.tags.dao.ResourceTagDao;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
+import com.cloud.user.AccountVO;
 import com.cloud.user.DomainManager;
 import com.cloud.user.OwnedBy;
 import com.cloud.user.UserVO;
@@ -89,11 +74,28 @@ import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.NicVO;
 import com.cloud.vm.UserVmVO;
 import com.cloud.vm.snapshot.VMSnapshotVO;
+import org.apache.cloudstack.api.Identity;
+import org.apache.cloudstack.api.InternalIdentity;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.commons.lang.StringUtils;
+import org.apache.log4j.Logger;
+
+import org.apache.commons.collections.MapUtils;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
 
 public class TaggedResourceManagerImpl extends ManagerBase implements TaggedResourceService {
     public static final Logger s_logger = Logger.getLogger(TaggedResourceManagerImpl.class);
 
-    private static final Map<ResourceObjectType, Class<?>> s_typeMap = new HashMap<ResourceObjectType, Class<?>>();
+    private static final Map<ResourceObjectType, Class<?>> s_typeMap = new HashMap<>();
     static {
         s_typeMap.put(ResourceObjectType.UserVm, UserVmVO.class);
         s_typeMap.put(ResourceObjectType.Volume, VolumeVO.class);
@@ -108,6 +110,7 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
         s_typeMap.put(ResourceObjectType.SecurityGroupRule, SecurityGroupRuleVO.class);
         s_typeMap.put(ResourceObjectType.PublicIpAddress, IPAddressVO.class);
         s_typeMap.put(ResourceObjectType.Project, ProjectVO.class);
+        s_typeMap.put(ResourceObjectType.Account, AccountVO.class);
         s_typeMap.put(ResourceObjectType.Vpc, VpcVO.class);
         s_typeMap.put(ResourceObjectType.Nic, NicVO.class);
         s_typeMap.put(ResourceObjectType.NetworkACL, NetworkACLItemVO.class);
@@ -140,8 +143,6 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
     @Inject
     ResourceTagDao _resourceTagDao;
     @Inject
-    ResourceTagJoinDao _resourceTagJoinDao;
-    @Inject
     DomainManager _domainMgr;
     @Inject
     AccountDao _accountDao;
@@ -194,6 +195,12 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
             domainId = ((SecurityGroupVO)SecurityGroup).getDomainId();
         }
 
+        if (resourceType == ResourceObjectType.Account) {
+            AccountVO account = (AccountVO)entity;
+            accountId = account.getId();
+            domainId = account.getDomainId();
+        }
+
         // if the resource type is network acl, get the accountId and domainId from VPC following: NetworkACLItem -> NetworkACL -> VPC
         if (resourceType == ResourceObjectType.NetworkACL) {
             NetworkACLItemVO aclItem = (NetworkACLItemVO)entity;
@@ -223,7 +230,23 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
         if ((domainId == null) || ((accountId != null) && (domainId.longValue() == -1))) {
             domainId = _accountDao.getDomainIdForGivenAccountId(accountId);
         }
-        return new Pair<Long, Long>(accountId, domainId);
+        return new Pair<>(accountId, domainId);
+    }
+
+    private void checkResourceAccessible(Long accountId, Long domainId, String exceptionMessage) {
+        Account caller = CallContext.current().getCallingAccount();
+        if (Objects.equals(domainId, -1))
+        {
+            throw new CloudRuntimeException("Invalid DomainId: -1");
+        }
+        if (accountId != null) {
+            _accountMgr.checkAccess(caller, null, false, _accountMgr.getAccount(accountId));
+        } else if (domainId != null && !_accountMgr.isNormalUser(caller.getId())) {
+            //check permissions;
+            _accountMgr.checkAccess(caller, _domainMgr.getDomain(domainId));
+        } else {
+            throw new PermissionDeniedException(exceptionMessage);
+        }
     }
 
     @Override
@@ -237,59 +260,6 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
         throw new InvalidParameterValueException("Invalid resource type " + resourceTypeStr);
     }
 
-    @Override
-    @DB
-    @ActionEvent(eventType = EventTypes.EVENT_TAGS_CREATE, eventDescription = "creating resource tags")
-    public List<ResourceTag> createTags(final List<String> resourceIds, final ResourceObjectType resourceType, final Map<String, String> tags, final String customer) {
-        final Account caller = CallContext.current().getCallingAccount();
-
-        final List<ResourceTag> resourceTags = new ArrayList<ResourceTag>(tags.size());
-
-        Transaction.execute(new TransactionCallbackNoReturn() {
-            @Override
-            public void doInTransactionWithoutResult(TransactionStatus status) {
-                for (String key : tags.keySet()) {
-                    for (String resourceId : resourceIds) {
-                        if (!resourceType.resourceTagsSupport()) {
-                            throw new InvalidParameterValueException("The resource type " + resourceType + " doesn't support resource tags");
-                        }
-
-                        long id = getResourceId(resourceId, resourceType);
-                        String resourceUuid = getUuid(resourceId, resourceType);
-
-                        Pair<Long, Long> accountDomainPair = getAccountDomain(id, resourceType);
-                        Long domainId = accountDomainPair.second();
-                        Long accountId = accountDomainPair.first();
-
-                        if ((domainId != null) && (domainId == -1)) {
-                            throw new CloudRuntimeException("Invalid DomainId : -1");
-                        }
-                        if (accountId != null) {
-                            _accountMgr.checkAccess(caller, null, false, _accountMgr.getAccount(accountId));
-                        } else if (domainId != null && !_accountMgr.isNormalUser(caller.getId())) {
-                            //check permissions;
-                            _accountMgr.checkAccess(caller, _domainMgr.getDomain(domainId));
-                        } else {
-                            throw new PermissionDeniedException("Account " + caller + " doesn't have permissions to create tags" + " for resource " + key);
-                        }
-
-                        String value = tags.get(key);
-
-                        if (value == null || value.isEmpty()) {
-                            throw new InvalidParameterValueException("Value for the key " + key + " is either null or empty");
-                        }
-
-                        ResourceTagVO resourceTag = new ResourceTagVO(key, value, accountDomainPair.first(), accountDomainPair.second(), id, resourceType, customer, resourceUuid);
-                        resourceTag = _resourceTagDao.persist(resourceTag);
-                        resourceTags.add(resourceTag);
-                    }
-                }
-            }
-        });
-
-        return resourceTags;
-    }
-
     @Override
     public String getUuid(String resourceId, ResourceObjectType resourceType) {
         if (!StringUtils.isNumeric(resourceId)) {
@@ -308,65 +278,119 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
 
     @Override
     @DB
-    @ActionEvent(eventType = EventTypes.EVENT_TAGS_DELETE, eventDescription = "deleting resource tags")
+    @ActionEvent(eventType = EventTypes.EVENT_TAGS_CREATE, eventDescription = "creating resource tags")
-    public boolean deleteTags(List<String> resourceIds, ResourceObjectType resourceType, Map<String, String> tags) {
+    public List<ResourceTag> createTags(final List<String> resourceIds, final ResourceObjectType resourceType, final Map<String, String> tags, final String customer) {
-        Account caller = CallContext.current().getCallingAccount();
+        final Account caller = CallContext.current().getCallingAccount();
 
+        final List<ResourceTag> resourceTags = new ArrayList<>(tags.size());
+
+        Transaction.execute(new TransactionCallbackNoReturn() {
+            @Override
+            public void doInTransactionWithoutResult(TransactionStatus status) {
+                for (String key : tags.keySet()) {
+                    for (String resourceId : resourceIds) {
+                        if (!resourceType.resourceTagsSupport()) {
+                            throw new InvalidParameterValueException("The resource type " + resourceType + " doesn't support resource tags");
+                        }
+
+                        long id = getResourceId(resourceId, resourceType);
+                        String resourceUuid = getUuid(resourceId, resourceType);
+
+                        Pair<Long, Long> accountDomainPair = getAccountDomain(id, resourceType);
+                        Long domainId = accountDomainPair.second();
+                        Long accountId = accountDomainPair.first();
+
+                        checkResourceAccessible(accountId, domainId, "Account '" + caller +
+                                "' doesn't have permissions to create tags" + " for resource '" + id + "(" + key + ")'.");
+
+                        String value = tags.get(key);
+
+                        if (value == null || value.isEmpty()) {
+                            throw new InvalidParameterValueException("Value for the key " + key + " is either null or empty");
+                        }
+
+                        ResourceTagVO resourceTag = new ResourceTagVO(key, value, accountDomainPair.first(), accountDomainPair.second(), id, resourceType, customer, resourceUuid);
+                        resourceTag = _resourceTagDao.persist(resourceTag);
+                        resourceTags.add(resourceTag);
+                    }
+                }
+            }
+        });
+
+        return resourceTags;
+    }
+
+    private List<? extends ResourceTag> searchResourceTags(List<String> resourceIds, ResourceObjectType resourceType) {
+        List<String> resourceUuids = resourceIds.stream().map(resourceId -> getUuid(resourceId, resourceType)).collect(Collectors.toList());
         SearchBuilder<ResourceTagVO> sb = _resourceTagDao.createSearchBuilder();
-        sb.and().op("resourceId", sb.entity().getResourceId(), SearchCriteria.Op.IN);
+        sb.and("resourceUuid", sb.entity().getResourceUuid(), SearchCriteria.Op.IN);
-        sb.or("resourceUuid", sb.entity().getResourceUuid(), SearchCriteria.Op.IN);
-        sb.cp();
         sb.and("resourceType", sb.entity().getResourceType(), SearchCriteria.Op.EQ);
 
         SearchCriteria<ResourceTagVO> sc = sb.create();
-        sc.setParameters("resourceId", resourceIds.toArray());
+        sc.setParameters("resourceUuid", resourceUuids.toArray());
-        sc.setParameters("resourceUuid", resourceIds.toArray());
         sc.setParameters("resourceType", resourceType);
+        return _resourceTagDao.search(sc, null);
+    }
 
-        List<? extends ResourceTag> resourceTags = _resourceTagDao.search(sc, null);
+    @Override
-        ;
+    @DB
-        final List<ResourceTag> tagsToRemove = new ArrayList<ResourceTag>();
+    @ActionEvent(eventType = EventTypes.EVENT_TAGS_DELETE, eventDescription = "deleting resource tags")
+    public boolean deleteTags(List<String> resourceIds, ResourceObjectType resourceType, Map<String, String> tags) {
+        Account caller = CallContext.current().getCallingAccount();
+        if(s_logger.isDebugEnabled()) {
+            s_logger.debug("ResourceIds to Find " + String.join(", ", resourceIds));
+        }
+        List<? extends ResourceTag> resourceTags = searchResourceTags(resourceIds, resourceType);
+        final List<ResourceTag> tagsToDelete = new ArrayList<>();
 
         // Finalize which tags should be removed
         for (ResourceTag resourceTag : resourceTags) {
             //1) validate the permissions
+            if(s_logger.isDebugEnabled()) {
+                s_logger.debug("Resource Tag Id: " + resourceTag.getResourceId());
+                s_logger.debug("Resource Tag AccountId: " + resourceTag.getAccountId());
+            }
             Account owner = _accountMgr.getAccount(resourceTag.getAccountId());
+            if(s_logger.isDebugEnabled()) {
+                s_logger.debug("Resource Owner: " + owner);
+            }
             _accountMgr.checkAccess(caller, null, false, owner);
             //2) Only remove tag if it matches key value pairs
-            if (tags != null && !tags.isEmpty()) {
+            if (MapUtils.isEmpty(tags)) {
+                tagsToDelete.add(resourceTag);
+            } else {
                 for (String key : tags.keySet()) {
-                    boolean canBeRemoved = false;
+                    boolean deleteTag = false;
                     if (resourceTag.getKey().equalsIgnoreCase(key)) {
                         String value = tags.get(key);
                         if (value != null) {
                             if (resourceTag.getValue().equalsIgnoreCase(value)) {
-                                canBeRemoved = true;
+                                deleteTag = true;
                             }
                         } else {
-                            canBeRemoved = true;
+                            deleteTag = true;
                         }
-                        if (canBeRemoved) {
+                        if (deleteTag) {
-                            tagsToRemove.add(resourceTag);
+                            tagsToDelete.add(resourceTag);
                             break;
                         }
                     }
                 }
-            } else {
-                tagsToRemove.add(resourceTag);
             }
         }
 
-        if (tagsToRemove.isEmpty()) {
+        if (tagsToDelete.isEmpty()) {
-            throw new InvalidParameterValueException("Unable to find tags by parameters specified");
+            throw new InvalidParameterValueException("Unable to find any tags which conform to specified delete parameters.");
         }
 
         //Remove the tags
         Transaction.execute(new TransactionCallbackNoReturn() {
             @Override
             public void doInTransactionWithoutResult(TransactionStatus status) {
-                for (ResourceTag tagToRemove : tagsToRemove) {
+                for (ResourceTag tagToRemove : tagsToDelete) {
                     _resourceTagDao.remove(tagToRemove.getId());
-                    s_logger.debug("Removed the tag " + tagToRemove);
+                    s_logger.debug("Removed the tag '" + tagToRemove + "' for resources (" +
+                            String.join(", ", resourceIds) + ")");
                 }
             }
         });
@@ -375,7 +399,7 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
     }
 
     @Override
-    public List<? extends ResourceTag> listByResourceTypeAndId(ResourceObjectType type, long resourceId) {
+    public List<? extends ResourceTag> listByResourceTypeAndId(ResourceObjectType resourceType, long resourceId) {
-        return _resourceTagDao.listBy(resourceId, type);
+        return _resourceTagDao.listBy(resourceId, resourceType);
     }
 }

setup/dev/advanced.cfg

@@ -154,7 +154,7 @@
     },
     "logger":
         {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
         },
     "globalConfig": [
         {

setup/dev/advancedsg.cfg

@@ -109,7 +109,7 @@
     },
     "logger": 
         {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
         },
     "globalConfig": [
         {

setup/dev/basic.cfg

@@ -110,7 +110,7 @@
     },
     "logger": 
         {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
         },
     "globalConfig": [
         {

setup/dev/local.cfg

@@ -25,7 +25,7 @@
     },
     "logger":
     {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
     },
     "mgtSvr": [
         {

setup/dev/s3.cfg

@@ -115,7 +115,7 @@
     ],
     "logger":
     {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
     },
     "mgtSvr": [
         {

tools/marvin/marvin/cloudstackConnection.py

@@ -103,8 +103,8 @@ class CSConnection(object):
                     elif job_status == JOB_FAILED:
                         raise Exception("Job failed: %s"\
                                          % async_response)
-                time.sleep(5)
+                time.sleep(1)
-                timeout -= 5
+                timeout -= 1
                 self.logger.debug("=== JobId:%s is Still Processing, "
                                   "Will TimeOut in:%s ====" % (str(jobid),
                                                                str(timeout)))

tools/marvin/marvin/deployDataCenter.py

@@ -35,6 +35,7 @@ from marvin.lib.utils import (random_gen)
 from marvin.config.test_data import test_data
 from sys import exit
 import os
+import errno
 import pickle
 from time import sleep, strftime, localtime
 from optparse import OptionParser
@@ -65,18 +66,16 @@ class DeployDataCenters(object):
     def __persistDcConfig(self):
         try:
             if self.__logFolderPath:
-                dc_file_path = self.__logFolderPath + "/dc_entries.obj"
+                dc_file_path = os.path.join(self.__logFolderPath, "dc_entries.obj")
             else:
                 ts = strftime("%b_%d_%Y_%H_%M_%S", localtime())
                 dc_file_path = "dc_entries_" + str(ts) + ".obj"
+
             file_to_write = open(dc_file_path, 'w')
             if file_to_write:
                 pickle.dump(self.__cleanUp, file_to_write)
-                print "\n=== Data Center Settings are dumped to %s===" % \
+                print "\n=== Data Center Settings are dumped to %s===" % dc_file_path
-                      dc_file_path
+                self.__tcRunLogger.debug("\n=== Data Center Settings are dumped to %s===" % dc_file_path)
-                self.__tcRunLogger.debug(
-                    "\n=== Data Center Settings are dumped to %s===" %
-                    dc_file_path)
         except Exception as e:
             print "Exception Occurred  while persisting DC Settings: %s" % \
                   GetDetailExceptionInfo(e)
@@ -1110,38 +1109,46 @@ class DeleteDataCenters:
         finally:
             return ret
 
+def mkdirpath(path):
+    try:
+        os.makedirs(path)
+    except OSError as exc:
+        if exc.errno == errno.EEXIST and os.path.isdir(path):
+            pass
+        else:
+            raise
 
 if __name__ == "__main__":
     '''
     @Desc : This module facilitates the following:
             1. Deploying DataCenter by using the input provided
             configuration.
-              EX: python deployDataCenter.py -i <inp-cfg-file>
+              EX: python deployDataCenter.py -i <inp-cfg-file> [-l <directory with logs and output data location>]
             2. Removes a created DataCenter by providing
             the input configuration file and data center settings file
               EX: python deployDataCenter.py -i <inp-cfg-file>
-              -r <dc_exported_entries>
+              -r <dc_exported_entries> [-l <directory with logs and output data location>] 
     '''
     parser = OptionParser()
-    parser.add_option("-i", "--input", action="store",
+    parser.add_option("-i", "--input", action="store", default=None, dest="input",
-                      default=None, dest="input",
+                      help="The path where the json zones config file is located.")
-                      help="the path \
+
-                      where the json config file generated")
+    parser.add_option("-r", "--remove", action="store", default=None, dest="remove",
+                      help="The path to file where the created dc entries are kept.")
+
+    parser.add_option("-l", "--logdir", action="store", default=None, dest="logdir",
+                      help="The directory where result logs of running the script are stored:"
+                      "[dc_entries.obj, failed_plus_exceptions.txt, runinfo.txt]." +
+                      "Created automatically if doesn't exists.")
 
-    parser.add_option("-r", "--remove", action="store",
-                      default=None, dest="remove",
-                      help="path to file\
-                      where the created dc entries are kept")
     (options, args) = parser.parse_args()
 
     '''
     Verify the input validity
     '''
     if options.input is None and options.remove is None:
-        print "\n==== For DeployDataCenter: Please Specify a " \
+        print "\n==== For DeployDataCenter: Please Specify a valid Input Configuration File===="
-              "Valid Input Configuration File===="
+        print "\n==== For DeleteDataCenters: Please Specify a valid Input Configuration File and DC Settings===="
-        print "\n==== For DeleteDataCenters: Please Specify a " \
-              "Valid Input Configuration File and DC Settings===="
         exit(1)
 
     '''
@@ -1161,8 +1168,10 @@ if __name__ == "__main__":
     cfg = configGenerator.getSetupConfig(options.input)
     log = cfg.logger
 
-    ret = log_obj.createLogs("DeployDataCenter",
+    if options.logdir != None:
-                             log)
+        mkdirpath(options.logdir)
+
+    ret = log_obj.createLogs("DeployDataCenter", log, options.logdir, options.logdir == None)
     if ret != FAILED:
         log_folder_path = log_obj.getLogFolderPath()
         tc_run_logger = log_obj.getLogger()

tools/marvin/marvin/lib/base.py

@@ -4304,7 +4304,8 @@ class Tag:
             })
         return Tag(apiclient.createTags(cmd).__dict__)
 
-    def delete(self, apiclient, resourceIds, resourceType, tags):
+    @classmethod
+    def delete(cls, apiclient, resourceIds, resourceType, tags):
         """Delete tags"""
 
         cmd = deleteTags.deleteTagsCmd()

tools/marvin/marvin/marvinLog.py

@@ -128,7 +128,7 @@ class MarvinLog:
     def createLogs(self,
                    test_module_name=None,
                    log_cfg=None,
-                   user_provided_logpath=None):
+                   user_provided_logpath=None, use_temp_path=True):
         '''
         @Name : createLogs
         @Desc : Gets the Logger with file paths initialized and created
@@ -140,29 +140,34 @@ class MarvinLog:
                                        If user provided log path
                                        is available, then one in cfg
                                        will  not be picked up.
+                 use_temp_path: Boolean value which specifies either logs will
+                                       be prepended by random path or not.
         @Output : SUCCESS\FAILED
         '''
         try:
-            temp_ts = time.strftime("%b_%d_%Y_%H_%M_%S",
+            temp_ts = time.strftime("%b_%d_%Y_%H_%M_%S", time.localtime())
-                                    time.localtime())
+
             if test_module_name is None:
                 temp_path = temp_ts + "_" + random_gen()
             else:
-                temp_path = str(test_module_name) + \
+                temp_path = str(test_module_name) + "__" + str(temp_ts) + "_" + random_gen()
-                    "__" + str(temp_ts) + "_" + random_gen()
 
             if user_provided_logpath:
-                temp_dir = user_provided_logpath + "/MarvinLogs"
+                temp_dir = os.path.join(user_provided_logpath, "MarvinLogs")
             elif ((log_cfg is not None) and
                     ('LogFolderPath' in log_cfg.__dict__.keys()) and
                     (log_cfg.__dict__.get('LogFolderPath') is not None)):
-                temp_dir = \
+                temp_dir = os.path.join(log_cfg.__dict__.get('LogFolderPath'), "MarvinLogs")
-                    log_cfg.__dict__.get('LogFolderPath') + "/MarvinLogs"
 
-            self.__logFolderDir = temp_dir + "//" + temp_path
+            if use_temp_path == True:
-            print "\n==== Log Folder Path: %s. " \
+                self.__logFolderDir = os.path.join(temp_dir, temp_path)
-                  "All logs will be available here ====" \
+            else:
-                  % str(self.__logFolderDir)
+                if test_module_name == None:
+                    self.__logFolderDir = temp_dir
+                else:
+                    self.__logFolderDir = os.path.join(temp_dir, str(test_module_name))
+
+            print "\n==== Log Folder Path: %s. All logs will be available here ====" % str(self.__logFolderDir)
             os.makedirs(self.__logFolderDir)
 
             '''
@@ -171,9 +176,10 @@ class MarvinLog:
             2. RunLog contains the complete Run Information for Test Run
             3. ResultFile contains the TC result information for Test Run
             '''
-            tc_failed_exception_log = \
+
-                self.__logFolderDir + "/failed_plus_exceptions.txt"
+            tc_failed_exception_log = os.path.join(self.__logFolderDir, "failed_plus_exceptions.txt")
-            tc_run_log = self.__logFolderDir + "/runinfo.txt"
+            tc_run_log = os.path.join(self.__logFolderDir, "runinfo.txt")
+
             if self.__setLogHandler(tc_run_log,
                                     log_level=logging.DEBUG) != FAILED:
                 self.__setLogHandler(tc_failed_exception_log,