CSTACKEX-36 Correct the logger

Author: Gupta, Surya

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

@@ -192,18 +192,17 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
                 s_logger.error("grantAccess : Storage Pool not found for id: " + dataStore.getId());
                 throw new CloudRuntimeException("grantAccess : Storage Pool not found for id: " + dataStore.getId());
             }
-            if (storagePool.getScope() != ScopeType.CLUSTER || storagePool.getScope() != ScopeType.ZONE) {
-                s_logger.error("grantAccess: Only Cluster and ZONE scoped primary storage is supported. Storage Pool: " + storagePool.getName());
-                throw new CloudRuntimeException("grantAccess: Only Cluster and ZONE scoped primary storage is supported. Storage Pool: " + storagePool.getName());
-            }
-
-            VolumeVO volumeVO = volumeDao.findById(dataObject.getId());
-            if(volumeVO == null) {
-                s_logger.error("grantAccess : Cloud Stack Volume not found for id: " + dataObject.getId());
-                throw new CloudRuntimeException("grantAccess : Cloud Stack Volume not found for id: " + dataObject.getId());
+            if (storagePool.getScope() != ScopeType.CLUSTER && storagePool.getScope() != ScopeType.ZONE) {
+                s_logger.error("grantAccess: Only Cluster and ZONE scoped primary storage is supported for storage Pool: " + storagePool.getName());
+                throw new CloudRuntimeException("grantAccess: Only Cluster and ZONE scoped primary storage is supported for Storage Pool: " + storagePool.getName());
             }
 
             if (dataObject.getType() == DataObjectType.VOLUME) {
+                VolumeVO volumeVO = volumeDao.findById(dataObject.getId());
+                if(volumeVO == null) {
+                    s_logger.error("grantAccess : Cloud Stack Volume not found for id: " + dataObject.getId());
+                    throw new CloudRuntimeException("grantAccess : Cloud Stack Volume not found for id: " + dataObject.getId());
+                }
                 grantAccessForVolume(storagePool, volumeVO, host);
             } else {
                 s_logger.error("Invalid DataObjectType (" + dataObject.getType() + ") passed to grantAccess");

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

@@ -181,60 +181,44 @@ public boolean attachCluster(DataStore dataStore, ClusterScope scope) {
             throw new InvalidParameterValueException("attachCluster: dataStore should not be null");
         }
         if (scope == null) {
-            throw new InvalidParameterValueException("attachCluster: clusterScope should not be null");
+            throw new InvalidParameterValueException("attachCluster: scope should not be null");
         }
         List<String> hostsIdentifier = new ArrayList<>();
         StoragePoolVO storagePool = storagePoolDao.findById(dataStore.getId());
         if(storagePool == null) {
             s_logger.error("attachCluster : Storage Pool not found for id: " + dataStore.getId());
             throw new CloudRuntimeException("attachCluster : Storage Pool not found for id: " + dataStore.getId());
         }
-        PrimaryDataStoreInfo primarystore = (PrimaryDataStoreInfo)dataStore;
-        List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInClusterForStorageConnection(primarystore);
+        PrimaryDataStoreInfo primaryStore = (PrimaryDataStoreInfo)dataStore;
+        List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInClusterForStorageConnection(primaryStore);
         // TODO- need to check if no host to connect then throw exception or just continue
-        logger.debug(String.format("Attaching the pool to each of the hosts %s in the cluster: %s", hostsToConnect, primarystore.getClusterId()));
+        logger.debug("attachCluster: Eligible Up and Enabled hosts: {} in cluster {}", hostsToConnect, primaryStore.getClusterId());
 
-        Map<String, String> details = primarystore.getDetails();
+        Map<String, String> details = primaryStore.getDetails();
         StorageStrategy strategy = utils.getStrategyByStoragePoolDetails(details);
         ProtocolType protocol = ProtocolType.valueOf(details.get(Constants.PROTOCOL));
+        //TODO- Check if we have to handle heterogeneous host within the cluster
         if (!isProtocolSupportedByAllHosts(hostsToConnect, protocol, hostsIdentifier)) {
-            throw new CloudRuntimeException("Not all hosts in the cluster support the protocol: " + protocol.toString());
+            s_logger.error("attachCluster: Not all hosts in the cluster support the protocol: " + protocol.name());
+            throw new CloudRuntimeException("attachCluster: Not all hosts in the cluster support the protocol: " + protocol.name());
         }
         //TODO - check if no host to connect then also need to create access group without initiators
         if (hostsIdentifier != null && hostsIdentifier.size() > 0) {
             AccessGroup accessGroupRequest = utils.createAccessGroupRequestByProtocol(storagePool, scope.getScopeId(), details, hostsIdentifier);
             strategy.createAccessGroup(accessGroupRequest);
         }
+        logger.debug("attachCluster: Attaching the pool to each of the host in the cluster: %s", primaryStore.getClusterId());
         for (HostVO host : hostsToConnect) {
             try {
                 _storageMgr.connectHostToSharedPool(host, dataStore.getId());
             } catch (Exception e) {
-                logger.warn("Unable to establish a connection between " + host + " and " + dataStore, e);
+                logger.warn("attachCluster: Unable to establish a connection between " + host + " and " + dataStore, e);
             }
         }
         _dataStoreHelper.attachCluster(dataStore);
         return true;
     }
 
-    private boolean isProtocolSupportedByAllHosts(List<HostVO> hosts, ProtocolType protocolType, List<String> hostIdentifiers) {
-        String protocolPrefix;
-        switch (protocolType) {
-            case ISCSI:
-                protocolPrefix = Constants.IQN;
-                for (HostVO host : hosts) {
-                    if (host == null || host.getStorageUrl() == null || host.getStorageUrl().trim().isEmpty()
-                            || !host.getStorageUrl().startsWith(protocolPrefix)) {
-                        return false;
-                    }
-                    hostIdentifiers.add(host.getStorageUrl());
-                }
-                break;
-            default:
-                throw new CloudRuntimeException("Unsupported protocol: " + protocolType.toString());
-        }
-        return true;
-    }
-
     @Override
     public boolean attachHost(DataStore store, HostScope scope, StoragePoolInfo existingInfo) {
         return false;
@@ -247,30 +231,31 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
             throw new InvalidParameterValueException("attachZone: dataStore should not be null");
         }
         if (scope == null) {
-            throw new InvalidParameterValueException("attachZone: clusterScope should not be null");
+            throw new InvalidParameterValueException("attachZone: scope should not be null");
         }
         List<String> hostsIdentifier = new ArrayList<>();
         StoragePoolVO storagePool = storagePoolDao.findById(dataStore.getId());
         if(storagePool == null) {
-            s_logger.error("attachCluster : Storage Pool not found for id: " + dataStore.getId());
+            s_logger.error("attachZone : Storage Pool not found for id: " + dataStore.getId());
             throw new CloudRuntimeException("attachCluster : Storage Pool not found for id: " + dataStore.getId());
         }
         List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInZoneForStorageConnection(dataStore, scope.getScopeId(), Hypervisor.HypervisorType.KVM);
         // TODO- need to check if no host to connect then throw exception or just continue
-        logger.debug(String.format("In createPool. Attaching the pool to each of the hosts in %s.", hostsToConnect));
+        logger.debug("attachZone: Eligible Up and Enabled hosts: {}", hostsToConnect);
 
         Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(dataStore.getId());
         StorageStrategy strategy = utils.getStrategyByStoragePoolDetails(details);
         ProtocolType protocol = ProtocolType.valueOf(details.get(Constants.PROTOCOL));
+        //TODO- Check if we have to handle heterogeneous host within the zone
         if (!isProtocolSupportedByAllHosts(hostsToConnect, protocol, hostsIdentifier)) {
-            throw new CloudRuntimeException("Not all hosts in the zone support the protocol: " + protocol.toString());
+            s_logger.error("attachZone: Not all hosts in the cluster support the protocol: " + protocol.name());
+            throw new CloudRuntimeException("attachZone: Not all hosts in the zone support the protocol: " + protocol.name());
         }
         if (hostsIdentifier != null && !hostsIdentifier.isEmpty()) {
             AccessGroup accessGroupRequest = utils.createAccessGroupRequestByProtocol(storagePool, scope.getScopeId(), details, hostsIdentifier);
             strategy.createAccessGroup(accessGroupRequest);
         }
         for (HostVO host : hostsToConnect) {
-            // TODO: Fetch the host IQN and add to the initiator group on ONTAP cluster
             try {
                 _storageMgr.connectHostToSharedPool(host, dataStore.getId());
             } catch (Exception e) {
@@ -281,6 +266,24 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
         return true;
     }
 
+    private boolean isProtocolSupportedByAllHosts(List<HostVO> hosts, ProtocolType protocolType, List<String> hostIdentifiers) {
+        switch (protocolType) {
+            case ISCSI:
+                String protocolPrefix = Constants.IQN;
+                for (HostVO host : hosts) {
+                    if (host == null || host.getStorageUrl() == null || host.getStorageUrl().trim().isEmpty()
+                            || !host.getStorageUrl().startsWith(protocolPrefix)) {
+                        return false;
+                    }
+                    hostIdentifiers.add(host.getStorageUrl());
+                }
+                break;
+            default:
+                throw new CloudRuntimeException("isProtocolSupportedByAllHosts : Unsupported protocol: " + protocolType.name());
+        }
+        return true;
+    }
+
     @Override
     public boolean maintain(DataStore store) {
         return true;

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

@@ -123,10 +123,10 @@ public CloudStackVolume getCloudStackVolume(Map<String, String> values) {
 
     @Override
     public AccessGroup createAccessGroup(AccessGroup accessGroup) {
-        s_logger.info("createAccessGroup : Creating Igroup with accessGroup request {} ", accessGroup);
+        s_logger.info("createAccessGroup : Creating Igroup with access group request {} ", accessGroup);
         if (accessGroup == null || accessGroup.getIgroup() == null) {
             s_logger.error("createAccessGroup: Igroup creation failed. Invalid request: {}", accessGroup);
-            throw new CloudRuntimeException("createAccessGroup : Failed to create Lun, invalid request");
+            throw new CloudRuntimeException("createAccessGroup : Failed to create Igroup, invalid request");
         }
         try {
             // Get AuthHeader
@@ -176,7 +176,7 @@ public AccessGroup getAccessGroup(Map<String, String> values) {
             URI url = utils.generateURI(Constants.CREATE_IGROUP);
             url = utils.addQueryParamsToURI(url, values);
             s_logger.info("getAccessGroup: URL: {}", url);
-            //TODO: It is possible that Lun creation will take time and we may need to handle through async job.
+
             OntapResponse<Igroup> igroupResponse = sanFeignClient.getIgroupResponse(url, authHeader);
             if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().size() == 0) {
                 s_logger.error("getAccessGroup: Failed to fetch Igroup");
@@ -213,8 +213,8 @@ public void enableLogicalAccess(Map<String, String> values) {
             URI url = utils.generateURI(Constants.CREATE_LUNMAP);
             OntapResponse<LunMap> createdLunMap = sanFeignClient.createLunMap(url, authHeader, true, lunMapRequest);
             if (createdLunMap == null || createdLunMap.getRecords() == null || createdLunMap.getRecords().size() == 0) {
-                s_logger.error("enableLogicalAccess: LunMap creation failed for Lun {} and igroup ", lunName, igroupName);
-                throw new CloudRuntimeException("Failed to create LunMap: creation failed for Lun: " +lunName+ "and igroup: " + igroupName);
+                s_logger.error("enableLogicalAccess: LunMap failed for Lun {} and igroup ", lunName, igroupName);
+                throw new CloudRuntimeException("Failed to perform LunMap for Lun: " +lunName+ "and igroup: " + igroupName);
             }
             LunMap lunMap = createdLunMap.getRecords().get(0);
             s_logger.debug("enableLogicalAccess: LunMap created successfully. LunMap: {}", lunMap);

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Constants.java

@@ -47,6 +47,7 @@ public class Constants {
 
     public static final String PATH_SEPARATOR = "/";
     public static final String UNDERSCORE = "_";
+    public static final String CS = "cs";
 
     public static final String VOLUME_PATH_PREFIX = "/vol/";
 

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

@@ -171,10 +171,8 @@ public AccessGroup createAccessGroupRequestByProtocol(StoragePoolVO storagePool,
         String svmName = details.get(Constants.SVM_NAME);
         switch (protocol) {
             case ISCSI:
-                String storagePoolName = storagePool.getName();
-                //TODO: Check if we have to change the access group name format
-                // Access group name format: StoragePoolName_ScopeId
-                String igroupName = getIgroupName(storagePoolName, scopeId);
+                // Access group name format: cs_svmName_scopeId
+                String igroupName = getIgroupName(svmName, scopeId);
                 Hypervisor.HypervisorType hypervisorType = storagePool.getHypervisor();
                 return createSANAccessGroupRequest(svmName, igroupName, hypervisorType, hostsIdentifier);
             default:
@@ -193,7 +191,7 @@ public AccessGroup createSANAccessGroupRequest(String svmName, String igroupName
             igroup.setSvm(svm);
         }
 
-        if(igroupName == null || igroupName.isEmpty()) {
+        if(igroupName != null || !igroupName.isEmpty()) {
             igroup.setName(igroupName);
         }
 
@@ -211,7 +209,6 @@ public AccessGroup createSANAccessGroupRequest(String svmName, String igroupName
             }
             igroup.setInitiators(initiators);
         }
-
         return accessGroupRequest;
     }
 
@@ -220,8 +217,8 @@ public String getLunName(String volName, String lunName) {
         return Constants.VOLUME_PATH_PREFIX + volName + Constants.PATH_SEPARATOR + lunName;
     }
 
-    public String getIgroupName(String storagePoolName, long scopeId) {
+    public String getIgroupName(String svmName, long scopeId) {
         // Igroup name format: StoragePoolName_ScopeId
-        return storagePoolName + Constants.UNDERSCORE + scopeId;
+        return Constants.CS + Constants.UNDERSCORE + svmName + Constants.UNDERSCORE + scopeId;
     }
 }