Skip to content

Commit fd67a41

Browse files
Locharla, SandeepLocharla, Sandeep
Locharla, Sandeep
authored and
Locharla, Sandeep
committed
CSTACKEX-01: Addressed all review comments and updated some code
1 parent b855306 commit fd67a41

34 files changed

+190
-179
lines changed

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,9 @@ public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver {
6262

6363
private static final Logger s_logger = LogManager.getLogger(OntapPrimaryDatastoreDriver.class);
6464

65-
private Utility utils;
6665
@Inject private StoragePoolDetailsDao storagePoolDetailsDao;
6766
@Inject private PrimaryDataStoreDao storagePoolDao;
68-
public OntapPrimaryDatastoreDriver() {
69-
utils = new Utility();
70-
}
67+
public OntapPrimaryDatastoreDriver() {}
7168
@Override
7269
public Map<String, String> getCapabilities() {
7370
s_logger.trace("OntapPrimaryDatastoreDriver: getCapabilities: Called");
@@ -133,7 +130,7 @@ private String createCloudStackVolumeForTypeVolume(DataStore dataStore, DataObje
133130
Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(dataStore.getId());
134131
StorageStrategy storageStrategy = getStrategyByStoragePoolDetails(details);
135132
s_logger.info("createCloudStackVolumeForTypeVolume: Connection to Ontap SVM [{}] successful, preparing CloudStackVolumeRequest", details.get(Constants.SVM_NAME));
136-
CloudStackVolume cloudStackVolumeRequest = utils.createCloudStackVolumeRequestByProtocol(storagePool, details, dataObject);
133+
CloudStackVolume cloudStackVolumeRequest = Utility.createCloudStackVolumeRequestByProtocol(storagePool, details, dataObject);
137134
CloudStackVolume cloudStackVolume = storageStrategy.createCloudStackVolume(cloudStackVolumeRequest);
138135
if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(Constants.PROTOCOL)) && cloudStackVolume.getLun() != null && cloudStackVolume.getLun().getName() != null) {
139136
return cloudStackVolume.getLun().getName();
@@ -274,7 +271,7 @@ public void detachVolumeFromAllStorageNodes(Volume volume) {
274271

275272
}
276273

277-
public StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details) {
274+
private StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details) {
278275
if (details == null || details.isEmpty()) {
279276
s_logger.error("getStrategyByStoragePoolDetails: Storage pool details are null or empty");
280277
throw new CloudRuntimeException("getStrategyByStoragePoolDetails: Storage pool details are null or empty");

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ public <T> T createClient(Class<T> clientClass, String baseURL) {
3838
.client(feignConfiguration.createClient())
3939
.encoder(feignConfiguration.createEncoder())
4040
.decoder(feignConfiguration.createDecoder())
41-
// .logger(feignConfiguration.createLogger())
4241
.retryer(feignConfiguration.createRetryer())
4342
.requestInterceptor(feignConfiguration.createRequestInterceptor())
4443
.target(clientClass, baseURL);

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

Lines changed: 15 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,3 @@
1-
/*
2-
* Licensed to the Apache Software Foundation (ASF) under one
3-
* or more contributor license agreements. See the NOTICE file
4-
* distributed with this work for additional information
5-
* regarding copyright ownership. The ASF licenses this file
6-
* to you under the Apache License, Version 2.0 (the
7-
* "License"); you may not use this file except in compliance
8-
* with the License. You may obtain a copy of the License at
9-
*
10-
* http://www.apache.org/licenses/LICENSE-2.0
11-
*
12-
* Unless required by applicable law or agreed to in writing,
13-
* software distributed under the License is distributed on an
14-
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15-
* KIND, either express or implied. See the License for the
16-
* specific language governing permissions and limitations
17-
* under the License.
18-
*/
19-
201
package org.apache.cloudstack.storage.feign;
212

223
import feign.RequestInterceptor;
@@ -25,13 +6,12 @@
256
import feign.httpclient.ApacheHttpClient;
267
import feign.codec.Decoder;
278
import feign.codec.Encoder;
28-
//import feign.slf4j.Slf4jLogger;
299
import feign.Response;
3010
import feign.codec.DecodeException;
3111
import feign.codec.EncodeException;
32-
import com.fasterxml.jackson.databind.ObjectMapper;
33-
import com.fasterxml.jackson.databind.DeserializationFeature;
3412
import com.fasterxml.jackson.core.JsonProcessingException;
13+
import com.fasterxml.jackson.databind.DeserializationFeature;
14+
import com.fasterxml.jackson.databind.json.JsonMapper;
3515
import org.apache.http.conn.ConnectionKeepAliveStrategy;
3616
import org.apache.http.conn.ssl.NoopHostnameVerifier;
3717
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
@@ -44,6 +24,7 @@
4424

4525
import javax.net.ssl.SSLContext;
4626
import java.io.IOException;
27+
import java.io.InputStream;
4728
import java.lang.reflect.Type;
4829
import java.nio.charset.StandardCharsets;
4930
import java.util.concurrent.TimeUnit;
@@ -55,12 +36,13 @@ public class FeignConfiguration {
5536
private final int retryMaxInterval = 5;
5637
private final String ontapFeignMaxConnection = "80";
5738
private final String ontapFeignMaxConnectionPerRoute = "20";
58-
private final ObjectMapper objectMapper;
39+
private final JsonMapper jsonMapper;
5940

6041
public FeignConfiguration() {
61-
this.objectMapper = new ObjectMapper();
62-
// Configure ObjectMapper to ignore unknown properties like _links
63-
this.objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
42+
this.jsonMapper = JsonMapper.builder()
43+
.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
44+
.findAndAddModules()
45+
.build();
6446
}
6547

6648
public Client createClient() {
@@ -69,18 +51,16 @@ public Client createClient() {
6951
try {
7052
maxConn = Integer.parseInt(this.ontapFeignMaxConnection);
7153
} catch (Exception e) {
72-
logger.error("ontapFeignClient: encounter exception while parse the max connection from env. setting default value");
54+
logger.error("ontapFeignClient: parse max connection failed, using default");
7355
maxConn = 20;
7456
}
7557
try {
7658
maxConnPerRoute = Integer.parseInt(this.ontapFeignMaxConnectionPerRoute);
7759
} catch (Exception e) {
78-
logger.error("ontapFeignClient: encounter exception while parse the max connection per route from env. setting default value");
60+
logger.error("ontapFeignClient: parse max connection per route failed, using default");
7961
maxConnPerRoute = 2;
8062
}
81-
82-
// Disable Keep Alive for Http Connection
83-
logger.debug("ontapFeignClient: Setting the feign client config values as max connection: {}, max connections per route: {}", maxConn, maxConnPerRoute);
63+
logger.debug("ontapFeignClient: maxConn={}, maxConnPerRoute={}", maxConn, maxConnPerRoute);
8464
ConnectionKeepAliveStrategy keepAliveStrategy = (response, context) -> 0;
8565
CloseableHttpClient httpClient = HttpClientBuilder.create()
8666
.setMaxConnTotal(maxConn)
@@ -94,7 +74,6 @@ public Client createClient() {
9474

9575
private SSLConnectionSocketFactory getSSLSocketFactory() {
9676
try {
97-
// The TrustAllStrategy is a strategy used in SSL context configuration that accepts any certificate
9877
SSLContext sslContext = SSLContexts.custom().loadTrustMaterial(null, new TrustAllStrategy()).build();
9978
return new SSLConnectionSocketFactory(sslContext, new NoopHostnameVerifier());
10079
} catch (Exception ex) {
@@ -108,7 +87,7 @@ public RequestInterceptor createRequestInterceptor() {
10887
logger.info("HTTP Method: {}", template.method());
10988
logger.info("Headers: {}", template.headers());
11089
if (template.body() != null) {
111-
logger.info("Body: {}", new String(template.body()));
90+
logger.info("Body: {}", new String(template.body(), StandardCharsets.UTF_8));
11291
}
11392
};
11493
}
@@ -126,7 +105,7 @@ public void encode(Object object, Type bodyType, feign.RequestTemplate template)
126105
return;
127106
}
128107
try {
129-
byte[] jsonBytes = objectMapper.writeValueAsBytes(object);
108+
byte[] jsonBytes = jsonMapper.writeValueAsBytes(object);
130109
template.body(jsonBytes, StandardCharsets.UTF_8);
131110
template.header("Content-Type", "application/json");
132111
} catch (JsonProcessingException e) {
@@ -144,20 +123,15 @@ public Object decode(Response response, Type type) throws IOException, DecodeExc
144123
return null;
145124
}
146125
String json = null;
147-
try (var bodyStream = response.body().asInputStream()) {
126+
try (InputStream bodyStream = response.body().asInputStream()) {
148127
json = new String(bodyStream.readAllBytes(), StandardCharsets.UTF_8);
149128
logger.debug("Decoding JSON response: {}", json);
150-
return objectMapper.readValue(json, objectMapper.getTypeFactory().constructType(type));
129+
return jsonMapper.readValue(json, jsonMapper.getTypeFactory().constructType(type));
151130
} catch (IOException e) {
152131
logger.error("Error decoding JSON response. Status: {}, Raw body: {}", response.status(), json, e);
153132
throw new DecodeException(response.status(), "Error decoding JSON response", response.request(), e);
154133
}
155134
}
156135
};
157136
}
158-
159-
// public Slf4jLogger createLogger() {
160-
// return new Slf4jLogger();
161-
// }
162137
}
163-

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

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,22 +45,20 @@ void deleteFile(@Param("authHeader") String authHeader,
4545
@Headers({"Authorization: {authHeader}"})
4646
void updateFile(@Param("authHeader") String authHeader,
4747
@Param("volumeUuid") String volumeUUID,
48-
@Param("path") String filePath,
49-
@Param("fileInfo") FileInfo fileInfo);
48+
@Param("path") String filePath, FileInfo fileInfo);
5049

5150
@RequestLine("POST /{volumeUuid}/files/{path}")
5251
@Headers({"Authorization: {authHeader}"})
5352
void createFile(@Param("authHeader") String authHeader,
5453
@Param("volumeUuid") String volumeUUID,
55-
@Param("path") String filePath,
56-
@Param("file") FileInfo file);
54+
@Param("path") String filePath, FileInfo file);
5755

5856
// Export Policy Operations
5957
@RequestLine("POST /")
6058
@Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"})
6159
ExportPolicy createExportPolicy(@Param("authHeader") String authHeader,
6260
@Param("returnRecords") boolean returnRecords,
63-
@Param("exportPolicy") ExportPolicy exportPolicy);
61+
ExportPolicy exportPolicy);
6462

6563
@RequestLine("GET /")
6664
@Headers({"Authorization: {authHeader}"})
@@ -80,5 +78,5 @@ void deleteExportPolicyById(@Param("authHeader") String authHeader,
8078
@Headers({"Authorization: {authHeader}"})
8179
OntapResponse<ExportPolicy> updateExportPolicy(@Param("authHeader") String authHeader,
8280
@Param("id") String id,
83-
@Param("request") ExportPolicy request);
81+
ExportPolicy request);
8482
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public interface SANFeignClient {
3434
@Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"})
3535
OntapResponse<Lun> createLun(@Param("authHeader") String authHeader,
3636
@Param("returnRecords") boolean returnRecords,
37-
@Param("lun") Lun lun);
37+
Lun lun);
3838

3939
@RequestLine("GET /")
4040
@Headers({"Authorization: {authHeader}"})
@@ -46,7 +46,7 @@ OntapResponse<Lun> createLun(@Param("authHeader") String authHeader,
4646

4747
@RequestLine("PATCH /{uuid}")
4848
@Headers({"Authorization: {authHeader}"})
49-
void updateLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, @Param("lun") Lun lun);
49+
void updateLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Lun lun);
5050

5151
@RequestLine("DELETE /{uuid}")
5252
@Headers({"Authorization: {authHeader}"})
@@ -57,10 +57,10 @@ OntapResponse<Lun> createLun(@Param("authHeader") String authHeader,
5757
@Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"})
5858
OntapResponse<Igroup> createIgroup(@Param("authHeader") String authHeader,
5959
@Param("returnRecords") boolean returnRecords,
60-
@Param("igroupRequest") Igroup igroupRequest);
60+
Igroup igroupRequest);
6161

6262
@RequestLine("GET /")
63-
@Headers({"Authorization: {authHeader}"})
63+
@Headers({"Authorization: {authHeader}"}) // TODO: Check this again, uuid should be part of the path?
6464
OntapResponse<Igroup> getIgroupResponse(@Param("authHeader") String authHeader, @Param("uuid") String uuid);
6565

6666
@RequestLine("GET /{uuid}")
@@ -75,13 +75,13 @@ OntapResponse<Igroup> createIgroup(@Param("authHeader") String authHeader,
7575
@Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"})
7676
OntapResponse<Igroup> addNestedIgroups(@Param("authHeader") String authHeader,
7777
@Param("uuid") String uuid,
78-
@Param("igroupNestedRequest") Igroup igroupNestedRequest,
78+
Igroup igroupNestedRequest,
7979
@Param("returnRecords") boolean returnRecords);
8080

8181
// LUN Maps Operation APIs
8282
@RequestLine("POST /")
8383
@Headers({"Authorization: {authHeader}"})
84-
OntapResponse<LunMap> createLunMap(@Param("authHeader") String authHeader, @Param("lunMap") LunMap lunMap);
84+
OntapResponse<LunMap> createLunMap(@Param("authHeader") String authHeader, LunMap lunMap);
8585

8686
@RequestLine("GET /")
8787
@Headers({"Authorization: {authHeader}"})

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@
1919

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

22+
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
2223
import com.fasterxml.jackson.annotation.JsonInclude;
2324
import com.fasterxml.jackson.annotation.JsonProperty;
2425

2526
import java.util.Objects;
2627

28+
@JsonIgnoreProperties(ignoreUnknown = true)
2729
@JsonInclude(JsonInclude.Include.NON_NULL)
2830
public class Aggregate {
2931

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

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

22+
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
2223
import com.fasterxml.jackson.annotation.JsonInclude;
2324
import com.fasterxml.jackson.annotation.JsonProperty;
2425

@@ -28,6 +29,7 @@
2829
* Complete cluster information
2930
*/
3031
@SuppressWarnings("checkstyle:RegexpSingleline")
32+
@JsonIgnoreProperties(ignoreUnknown = true)
3133
@JsonInclude(JsonInclude.Include.NON_NULL)
3234
public class Cluster {
3335

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,14 @@
1919

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

22+
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
2223
import com.fasterxml.jackson.annotation.JsonInclude;
2324
import com.fasterxml.jackson.annotation.JsonProperty;
2425
import java.math.BigInteger;
2526
import java.util.List;
2627
import java.util.Objects;
2728

29+
@JsonIgnoreProperties(ignoreUnknown = true)
2830
@JsonInclude(JsonInclude.Include.NON_NULL)
2931
public class ExportPolicy {
3032

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,15 @@
1919

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

22+
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
2223
import com.fasterxml.jackson.annotation.JsonInclude;
2324
import com.fasterxml.jackson.annotation.JsonProperty;
2425
import java.util.List;
2526

2627
/**
2728
* ExportRule
2829
*/
30+
@JsonIgnoreProperties(ignoreUnknown = true)
2931
@JsonInclude(JsonInclude.Include.NON_NULL)
3032
public class ExportRule {
3133
@JsonProperty("anonymous_user")

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.apache.cloudstack.storage.feign.model;
2121

2222
import com.fasterxml.jackson.annotation.JsonCreator;
23+
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
2324
import com.fasterxml.jackson.annotation.JsonInclude;
2425
import com.fasterxml.jackson.annotation.JsonProperty;
2526
import com.fasterxml.jackson.annotation.JsonValue;
@@ -30,6 +31,7 @@
3031
/**
3132
* Information about a single file.
3233
*/
34+
@JsonIgnoreProperties(ignoreUnknown = true)
3335
@JsonInclude(JsonInclude.Include.NON_NULL)
3436
public class FileInfo {
3537
@JsonProperty("bytes_used")

0 commit comments

Comments
 (0)