You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "piyush5netapp (via GitHub)" <gi...@apache.org> on 2026/07/09 16:07:09 UTC
[PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
piyush5netapp opened a new pull request, #13578:
URL: https://github.com/apache/cloudstack/pull/13578
### Description
This PR...
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache closed pull request #13578: Netapp ONTAP bugfixes for 4.23.0
URL: https://github.com/apache/cloudstack/pull/13578
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3558471491
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -251,6 +263,7 @@ public Volume createStorageVolume(String volumeName, Long size) {
volumeRequest.setAggregates(List.of(aggr));
volumeRequest.setSize(size);
volumeRequest.setNas(nas);
+ volumeRequest.setGuarantee(new Volume.Guarantee(Volume.Guarantee.TypeEnum.NONE));
Review Comment:
Yes, thick provisioning is one of the future support we are aiming for but for now we have defaulted it to thin. In future, that will become a user input of choosing between these options.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4970907667
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4934741590
> @piyush5netapp I left some reviews. Also, could you include the suggestion in [#13053 (comment)](https://github.com/apache/cloudstack/pull/13053#issuecomment-4886348787) here?
Yes, I have raised an internal PR with my team(waiting for approval). Will update in next commit. FYI- I have also added a similar Netapp ONTAP provider check in VolumeApiServiceImpl.java .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3579692915
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ if (volumeToAttachStoragePool != null && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())) {
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
+ }
Review Comment:
After the ONTAP-specific reload, _volsDao.findById(...) can return null (e.g., concurrent delete or DB issue). That would cause a NullPointerException later when volumeToAttach is dereferenced (getPath/get_iScsiName/etc.). Add a null-check and fail fast with a clear exception.
##########
plugins/storage/volume/ontap/pom.xml:
##########
@@ -105,21 +103,13 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
- <version>${mockito.version}</version>
+ <version>${cs.mockito.version}</version>
<scope>test</scope>
Review Comment:
This module’s tests use Mockito.mockStatic(...) (static mocking), but the POM no longer includes mockito-inline and there is no mockito-extensions/org.mockito.plugins.MockMaker file in the repo. With only mockito-core on the classpath, static mocking will fail at runtime. Re-add mockito-inline (test scope) or add the inline mock-maker extension file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3568813719
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1363,11 +1364,13 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
- volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
- // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
- String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
- details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
- primaryDataStore.setDetails(details);
+ if (DataStoreProvider.ONTAP_PLUGIN_NAME.equals(primaryDataStore.getStorageProviderName())) {
+ // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
+ String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
Review Comment:
The MANAGED_STORE_TARGET detail used by hypervisor copy/attach logic can be null unless it’s recomputed after grantAccess(). Previously this code always updated it using a safe fallback (iScsiName or UUID), but now it only happens for ONTAP. To avoid null targets for other managed providers, always recompute MANAGED_STORE_TARGET after grantAccess(), while only refreshing volumeInfo from DB for ONTAP.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -337,9 +337,12 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
logger.error("attachZone : Storage Pool not found for id: " + dataStore.getId());
throw new CloudRuntimeException("Storage Pool not found for id: " + dataStore.getId());
}
-
+ if (!Hypervisor.HypervisorType.KVM.equals(hypervisorType)){
+ logger.error("attachZone : ONTAP primary storage is supported only for KVM hypervisor");
+ throw new CloudRuntimeException("ONTAP primary storage is supported only for KVM hypervisor");
+ }
Review Comment:
Missing space before the opening brace in this newly added if-statement is inconsistent with the surrounding formatting and may trigger style checks.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ if(DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())){
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
+ }
Review Comment:
Missing spaces around the if-condition and opening brace deviates from the surrounding style and may trip formatting/checkstyle rules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4955509669
> Could you please fix these two issues as soon as possible in a separated PR ?
>
> Thanks!
@weizhouapache This has been fixed in latest diff. Please cross-check once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3559518159
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -337,9 +337,16 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
logger.error("attachZone : Storage Pool not found for id: " + dataStore.getId());
throw new CloudRuntimeException("Storage Pool not found for id: " + dataStore.getId());
}
+ if (!Hypervisor.HypervisorType.KVM.equals(hypervisorType)){
+ logger.error("attachZone : ONTAP primary storage is supported only for KVM hypervisor");
+ throw new CloudRuntimeException("ONTAP primary storage is supported only for KVM hypervisor");
+ }
+ storagePool.setHypervisor(hypervisorType);
+ storagePoolDao.update(storagePool.getId(),storagePool);
+ logger.debug("attachZone : Set Hypervisor type for storage pool {} to {}", storagePool.getName(), hypervisorType);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "sandeeplocharla (via GitHub)" <gi...@apache.org>.
sandeeplocharla commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4950323847
> @piyush5netapp
>
> I got two issues with the plugin
>
> * the unit tests are always run even if `-DskipTests` flag is passed.
>
> can you please remove `skiptTests` in pom.xml ?
>
> ```
> # grep skipTests plugins/storage/volume/ontap/pom.xml
> <skipTests>false</skipTests>
> ```
>
> * there are a lot of test failures when build the plugin with openjdk 21
>
> ```
> JAVA_HOME=/usr/lib/jvm/java-21-openjdk-amd64 mvn -T8 -P developer,systemvm clean install -Dnoredist -pl plugins/storage/volume/ontap/
> ```
>
> no issues when build with openjdk 17.
>
> could you please check ? the issue seems related to mockito versions which are too old.
>
> ```
> <mockito.version>3.12.4</mockito.version>
> <mockito-junit-jupiter.version>5.2.0</mockito-junit-jupiter.version>
> ```
Hi @winterhazel
Thankyou, will take care of `skipTests` being set to `false` in pom.xml.
Quick question on Java 21 JDK. We've noticed that the 4.22 documentation recommends Java 17 [https://docs.cloudstack.apache.org/en/latest/upgrading/upgrade/_java_version.html]. So, was wondering, what's the recommended Java version for v4.23 and above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4969508165
@piyush5netapp
can you please address the review comments asap ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3575995018
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ if(DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())){
Review Comment:
```suggestion
if (volumeToAttachStoragePool != null && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())) {
```
Thanks for already adding the check here. A single remark: it would be nice to include a `volumeToAttachStoragePool != null` here to prevent a NPE. According to L5074, `volumeToAttachStoragePool` is null when the VM we are attaching the disk to has never been started before.
(also, when `volumeToAttachStoragePool` is null, `volService.grantAccess(...)` does not do anything because it will not be able to find the provider's driver (current behavior), so this extra condition should not be a problem for the ONTAP plugin)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3569406386
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -135,31 +136,42 @@ public boolean connect() {
logger.error("No aggregates are assigned to SVM " + svmName);
throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName);
}
+ // Collect all online aggregates assigned to the SVM. Capacity-based selection is
+ // intentionally deferred to createStorageVolume(name, size), which validates the
+ // available space against the actual requested volume size.
+ List<Aggregate> eligibleAggregates = new ArrayList<>();
for (Aggregate aggr : aggrs) {
logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid());
Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid());
if (aggrResp == null) {
logger.warn("Aggregate details response is null for aggregate " + aggr.getName() + ". Skipping.");
- break;
+ continue;
}
if (!Objects.equals(aggrResp.getState(), Aggregate.StateEnum.ONLINE)) {
logger.warn("Aggregate " + aggr.getName() + " is not in online state. Skipping this aggregate.");
continue;
- } else if (aggrResp.getSpace() == null || aggrResp.getAvailableBlockStorageSpace() == null ||
- aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) {
- logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Skipping this aggregate.");
- continue;
}
- logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
- this.aggregates = List.of(aggr);
- break;
+ logger.debug("Aggregate " + aggr.getName() + " is online and eligible for volume operations.");
+ eligibleAggregates.add(aggr);
}
- if (this.aggregates == null || this.aggregates.isEmpty()) {
- logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation.");
- throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume creation.");
+ if (eligibleAggregates.isEmpty()) {
+ logger.error("No suitable aggregates found on SVM " + svmName + " for volume operations.");
+ throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume operations.");
}
+ this.aggregates = eligibleAggregates;
+ logger.info("Found " + eligibleAggregates.size() + " online aggregate(s) on SVM " + svmName + " for volume operations.");
logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided");
+ } catch (FeignException.Unauthorized e) {
+ logger.error("Authentication failed while connecting to ONTAP cluster at " + storage.getStorageIP() +
+ ". Please verify the username and password.", e);
+ throw new CloudRuntimeException("Authentication failed: Invalid credentials for ONTAP cluster at " +
+ storage.getStorageIP() + ". Please verify the username and password.");
Review Comment:
The Unauthorized handler logs the underlying FeignException but then throws a new CloudRuntimeException without preserving the cause, which makes downstream troubleshooting (and error aggregation) harder when logs are not available. Prefer using the (message, cause) constructor so the original exception is retained.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -135,31 +136,42 @@ public boolean connect() {
logger.error("No aggregates are assigned to SVM " + svmName);
throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName);
}
+ // Collect all online aggregates assigned to the SVM. Capacity-based selection is
+ // intentionally deferred to createStorageVolume(name, size), which validates the
+ // available space against the actual requested volume size.
+ List<Aggregate> eligibleAggregates = new ArrayList<>();
for (Aggregate aggr : aggrs) {
logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid());
Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid());
if (aggrResp == null) {
logger.warn("Aggregate details response is null for aggregate " + aggr.getName() + ". Skipping.");
- break;
+ continue;
}
if (!Objects.equals(aggrResp.getState(), Aggregate.StateEnum.ONLINE)) {
logger.warn("Aggregate " + aggr.getName() + " is not in online state. Skipping this aggregate.");
continue;
- } else if (aggrResp.getSpace() == null || aggrResp.getAvailableBlockStorageSpace() == null ||
- aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) {
- logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Skipping this aggregate.");
- continue;
}
- logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
- this.aggregates = List.of(aggr);
- break;
+ logger.debug("Aggregate " + aggr.getName() + " is online and eligible for volume operations.");
+ eligibleAggregates.add(aggr);
}
- if (this.aggregates == null || this.aggregates.isEmpty()) {
- logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation.");
- throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume creation.");
+ if (eligibleAggregates.isEmpty()) {
+ logger.error("No suitable aggregates found on SVM " + svmName + " for volume operations.");
+ throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume operations.");
}
+ this.aggregates = eligibleAggregates;
+ logger.info("Found " + eligibleAggregates.size() + " online aggregate(s) on SVM " + svmName + " for volume operations.");
logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided");
+ } catch (FeignException.Unauthorized e) {
+ logger.error("Authentication failed while connecting to ONTAP cluster at " + storage.getStorageIP() +
+ ". Please verify the username and password.", e);
+ throw new CloudRuntimeException("Authentication failed: Invalid credentials for ONTAP cluster at " +
+ storage.getStorageIP() + ". Please verify the username and password.");
+ } catch (FeignException.Forbidden e) {
+ logger.error("Authorization failed while connecting to ONTAP cluster at " + storage.getStorageIP() +
+ ". The user does not have sufficient privileges.", e);
+ throw new CloudRuntimeException("Authorization failed: User does not have sufficient privileges on ONTAP cluster at " +
+ storage.getStorageIP() + ". Please verify user permissions.");
Review Comment:
Same as the Unauthorized case: the Forbidden handler discards the original exception when rethrowing, which loses stack/context for callers. Preserve the cause in the CloudRuntimeException.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ if(DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())){
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
+ }
Review Comment:
Minor formatting: this `if` is missing the usual space after the keyword (`if (`), which is the prevailing style in this file (see e.g. `if (host != null)` above).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3579395404
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1363,11 +1364,13 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
- volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
- // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
- String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
- details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
- primaryDataStore.setDetails(details);
+ if (DataStoreProvider.ONTAP_PLUGIN_NAME.equals(primaryDataStore.getStorageProviderName())) {
+ // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
+ String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
+ details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
Review Comment:
This has been specifically added for Netapp ONTAP plugin as it saves extra db call for other vendors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4969847590
> @piyush5netapp can you please address the review comments asap ?
Working on resolving winterhazel comment about introducing the null check. Will update it shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3569397297
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1363,11 +1364,13 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
- volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
- // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
- String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
- details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
- primaryDataStore.setDetails(details);
+ if (DataStoreProvider.ONTAP_PLUGIN_NAME.equals(primaryDataStore.getStorageProviderName())) {
+ // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
+ String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
Review Comment:
This has been introduced specific to Netapp ONTAP plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4970092500
> thanks @piyush5netapp We will freeze the main branch in 2 hours.
@weizhouapache I have pushed a commit now which resolved the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3553132024
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5092,11 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
Review Comment:
After grantAccess(), the code reloads the volume from the DB but doesn’t handle the (unlikely but possible) case where _volsDao.findById(...) returns null. That would cause a NullPointerException later when volumeToAttach is dereferenced (e.g., getPath/get_iScsiName).
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:
##########
@@ -139,10 +139,15 @@ public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String
}
}
- public static String getIgroupName(String svmName, String hostName) {
- //Igroup name format: cs_svmName_hostName
- String sanitizedHostName = hostName.split("\\.")[0].replaceAll("[^a-zA-Z0-9_-]", "_");
- return OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE + sanitizedHostName;
+ public static String getIgroupName(String svmName, String hostUuid) {
+ //Igroup name format: cs_svmName_hostUuid
+ String sanitizedHostUuid = hostUuid.replaceAll("[^a-zA-Z0-9_-]", "_");
+ String igroupName = OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE + sanitizedHostUuid;
+ // ONTAP igroup names are limited to 96 characters; truncate if longer.
+ if (igroupName.length() > OntapStorageConstants.IGROUP_NAME_MAX_LENGTH) {
+ igroupName = igroupName.substring(0, OntapStorageConstants.IGROUP_NAME_MAX_LENGTH);
+ }
+ return igroupName;
}
Review Comment:
getIgroupName() truncates the full "cs_<svm>_<hostUuid>" string as a whole. If svmName is long, this can truncate away most or all of the hostUuid portion, increasing the chance of igroup name collisions across different hosts (and also throws a NullPointerException if hostUuid is null). Consider validating inputs and truncating svmName first so the host UUID remains intact (or, if hostUuid itself is too long, truncate only the UUID).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3558440595
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5092,11 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
Review Comment:
Its unlikely
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4950625430
> >
> >
> >
> > no issues when build with openjdk 17.
> > could you please check ? the issue seems related to mockito versions which are too old.
> > ```
> > <mockito.version>3.12.4</mockito.version>
> > <mockito-junit-jupiter.version>5.2.0</mockito-junit-jupiter.version>
> > ```
>
> Hi @winterhazel Thankyou, will take care of `skipTests` being set to `false` in pom.xml. Quick question on Java 21 JDK. We've noticed that the 4.22 documentation recommends Java 17 [https://docs.cloudstack.apache.org/en/latest/upgrading/upgrade/_java_version.html]. So, was wondering, what's the recommended Java version for v4.23 and above?
@sandeeplocharla
Java 17 is the recommended version, although Java 21 is also supported as a runtime.
At the moment, building with Java 21 is broken due to test failures in the NetApp plugin.
If possible, please use the Mockito version defined in the pom.xml:
```
<cs.mockito.version>5.16.1</cs.mockito.version>
```
Could you please fix these two issues as soon as possible in a separated PR ?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3579269309
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:
##########
@@ -139,10 +139,15 @@ public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String
}
}
- public static String getIgroupName(String svmName, String hostName) {
- //Igroup name format: cs_svmName_hostName
- String sanitizedHostName = hostName.split("\\.")[0].replaceAll("[^a-zA-Z0-9_-]", "_");
- return OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE + sanitizedHostName;
+ public static String getIgroupName(String svmName, String hostUuid) {
+ //Igroup name format: cs_svmName_hostUuid
+ String sanitizedHostUuid = hostUuid.replaceAll("[^a-zA-Z0-9_-]", "_");
+ String igroupName = OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE + sanitizedHostUuid;
+ // ONTAP igroup names are limited to 96 characters; truncate if longer.
+ if (igroupName.length() > OntapStorageConstants.IGROUP_NAME_MAX_LENGTH) {
+ igroupName = igroupName.substring(0, OntapStorageConstants.IGROUP_NAME_MAX_LENGTH);
+ }
+ return igroupName;
}
Review Comment:
we have updated logic to have format cs_hostUuid_svmName.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4970022506
> > @piyush5netapp can you please address the review comments asap ?
>
> Working on resolving winterhazel comment about introducing the null check. Will update it shortly.
thanks @piyush5netapp
We will freeze the main branch in 2 hours.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4932812507
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13578?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.46%. Comparing base ([`33c3967`](https://app.codecov.io/gh/apache/cloudstack/commit/33c3967b6b636ff50a644be8f445c558b5076051?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e377eff`](https://app.codecov.io/gh/apache/cloudstack/commit/e377eff94a212a725fa812c125a359ef2279d567?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (33c3967) and HEAD (e377eff). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (33c3967) | HEAD (e377eff) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13578 +/- ##
=============================================
- Coverage 19.46% 3.46% -16.01%
=============================================
Files 6288 479 -5809
Lines 565692 41229 -524463
Branches 69079 7807 -61272
=============================================
- Hits 110132 1427 -108705
+ Misses 443595 39609 -403986
+ Partials 11965 193 -11772
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13578/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13578/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.46% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13578/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13578?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4950331265
> > @piyush5netapp
> > I got two issues with the plugin
> >
> > * the unit tests are always run even if `-DskipTests` flag is passed.
> >
> > can you please remove `skiptTests` in pom.xml ?
> > ```
> > # grep skipTests plugins/storage/volume/ontap/pom.xml
> > <skipTests>false</skipTests>
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > * there are a lot of test failures when build the plugin with openjdk 21
> >
> > ```
> > JAVA_HOME=/usr/lib/jvm/java-21-openjdk-amd64 mvn -T8 -P developer,systemvm clean install -Dnoredist -pl plugins/storage/volume/ontap/
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > no issues when build with openjdk 17.
> > could you please check ? the issue seems related to mockito versions which are too old.
> > ```
> > <mockito.version>3.12.4</mockito.version>
> > <mockito-junit-jupiter.version>5.2.0</mockito-junit-jupiter.version>
> > ```
>
> Hi @winterhazel Thankyou, will take care of `skipTests` being set to `false` in pom.xml. Quick question on Java 21 JDK. We've noticed that the 4.22 documentation recommends Java 17 [https://docs.cloudstack.apache.org/en/latest/upgrading/upgrade/_java_version.html]. So, was wondering, what's the recommended Java version for v4.23 and above?
Tagging @weizhouapache as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4970315553
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache merged PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4936160855
@winterhazel I have pushed some changes which resolves all of your comment. Please re-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3559567618
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ if(DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())){
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
+ }
Review Comment:
After grantAccess(), volumeToAttach is conditionally reloaded from the DB. If _volsDao.findById(...) returns null (e.g., volume removed concurrently), the subsequent dereferences of volumeToAttach (getPath/get_iScsiName/etc.) will throw a NullPointerException. Add a null-check and fail fast with a clear CloudRuntimeException.
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1363,11 +1364,13 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
- volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
- // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
- String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
- details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
- primaryDataStore.setDetails(details);
+ if (DataStoreProvider.ONTAP_PLUGIN_NAME.equals(primaryDataStore.getStorageProviderName())) {
+ // For Netapp ONTAP iscsiName or Lun path is available only after grantAccess
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
+ String managedStoreTarget = ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
+ details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget);
Review Comment:
createManagedVolumeCopyTemplateAsync() previously refreshed MANAGED_STORE_TARGET after grantAccess() (with a UUID fallback). The new ONTAP-only conditional means non-ONTAP managed storage no longer gets this post-grantAccess update, so PrimaryDataStore.MANAGED_STORE_TARGET can remain null/stale if iScsiName is not populated before grantAccess; hypervisor storage processors (e.g., VMware) use this value directly. Update MANAGED_STORE_TARGET for all providers, while only reloading volumeInfo from DB for ONTAP if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4936468420
@piyush5netapp
I got two issues with the plugin
- the unit tests are always run even if `-DskipTests` flag is passed.
can you please remove `skiptTests` in pom.xml ?
```
# grep skipTests plugins/storage/volume/ontap/pom.xml
<skipTests>false</skipTests>
```
- there are a lot of test failures when build the plugin with openjdk 21
```
JAVA_HOME=/usr/lib/jvm/java-21-openjdk-amd64 mvn -T8 -P developer,systemvm clean install -Dnoredist -pl plugins/storage/volume/ontap/
```
no issues when build with openjdk 17.
could you please check ? the issue seems related to mockito versions which are too old.
```
<mockito.version>3.12.4</mockito.version>
<mockito-junit-jupiter.version>5.2.0</mockito-junit-jupiter.version>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3579386545
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during grantAccess, so the local
+ // volumeToAttach object can be stale.
+ if(DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())){
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
+ }
Review Comment:
will add null check before provider check , resolving it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4970304346
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4971092669
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3555038804
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
Review Comment:
A bit of a corner case, but does it make sense to change this to `availableBytes < size`? The way it is now, aggregates that have exactly the requested size are skipped.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -337,9 +337,16 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
logger.error("attachZone : Storage Pool not found for id: " + dataStore.getId());
throw new CloudRuntimeException("Storage Pool not found for id: " + dataStore.getId());
}
+ if (!Hypervisor.HypervisorType.KVM.equals(hypervisorType)){
+ logger.error("attachZone : ONTAP primary storage is supported only for KVM hypervisor");
+ throw new CloudRuntimeException("ONTAP primary storage is supported only for KVM hypervisor");
+ }
+ storagePool.setHypervisor(hypervisorType);
+ storagePoolDao.update(storagePool.getId(),storagePool);
+ logger.debug("attachZone : Set Hypervisor type for storage pool {} to {}", storagePool.getName(), hypervisorType);
Review Comment:
```suggestion
```
You can pass `hypervisorType` as a second parameter of `_dataStoreHelper.attachZone` at the end of this method.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -251,6 +263,7 @@ public Volume createStorageVolume(String volumeName, Long size) {
volumeRequest.setAggregates(List.of(aggr));
volumeRequest.setSize(size);
volumeRequest.setNas(nas);
+ volumeRequest.setGuarantee(new Volume.Guarantee(Volume.Guarantee.TypeEnum.NONE));
Review Comment:
Out of curiosity, is thick provisioning support planned for the future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "piyush5netapp (via GitHub)" <gi...@apache.org>.
piyush5netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3559507651
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
Review Comment:
Updated in latest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "rajiv-jain-netapp (via GitHub)" <gi...@apache.org>.
rajiv-jain-netapp commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3568070131
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -251,6 +263,7 @@ public Volume createStorageVolume(String volumeName, Long size) {
volumeRequest.setAggregates(List.of(aggr));
volumeRequest.setSize(size);
volumeRequest.setNas(nas);
+ volumeRequest.setGuarantee(new Volume.Guarantee(Volume.Guarantee.TypeEnum.NONE));
Review Comment:
@winterhazel We plan to extend support for thin provisioning, thick provisioning, and additional storage capabilities through the service offering workflows in future releases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Netapp ONTAP bugfixes for 4.23.0 [cloudstack]
Posted by "sandeeplocharla (via GitHub)" <gi...@apache.org>.
sandeeplocharla commented on PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#issuecomment-4951245159
> > > no issues when build with openjdk 17.
> > > could you please check ? the issue seems related to mockito versions which are too old.
> > > ```
> > > <mockito.version>3.12.4</mockito.version>
> > > <mockito-junit-jupiter.version>5.2.0</mockito-junit-jupiter.version>
> > > ```
> >
> >
> > Hi @winterhazel Thankyou, will take care of `skipTests` being set to `false` in pom.xml. Quick question on Java 21 JDK. We've noticed that the 4.22 documentation recommends Java 17 [https://docs.cloudstack.apache.org/en/latest/upgrading/upgrade/_java_version.html]. So, was wondering, what's the recommended Java version for v4.23 and above?
>
> @sandeeplocharla @piyush5netapp Java 17 is the recommended version, although Java 21 is also supported as a runtime.
>
> At the moment, building with Java 21 is broken due to test failures in the NetApp plugin.
>
> If possible, please use the Mockito version defined in the pom.xml:
>
> ```
> <cs.mockito.version>5.16.1</cs.mockito.version>
> ```
>
> Could you please fix these two issues as soon as possible in a separated PR ?
>
> Thanks!
Sure, thankyou. Will raise the diff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org