-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/cstackex-01: Primary Storage pool creation #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements primary storage pool creation functionality for NetApp ONTAP integration in CloudStack. The changes migrate from Spring Cloud OpenFeign to standard OpenFeign, refactor dependency injection patterns, and add core infrastructure for creating and managing ONTAP storage pools.
Key Changes:
- Migration from Spring Cloud OpenFeign to core OpenFeign library with manual client initialization
- Implementation of primary storage pool creation workflow including SVM validation and volume creation
- Refactoring of dependency injection from Spring @Inject to constructor-based instantiation
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Replaces Spring Cloud OpenFeign dependencies with core OpenFeign libraries (feign-core, feign-httpclient, feign-jackson) |
| FeignClientFactory.java | New factory class for creating Feign clients with custom configuration |
| FeignConfiguration.java | Migrated from Spring @bean annotations to factory methods for Feign components |
| All FeignClient interfaces | Converted from Spring annotations (@FeignClient, @RequestMapping) to core Feign annotations (@RequestLine, @headers) |
| StorageStrategy.java | Replaced @Inject dependencies with constructor-based initialization using FeignClientFactory |
| UnifiedSANStrategy.java, UnifiedNASStrategy.java | Added constructor-based initialization and setter methods for OntapStorage |
| StorageProviderFactory.java | Converted from Spring @component to static factory pattern |
| OntapPrimaryDatastoreLifecycle.java | Implemented primary storage creation logic with URL parsing and validation |
| OntapStorage.java | Fixed setter methods to properly assign instance variables |
| Utility.java | Removed unused methods and Spring dependencies |
| Constants.java | Added SIZE constant, removed unused API endpoint constants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s_logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided"); | ||
| } catch (Exception e) { | ||
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage()); | ||
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e); |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message loses the original exception's specific message. Consider using 'e.getMessage()' instead of 'e' to avoid printing the full stack trace in the message, or wrap the exception properly with 'new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage(), e)' to preserve the cause.
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e); | |
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| s_logger.info("Attempting to connect to ONTAP cluster at " + storage.getManagementLIF() + " and validate SVM " + | ||
| storage.getSvmName() + ", username " + storage.getUsername() + ", password " + storage.getPassword() + ", protocol " + storage.getProtocol()); |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging sensitive credentials (username and password) in plaintext is a security risk. Remove password from the log statement and consider masking or omitting the username as well.
| s_logger.info("Attempting to connect to ONTAP cluster at " + storage.getManagementLIF() + " and validate SVM " + | |
| storage.getSvmName() + ", username " + storage.getUsername() + ", password " + storage.getPassword() + ", protocol " + storage.getProtocol()); | |
| // Mask username except first character, if present | |
| String maskedUsername = storage.getUsername() != null && !storage.getUsername().isEmpty() | |
| ? storage.getUsername().charAt(0) + "***" | |
| : ""; | |
| s_logger.info("Attempting to connect to ONTAP cluster at " + storage.getManagementLIF() + | |
| " and validate SVM " + storage.getSvmName() + | |
| ", username " + maskedUsername + | |
| ", protocol " + storage.getProtocol()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @PathVariable(name="igroup.uuid", required=true) String igroupUUID); | ||
|
|
||
| // LUN Operation APIs | ||
| @RequestLine("POST /") |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RequestLine is incomplete - it uses '/' as the path, but based on the previous implementation this should include the full API path like 'POST /api/storage/luns'. Without the full path in the annotation, the API calls will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added TODO, for developers who are going to use this file
...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java
Outdated
Show resolved
Hide resolved
| return null; | ||
| } | ||
| String json = null; | ||
| try (var bodyStream = response.body().asInputStream()) { |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using 'var' for type inference is valid in Java 11+, but may reduce code readability. Consider explicitly declaring the type as 'InputStream bodyStream' for clarity.
| try (var bodyStream = response.body().asInputStream()) { | |
| try (InputStream bodyStream = response.body().asInputStream()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java
Show resolved
Hide resolved
...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java
Outdated
Show resolved
Hide resolved
| try { | ||
| s_logger.info("Fetching the SVM details..."); | ||
| if (svmFeignClient == null) { | ||
| throw new CloudRuntimeException("SVM Feign client is not initialized."); | ||
| } | ||
| Map<String, Object> queryParams = Map.of("name", svmName, "fields", "aggregates,state"); | ||
| OntapResponse<Svm> svms = svmFeignClient.getSvmResponse(queryParams, authHeader); | ||
| if (svms != null && svms.getRecords() != null && !svms.getRecords().isEmpty()) { | ||
| svm = svms.getRecords().get(0); | ||
| } else { | ||
| throw new CloudRuntimeException("No SVM found on the ONTAP cluster by the name" + svmName + "."); | ||
| } | ||
| } catch (FeignException.FeignClientException e) { | ||
| throw new CloudRuntimeException("Failed to fetch SVM details: " + e.getMessage()); | ||
| } |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catch block only handles FeignException.FeignClientException, but line 129 has a broader catch block for all exceptions. The inner try-catch adds unnecessary nesting and the outer catch will handle FeignException already. Consider removing the inner try-catch block.
| try { | |
| s_logger.info("Fetching the SVM details..."); | |
| if (svmFeignClient == null) { | |
| throw new CloudRuntimeException("SVM Feign client is not initialized."); | |
| } | |
| Map<String, Object> queryParams = Map.of("name", svmName, "fields", "aggregates,state"); | |
| OntapResponse<Svm> svms = svmFeignClient.getSvmResponse(queryParams, authHeader); | |
| if (svms != null && svms.getRecords() != null && !svms.getRecords().isEmpty()) { | |
| svm = svms.getRecords().get(0); | |
| } else { | |
| throw new CloudRuntimeException("No SVM found on the ONTAP cluster by the name" + svmName + "."); | |
| } | |
| } catch (FeignException.FeignClientException e) { | |
| throw new CloudRuntimeException("Failed to fetch SVM details: " + e.getMessage()); | |
| } | |
| s_logger.info("Fetching the SVM details..."); | |
| if (svmFeignClient == null) { | |
| throw new CloudRuntimeException("SVM Feign client is not initialized."); | |
| } | |
| Map<String, Object> queryParams = Map.of("name", svmName, "fields", "aggregates,state"); | |
| OntapResponse<Svm> svms = svmFeignClient.getSvmResponse(queryParams, authHeader); | |
| if (svms != null && svms.getRecords() != null && !svms.getRecords().isEmpty()) { | |
| svm = svms.getRecords().get(0); | |
| } else { | |
| throw new CloudRuntimeException("No SVM found on the ONTAP cluster by the name" + svmName + "."); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| } | ||
|
|
||
| public StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it 'private'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"}) | ||
| OntapResponse<Lun> createLun(@Param("authHeader") String authHeader, | ||
| @Param("returnRecords") boolean returnRecords, | ||
| @Param("lun") Lun lun); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: All feign clients should be revisited for proper implementation according to ontap api spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Developer now has to only take care of the URLs in SANFeignClient and NASFeignClient files. Will try to make those changes also in the next PR, if its not already taken care of
| private String password; | ||
| private String managementLIF; | ||
| private String svmName; | ||
| private String username; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make them all final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| private static final Logger s_logger = LogManager.getLogger(OntapPrimaryDatastoreDriver.class); | ||
|
|
||
| @Inject private Utility utils; | ||
| private Utility utils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it 'final'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| @JsonInclude(JsonInclude.Include.NON_NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Unknown properties to all models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its ignore unknown propoerties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @Bean | ||
| public Client client(ApacheHttpClientFactory httpClientFactory) { | ||
| public FeignConfiguration() { | ||
| this.objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out if json mapper could be used here to ignore unknown properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Long clusterId = (Long)dsInfos.get("clusterId"); | ||
| String storagePoolName = (String)dsInfos.get("name"); | ||
| String providerName = (String)dsInfos.get("providerName"); | ||
| Long capacityBytes = (Long)dsInfos.get("capacityBytes"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reuse this for Volume size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| String storagePoolName = (String)dsInfos.get("name"); | ||
| String providerName = (String)dsInfos.get("providerName"); | ||
| Long capacityBytes = (Long)dsInfos.get("capacityBytes"); | ||
| String tags = (String)dsInfos.get("tags"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add validation on this (for NPE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary, as cloudstack provides 'tags' key in dsInfos map, while the value could be 'null', if in case tags are not set.
| @SuppressWarnings("unchecked") | ||
| Map<String, String> details = (Map<String, String>)dsInfos.get("details"); | ||
| // Validations | ||
| if (capacityBytes == null || capacityBytes <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Ontap related check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
|
|
||
| // Get ONTAP details from the URL | ||
| Map<String, String> storageDetails = Map.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use hashset from here and check below if the key exists in it before setting, else throw an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, explaination on current implementation has been provided for another comment on the same file
| // TODO: scheme could be 'custom' in our case and we might have to ask 'protocol' separately to the user | ||
| ProtocolType protocol = ProtocolType.valueOf(details.get(Constants.PROTOCOL).toLowerCase()); | ||
| String path = ""; | ||
| ProtocolType protocol = ProtocolType.valueOf(details.get(Constants.PROTOCOL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add NFS3 in the Protocols enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java
Outdated
Show resolved
Hide resolved
...ume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/AggregateFeignClient.java
Show resolved
Hide resolved
...olume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/ClusterFeignClient.java
Show resolved
Hide resolved
|
|
||
| @RequestLine("POST /{uuid}/igroups") | ||
| @Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"}) | ||
| OntapResponse<Igroup> addNestedIgroups(@Param("authHeader") String authHeader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will not have nested igroups here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending to be addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SvmFeignClient.java
Show resolved
Hide resolved
| parameters.setClusterId(clusterId); | ||
| parameters.setName(storagePoolName); | ||
| parameters.setProviderName(providerName); | ||
| parameters.setManaged(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this hardcoded, shall we assign this based on the check box value coming from CS UI form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever, user chooses ONTAP storage provider, any entity created in cloudstack via ONTAP provider should be managed by us. So, by default irrespective of whether user chooses to be managed or not, this should be 'true'. I've checked other vendors implementation also doing the same.
One thing that we could do is to check if 'managed' field is set to 'null' by the user, callout via an exception to draw user's attention to enable it.
Let me know if we should be doing this?
| ProtocolType protocol = ontapStorage.getProtocol(); | ||
| s_logger.info("Initializing StorageProviderFactory with protocol: " + protocol); | ||
| switch (protocol) { | ||
| case NFS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets add NFS3 even here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| public boolean connect() { | ||
| s_logger.info("Attempting to connect to ONTAP cluster at " + storage.getManagementLIF()); | ||
| s_logger.info("Attempting to connect to ONTAP cluster at " + storage.getManagementLIF() + " and validate SVM " + | ||
| storage.getSvmName() + ", username " + storage.getUsername() + ", password " + storage.getPassword() + ", protocol " + storage.getProtocol()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not print username and password in logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| throw new CloudRuntimeException("No SVM found on the ONTAP cluster by the name" + svmName + "."); | ||
| try { | ||
| s_logger.info("Fetching the SVM details..."); | ||
| if (svmFeignClient == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This initialization is happening in the constructor of this class, do we see any chances of not getting this initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this came in from earlier code where we were using spring-feign, will don't need this
| abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); | ||
|
|
||
| /** | ||
| * Method encapsulates the behavior based on the opted protocol in subclasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reasons for removing these methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted them, thankyou
Copilot has removed them due to a comment, I've added for my reference in the code
plugins/storage/volume/ontap/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| <version>2.15.2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this version inside property tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thankyou
plugins/storage/volume/ontap/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.apache.httpcomponents</groupId> | ||
| <artifactId>httpclient</artifactId> | ||
| <version>4.5.14</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thankyou
| private Utility utils; | ||
| @Inject private StoragePoolDetailsDao storagePoolDetailsDao; | ||
| @Inject private PrimaryDataStoreDao storagePoolDao; | ||
| public OntapPrimaryDatastoreDriver() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can also create utility method as static, so we need not to instantiate utility class here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thankyou
| this.utils = new Utility(); | ||
| // Initialize FeignClientFactory and create clients | ||
| this.feignClientFactory = new FeignClientFactory(); | ||
| this.volumeFeignClient = feignClientFactory.createClient(VolumeFeignClient.class, baseURL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also create the feign client based on the protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently required in StorageStrategy also any protocol specific feign calls must be made in the respective classes
| if (svmFeignClient == null) { | ||
| throw new CloudRuntimeException("SVM Feign client is not initialized."); | ||
| } | ||
| Map<String, Object> queryParams = Map.of("name", svmName, "fields", "aggregates,state"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass these values as a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @RequestLine("POST /") | ||
| @Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"}) | ||
| OntapResponse<Lun> createLun(@Param("authHeader") String authHeader, | ||
| @Param("returnRecords") boolean returnRecords, | ||
| Lun lun); |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @RequestLine annotation specifies only POST / without the full API path. This should include the complete endpoint path like POST /api/storage/luns to match the ONTAP API structure, similar to other Feign clients in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a TODO task to be taken up when using these methods in implementation
...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java
Show resolved
Hide resolved
...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java
Outdated
Show resolved
Hide resolved
| logger.info("Body: {}", template.requestBody().asString()); | ||
| public void encode(Object object, Type bodyType, feign.RequestTemplate template) throws EncodeException { | ||
| if (object == null) { | ||
| template.body((byte[]) null, StandardCharsets.UTF_8); |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting null to (byte[]) is unnecessary. The method accepts a null value directly, so this cast can be removed for cleaner code.
| template.body((byte[]) null, StandardCharsets.UTF_8); | |
| template.body(null, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken care
...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java
Show resolved
Hide resolved
| s_logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided"); | ||
| } catch (Exception e) { | ||
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage()); | ||
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage(), e); |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message only includes the exception message but loses important context about which SVM connection failed. Consider including the SVM name and management LIF in the error message for better troubleshooting: \"Failed to connect to ONTAP cluster at \" + storage.getManagementLIF() + \" for SVM \" + storage.getSvmName() + \": \" + e.getMessage()
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster: " + e.getMessage(), e); | |
| throw new CloudRuntimeException("Failed to connect to ONTAP cluster at " + storage.getManagementLIF() + " for SVM " + storage.getSvmName() + ": " + e.getMessage(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The details are already being logged earlier in the code
rajiv-jain-netapp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple comments are pending
| @Inject private Utility utils; | ||
| @Inject private StoragePoolDetailsDao storagePoolDetailsDao; | ||
| @Inject private PrimaryDataStoreDao storagePoolDao; | ||
| public OntapPrimaryDatastoreDriver() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed. Removed
...olume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/ClusterFeignClient.java
Show resolved
Hide resolved
|
|
||
| @RequestLine("POST /{uuid}/igroups") | ||
| @Headers({"Authorization: {authHeader}", "return_records: {returnRecords}"}) | ||
| OntapResponse<Igroup> addNestedIgroups(@Param("authHeader") String authHeader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending to be addressed
| break; | ||
| case ISCSI: | ||
| parameters.setType(Storage.StoragePoolType.Iscsi); | ||
| path = "iqn.1992-08.com.netapp:" + details.get(Constants.SVM_NAME) + "." + storagePoolName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can put this in constant
Description
This PR includes changes for Primary storage pool creation
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Primary Storage Pool 'SanPrimary' creation has been tested from Management Server



How did you try to break this feature and the system with this change?