CSTACKEX-36 Resolved Review Comments

Author: Gupta, Surya

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

@@ -225,7 +225,7 @@ private void grantAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO,
             String accessGroupName = utils.getIgroupName(svmName, scopeId);
             CloudStackVolume cloudStackVolume = getCloudStackVolumeByName(storageStrategy, svmName, volumeVO.getPath());
             AccessGroup accessGroup = getAccessGroupByName(storageStrategy, svmName, accessGroupName);
-            if(!accessGroup.getIgroup().getInitiators().contains(host.getStorageUrl())) {
+            if(accessGroup.getIgroup().getInitiators() == null || accessGroup.getIgroup().getInitiators().size() == 0 || !accessGroup.getIgroup().getInitiators().contains(host.getStorageUrl())) {
                 s_logger.error("grantAccess: initiator [{}] is not present in iGroup [{}]", host.getStorageUrl(), accessGroupName);
                 throw new CloudRuntimeException("grantAccess: initiator [" + host.getStorageUrl() + "] is not present in iGroup [" + accessGroupName);
             }
@@ -268,8 +268,8 @@ public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore)
                 }
                 revokeAccessForVolume(storagePool, volumeVO, host);
             } else {
-                s_logger.error("revokeAccess: Invalid DataObjectType (" + dataObject.getType() + ") passed to grantAccess");
-                throw new CloudRuntimeException("Invalid DataObjectType (" + dataObject.getType() + ") passed to grantAccess");
+                s_logger.error("revokeAccess: Invalid DataObjectType (" + dataObject.getType() + ") passed to revokeAccess");
+                throw new CloudRuntimeException("Invalid DataObjectType (" + dataObject.getType() + ") passed to revokeAccess");
             }
         } catch(Exception e){
             s_logger.error("revokeAccess: Failed for dataObject [{}]: {}", dataObject, e.getMessage());
@@ -305,7 +305,7 @@ private CloudStackVolume getCloudStackVolumeByName(StorageStrategy storageStrate
         getCloudStackVolumeMap.put(Constants.NAME, cloudStackVolumeName);
         getCloudStackVolumeMap.put(Constants.SVM_DOT_NAME, svmName);
         CloudStackVolume cloudStackVolume = storageStrategy.getCloudStackVolume(getCloudStackVolumeMap);
-        if(cloudStackVolume ==  null ||cloudStackVolume.getLun() == null || cloudStackVolume.getLun().getName() == null) {
+        if(cloudStackVolume == null || cloudStackVolume.getLun() == null || cloudStackVolume.getLun().getName() == null) {
             s_logger.error("getCloudStackVolumeByName: Failed to get LUN details [{}]", cloudStackVolumeName);
             throw new CloudRuntimeException("getCloudStackVolumeByName: Failed to get LUN [" + cloudStackVolumeName + "]");
         }

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

@@ -207,7 +207,7 @@ public boolean attachCluster(DataStore dataStore, ClusterScope scope) {
             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());
+        logger.debug("attachCluster: Attaching the pool to each of the host in the cluster: {}", primaryStore.getClusterId());
         for (HostVO host : hostsToConnect) {
             try {
                 _storageMgr.connectHostToSharedPool(host, dataStore.getId());
@@ -237,7 +237,7 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
         StoragePoolVO storagePool = storagePoolDao.findById(dataStore.getId());
         if(storagePool == null) {
             s_logger.error("attachZone : Storage Pool not found for id: " + dataStore.getId());
-            throw new CloudRuntimeException("attachCluster : Storage Pool not found for id: " + dataStore.getId());
+            throw new CloudRuntimeException("attachZone : 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

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

@@ -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 failed for Lun {} and igroup ", lunName, igroupName);
-                throw new CloudRuntimeException("Failed to perform LunMap 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/Utility.java

@@ -108,7 +108,7 @@ public CloudStackVolume createSANCloudStackVolumeRequest(String svmName, String
         CloudStackVolume cloudStackVolumeRequest = new CloudStackVolume();
         Lun lunRequest = new Lun();
 
-        if(svmName != null || !svmName.isEmpty()) {
+        if(svmName != null && !svmName.isEmpty()) {
             Svm svm = new Svm();
             svm.setName(svmName);
             lunRequest.setSvm(svm);
@@ -121,7 +121,7 @@ public CloudStackVolume createSANCloudStackVolumeRequest(String svmName, String
         }
 
         //Lun name is full path like in unified "/vol/VolumeName/LunName"
-        if(lunName != null || !lunName.isEmpty()) {
+        if(lunName != null && !lunName.isEmpty()) {
             String lunFullName = getLunName(volName, lunName);
             lunRequest.setName(lunFullName);
         }
@@ -139,7 +139,7 @@ public String getOSTypeFromHypervisor(String hypervisorType){
             case Constants.KVM:
                 return Lun.OsTypeEnum.LINUX.getValue();
             default:
-                String errMsg = "getOStypeFromHypervisorType : Unsupported hypervisor type " + hypervisorType + " for ONTAP storage";
+                String errMsg = "getOSTypeFromHypervisor : Unsupported hypervisor type " + hypervisorType + " for ONTAP storage";
                 s_logger.error(errMsg);
                 throw new CloudRuntimeException(errMsg);
         }
@@ -184,13 +184,13 @@ public AccessGroup createSANAccessGroupRequest(String svmName, String igroupName
         AccessGroup accessGroupRequest = new AccessGroup();
         Igroup igroup = new Igroup();
 
-        if(svmName != null || !svmName.isEmpty()) {
+        if(svmName != null && !svmName.isEmpty()) {
             Svm svm = new Svm();
             svm.setName(svmName);
             igroup.setSvm(svm);
         }
 
-        if(igroupName != null || !igroupName.isEmpty()) {
+        if(igroupName != null && !igroupName.isEmpty()) {
             igroup.setName(igroupName);
         }
 
@@ -208,6 +208,7 @@ public AccessGroup createSANAccessGroupRequest(String svmName, String igroupName
             }
             igroup.setInitiators(initiators);
         }
+        accessGroupRequest.setIgroup(igroup);
         return accessGroupRequest;
     }