Skip to content

Commit a9c7f65

Browse files
Locharla, SandeepLocharla, Sandeep
Locharla, Sandeep
authored and
Locharla, Sandeep
committed
CSTACKEX-7: Addressed review comments
1 parent 9547f02 commit a9c7f65

File tree

6 files changed

+93
-87
lines changed

6 files changed

+93
-87
lines changed

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SvmFeignClient.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,17 @@
2626
import org.springframework.web.bind.annotation.RequestHeader;
2727
import org.springframework.web.bind.annotation.RequestMapping;
2828
import org.springframework.web.bind.annotation.RequestMethod;
29+
import org.springframework.web.bind.annotation.RequestParam;
2930

3031
import java.net.URI;
32+
import java.util.Map;
3133

3234
@FeignClient(name = "SvmClient", url = "https://{clusterIP}/api/svm/svms", configuration = FeignConfiguration.class)
3335
public interface SvmFeignClient {
3436

3537
//this method to get all svms and also filtered svms based on query params as a part of URL
3638
@RequestMapping(method = RequestMethod.GET)
37-
OntapResponse<Svm> getSvms(URI baseURL, @RequestHeader("Authorization") String header);
39+
OntapResponse<Svm> getSvmResponse(URI baseURL, @RequestHeader("Authorization") String header, @RequestParam Map<String, String> queryParams);
3840

3941
@RequestMapping(method = RequestMethod.GET, value = "/{uuid}")
4042
Svm getSvmByUUID(URI baseURL, @RequestHeader("Authorization") String header);

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/OntapStorage.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,21 @@
1919

2020
package org.apache.cloudstack.storage.feign.model;
2121

22+
import org.apache.cloudstack.storage.utils.Constants.ProtocolType;
23+
2224
public class OntapStorage {
2325
public static String Username;
2426
public static String Password;
2527
public static String ManagementLIF;
26-
public static String Svm;
27-
public static String Protocol;
28+
public static String SvmName;
29+
public static ProtocolType Protocol;
2830
public static Boolean IsDisaggregated;
2931

30-
public OntapStorage(String username, String password, String managementLIF, String svm, String protocol, Boolean isDisaggregated) {
32+
public OntapStorage(String username, String password, String managementLIF, String svmName, ProtocolType protocol, Boolean isDisaggregated) {
3133
Username = username;
3234
Password = password;
3335
ManagementLIF = managementLIF;
34-
Svm = svm;
36+
SvmName = svmName;
3537
Protocol = protocol;
3638
IsDisaggregated = isDisaggregated;
3739
}
@@ -60,19 +62,19 @@ public void setManagementLIF(String managementLIF) {
6062
ManagementLIF = managementLIF;
6163
}
6264

63-
public String getSVM() {
64-
return Svm;
65+
public String getSvmName() {
66+
return SvmName;
6567
}
6668

67-
public void setSVM(String svm) {
68-
Svm = svm;
69+
public void setSvmName(String svmName) {
70+
SvmName = svmName;
6971
}
7072

71-
public String getProtocol() {
73+
public ProtocolType getProtocol() {
7274
return Protocol;
7375
}
7476

75-
public void setProtocol(String protocol) {
77+
public void setProtocol(ProtocolType protocol) {
7678
Protocol = protocol;
7779
}
7880

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/response/OntapResponse.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ public class OntapResponse<T> {
3434
@JsonProperty("records")
3535
private List<T> records;
3636

37-
public OntapResponse() {
37+
public OntapResponse () {
3838
// Default constructor
3939
}
4040

41-
public OntapResponse(List<T> records) {
41+
public OntapResponse (List<T> records) {
4242
this.records = records;
4343
this.numRecords = (records != null) ? records.size() : 0;
4444
}
@@ -59,4 +59,4 @@ public void setRecords(List<T> records) {
5959
this.records = records;
6060
this.numRecords = (records != null) ? records.size() : 0;
6161
}
62-
}
62+
}

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

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.apache.cloudstack.storage.provider.StorageProviderFactory;
4444
import org.apache.cloudstack.storage.service.StorageStrategy;
4545
import org.apache.cloudstack.storage.utils.Constants;
46+
import org.apache.cloudstack.storage.utils.Constants.ProtocolType;
4647
import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper;
4748
import org.apache.logging.log4j.LogManager;
4849
import org.apache.logging.log4j.Logger;
@@ -70,12 +71,12 @@ public DataStore initialize(Map<String, Object> dsInfos) {
7071
throw new CloudRuntimeException("Datastore info map is null, cannot create primary storage");
7172
}
7273
String url = dsInfos.get("url").toString(); // TODO: Decide on whether should the customer enter just the Management LIF IP or https://ManagementLIF
73-
Long zoneId = (Long) dsInfos.get("zoneId");
74-
Long podId = (Long)dsInfos.get("podId");
75-
Long clusterId = (Long)dsInfos.get("clusterId");
76-
String storagePoolName = dsInfos.get("name").toString();
77-
String providerName = dsInfos.get("providerName").toString();
78-
String tags = dsInfos.get("tags").toString();
74+
Long zoneId = dsInfos.get("zoneId").toString().trim().isEmpty() ? null : (Long)dsInfos.get("zoneId");
75+
Long podId = dsInfos.get("podId").toString().trim().isEmpty() ? null : (Long)dsInfos.get("zoneId");
76+
Long clusterId = dsInfos.get("clusterId").toString().trim().isEmpty() ? null : (Long)dsInfos.get("clusterId");
77+
String storagePoolName = dsInfos.get("name").toString().trim();
78+
String providerName = dsInfos.get("providerName").toString().trim();
79+
String tags = dsInfos.get("tags").toString().trim();
7980
Boolean isTagARule = (Boolean) dsInfos.get("isTagARule");
8081
String scheme = dsInfos.get("scheme").toString();
8182

@@ -86,12 +87,8 @@ public DataStore initialize(Map<String, Object> dsInfos) {
8687
@SuppressWarnings("unchecked")
8788
Map<String, String> details = (Map<String, String>)dsInfos.get("details");
8889
// Validations
89-
if (podId != null && clusterId == null) {
90-
s_logger.error("Cluster Id is null, cannot create primary storage");
91-
return null;
92-
} else if (podId == null && clusterId != null) {
93-
s_logger.error("Pod Id is null, cannot create primary storage");
94-
return null;
90+
if (podId == null ^ clusterId == null) {
91+
throw new CloudRuntimeException("Cluster Id or Pod Id is null, cannot create primary storage");
9592
}
9693

9794
if (podId == null && clusterId == null) {
@@ -122,28 +119,28 @@ public DataStore initialize(Map<String, Object> dsInfos) {
122119

123120
// TODO: While testing need to check what does this actually do and if the fields corresponding to each protocol should also be set
124121
// TODO: scheme could be 'custom' in our case and we might have to ask 'protocol' separately to the user
125-
String protocol = details.get(Constants.PROTOCOL);
126-
switch (protocol.toLowerCase()) {
127-
case Constants.NFS:
122+
ProtocolType protocol = ProtocolType.valueOf(details.get(Constants.PROTOCOL).toLowerCase());
123+
switch (protocol) {
124+
case NFS:
128125
parameters.setType(Storage.StoragePoolType.NetworkFilesystem);
129126
break;
130-
case Constants.ISCSI:
127+
case ISCSI:
131128
parameters.setType(Storage.StoragePoolType.Iscsi);
132129
break;
133130
default:
134131
throw new CloudRuntimeException("Unsupported protocol: " + scheme + ", cannot create primary storage");
135132
}
136133

137-
details.put(Constants.MANAGEMENTLIF, url);
134+
details.put(Constants.MANAGEMENT_LIF, url);
138135

139136
// Validate the ONTAP details
140-
if(details.get(Constants.ISDISAGGREGATED) == null || details.get(Constants.ISDISAGGREGATED).isEmpty()) {
141-
details.put(Constants.ISDISAGGREGATED, "false");
137+
if(details.get(Constants.IS_DISAGGREGATED) == null || details.get(Constants.IS_DISAGGREGATED).isEmpty()) {
138+
details.put(Constants.IS_DISAGGREGATED, "false");
142139
}
143140

144141
OntapStorage ontapStorage = new OntapStorage(details.get(Constants.USERNAME), details.get(Constants.PASSWORD),
145-
details.get(Constants.MANAGEMENTLIF), details.get(Constants.SVMNAME), details.get(Constants.PROTOCOL),
146-
Boolean.parseBoolean(details.get(Constants.ISDISAGGREGATED)));
142+
details.get(Constants.MANAGEMENT_LIF), details.get(Constants.SVM_NAME), protocol,
143+
Boolean.parseBoolean(details.get(Constants.IS_DISAGGREGATED)));
147144
StorageProviderFactory storageProviderManager = new StorageProviderFactory(ontapStorage);
148145
StorageStrategy storageStrategy = storageProviderManager.getStrategy();
149146
boolean isValid = storageStrategy.connect();

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

Lines changed: 39 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.apache.logging.log4j.Logger;
3838

3939
import javax.inject.Inject;
40+
import java.util.Map;
4041
import java.net.URI;
4142
import java.util.List;
4243
import java.util.Objects;
@@ -69,42 +70,37 @@ public boolean connect() {
6970
s_logger.info("Attempting to connect to ONTAP cluster at " + storage.getManagementLIF());
7071
//Get AuthHeader
7172
String authHeader = utils.generateAuthHeader(storage.getUsername(), storage.getPassword());
73+
String svmName = storage.getSvmName();
7274
try {
7375
// Call the SVM API to check if the SVM exists
7476
Svm svm = null;
75-
URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + Constants.GETSVMs);
76-
OntapResponse<Svm> svms = svmFeignClient.getSvms(url, authHeader);
77-
for (Svm storageVM : svms.getRecords()) {
78-
if (storageVM.getName().equals(storage.getSVM())) {
79-
svm = storageVM;
80-
s_logger.info("Found SVM: " + storage.getSVM());
81-
break;
82-
}
77+
URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + Constants.GET_SVMs);
78+
Map<String, String> queryParams = Map.of("name", svmName);
79+
OntapResponse<Svm> svms = svmFeignClient.getSvmResponse(url, authHeader, queryParams);
80+
if (svms != null && svms.getRecords() != null && !svms.getRecords().isEmpty()) {
81+
svm = svms.getRecords().get(0);
82+
} else {
83+
throw new CloudRuntimeException("No SVM found on the ONTAP cluster by the name" + svmName + ".");
8384
}
8485

8586
// Validations
86-
if (svm == null) {
87-
s_logger.error("SVM with name " + storage.getSVM() + " not found.");
88-
throw new CloudRuntimeException("SVM with name " + storage.getSVM() + " not found.");
89-
} else {
90-
if (svm.getState() != Constants.RUNNING) {
91-
s_logger.error("SVM " + storage.getSVM() + " is not in running state.");
92-
throw new CloudRuntimeException("SVM " + storage.getSVM() + " is not in running state.");
93-
}
94-
if (Objects.equals(storage.getProtocol(), Constants.NFS) && !svm.getNfsEnabled()) {
95-
s_logger.error("NFS protocol is not enabled on SVM " + storage.getSVM());
96-
throw new CloudRuntimeException("NFS protocol is not enabled on SVM " + storage.getSVM());
97-
} else if (Objects.equals(storage.getProtocol(), Constants.ISCSI) && !svm.getIscsiEnabled()) {
98-
s_logger.error("iSCSI protocol is not enabled on SVM " + storage.getSVM());
99-
throw new CloudRuntimeException("iSCSI protocol is not enabled on SVM " + storage.getSVM());
100-
}
101-
List<Aggregate> aggrs = svm.getAggregates();
102-
if (aggrs == null || aggrs.isEmpty()) {
103-
s_logger.error("No aggregates are assigned to SVM " + storage.getSVM());
104-
throw new CloudRuntimeException("No aggregates are assigned to SVM " + storage.getSVM());
105-
}
106-
this.aggregates = aggrs;
87+
if (!Objects.equals(svm.getState(), Constants.RUNNING)) {
88+
s_logger.error("SVM " + svmName + " is not in running state.");
89+
throw new CloudRuntimeException("SVM " + svmName + " is not in running state.");
90+
}
91+
if (Objects.equals(storage.getProtocol(), Constants.NFS) && !svm.getNfsEnabled()) {
92+
s_logger.error("NFS protocol is not enabled on SVM " + svmName);
93+
throw new CloudRuntimeException("NFS protocol is not enabled on SVM " + svmName);
94+
} else if (Objects.equals(storage.getProtocol(), Constants.ISCSI) && !svm.getIscsiEnabled()) {
95+
s_logger.error("iSCSI protocol is not enabled on SVM " + svmName);
96+
throw new CloudRuntimeException("iSCSI protocol is not enabled on SVM " + svmName);
10797
}
98+
List<Aggregate> aggrs = svm.getAggregates();
99+
if (aggrs == null || aggrs.isEmpty()) {
100+
s_logger.error("No aggregates are assigned to SVM " + svmName);
101+
throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName);
102+
}
103+
this.aggregates = aggrs;
108104
s_logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided");
109105
} catch (Exception e) {
110106
throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage());
@@ -116,17 +112,18 @@ public boolean connect() {
116112
public void createVolume(String volumeName, Long size) {
117113
s_logger.info("Creating volume: " + volumeName + " of size: " + size + " bytes");
118114

115+
String svmName = storage.getSvmName();
119116
if (aggregates == null || aggregates.isEmpty()) {
120-
s_logger.error("No aggregates available to create volume on SVM " + storage.getSVM());
121-
throw new CloudRuntimeException("No aggregates available to create volume on SVM " + storage.getSVM());
117+
s_logger.error("No aggregates available to create volume on SVM " + svmName);
118+
throw new CloudRuntimeException("No aggregates available to create volume on SVM " + svmName);
122119
}
123120
// Get the AuthHeader
124121
String authHeader = utils.generateAuthHeader(storage.getUsername(), storage.getPassword());
125122

126123
// Generate the Create Volume Request
127124
Volume volumeRequest = new Volume();
128125
Svm svm = new Svm();
129-
svm.setName(storage.getSVM());
126+
svm.setName(svmName);
130127

131128
volumeRequest.setName(volumeName);
132129
volumeRequest.setSvm(svm);
@@ -135,17 +132,20 @@ public void createVolume(String volumeName, Long size) {
135132
// Make the POST API call to create the volume
136133
try {
137134
// Create URI for POST CreateVolume API
138-
URI url = utils.generateURI(Constants.CREATEVOLUME);
135+
URI url = utils.generateURI(Constants.CREATE_VOLUME);
139136
// Call the VolumeFeignClient to create the volume
140137
JobResponse jobResponse = volumeFeignClient.createVolumeWithJob(url, authHeader, volumeRequest);
138+
if (jobResponse == null || jobResponse.getJob() == null) {
139+
throw new CloudRuntimeException("Failed to initiate volume creation for " + volumeName);
140+
}
141141
String jobUUID = jobResponse.getJob().getUuid();
142142

143143
//Create URI for GET Job API
144-
url = utils.generateURI(Constants.GETJOBBYUUID);
145-
int jobRetryCount = 0, maxJobRetries = Constants.JOBMAXRETRIES;
144+
url = utils.generateURI(Constants.GET_JOB_BY_UUID);
145+
int jobRetryCount = 0;
146146
Job createVolumeJob = null;
147-
while(createVolumeJob == null || createVolumeJob.getState().equals(Constants.JOBRUNNING) || createVolumeJob.getState().equals(Constants.JOBQUEUE) || createVolumeJob.getState().equals(Constants.JOBPAUSED)) {
148-
if(jobRetryCount >= maxJobRetries) {
147+
while(createVolumeJob == null || !createVolumeJob.getState().equals(Constants.JOB_SUCCESS)) {
148+
if(jobRetryCount >= Constants.JOB_MAX_RETRIES) {
149149
s_logger.error("Job to create volume " + volumeName + " did not complete within expected time.");
150150
throw new CloudRuntimeException("Job to create volume " + volumeName + " did not complete within expected time.");
151151
}
@@ -154,15 +154,15 @@ public void createVolume(String volumeName, Long size) {
154154
createVolumeJob = jobFeignClient.getJobByUUID(url, authHeader, jobUUID);
155155
if (createVolumeJob == null) {
156156
s_logger.warn("Job with UUID " + jobUUID + " not found. Retrying...");
157-
} else if (createVolumeJob.getState().equals(Constants.JOBFAILURE)) {
157+
} else if (createVolumeJob.getState().equals(Constants.JOB_FAILURE)) {
158158
throw new CloudRuntimeException("Job to create volume " + volumeName + " failed with error: " + createVolumeJob.getMessage());
159159
}
160160
} catch (FeignException.FeignClientException e) {
161161
throw new CloudRuntimeException("Failed to fetch job status: " + e.getMessage());
162162
}
163163

164164
jobRetryCount++;
165-
Thread.sleep(Constants.CREATEVOLUMECHECKSLEEPTIME); // Sleep for 2 seconds before polling again
165+
Thread.sleep(Constants.CREATE_VOLUME_CHECK_SLEEP_TIME); // Sleep for 2 seconds before polling again
166166
}
167167
} catch (Exception e) {
168168
s_logger.error("Exception while creating volume: ", e);

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

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,27 +20,32 @@
2020
package org.apache.cloudstack.storage.utils;
2121

2222
public class Constants {
23+
public enum ProtocolType {
24+
NFS,
25+
ISCSI
26+
}
27+
2328
public static final String NFS = "nfs";
2429
public static final String ISCSI = "iscsi";
2530
public static final String PROTOCOL = "protocol";
26-
public static final String SVMNAME = "svmName";
31+
public static final String SVM_NAME = "svmName";
2732
public static final String USERNAME = "username";
2833
public static final String PASSWORD = "password";
29-
public static final String MANAGEMENTLIF = "managementLIF";
30-
public static final String ISDISAGGREGATED = "isDisaggregated";
34+
public static final String MANAGEMENT_LIF = "managementLIF";
35+
public static final String IS_DISAGGREGATED = "isDisaggregated";
3136
public static final String RUNNING = "running";
3237

33-
public static final String JOBRUNNING = "running";
34-
public static final String JOBQUEUE = "queued";
35-
public static final String JOBPAUSED = "paused";
36-
public static final String JOBFAILURE = "failure";
37-
public static final String JOBSUCCESS = "success";
38+
public static final String JOB_RUNNING = "running";
39+
public static final String JOB_QUEUE = "queued";
40+
public static final String JOB_PAUSED = "paused";
41+
public static final String JOB_FAILURE = "failure";
42+
public static final String JOB_SUCCESS = "success";
3843

39-
public static final int JOBMAXRETRIES = 100;
40-
public static final int CREATEVOLUMECHECKSLEEPTIME = 2000;
44+
public static final int JOB_MAX_RETRIES = 100;
45+
public static final int CREATE_VOLUME_CHECK_SLEEP_TIME = 2000;
4146

4247
public static final String HTTPS = "https://";
43-
public static final String GETSVMs = "/api/svm/svms";
44-
public static final String CREATEVOLUME = "/api/storage/volumes";
45-
public static final String GETJOBBYUUID = "/api/cluster/jobs";
48+
public static final String GET_SVMs = "/api/svm/svms";
49+
public static final String CREATE_VOLUME = "/api/storage/volumes";
50+
public static final String GET_JOB_BY_UUID = "/api/cluster/jobs";
4651
}

0 commit comments

Comments
 (0)