Repository navigation
Enhancing storagepool operations and snapshot related fixes in NetApp ONTAP storage plugin - #13897
Open
sandeeplocharla wants to merge 21 commits into
Open
sandeeplocharla wants to merge 21 commits into
sandeeplocharla wants to merge 21 commits into
Conversation
boring-cyborg
Bot
added
component:kvm
component:primary-storage
component:storage
labels
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the NetApp ONTAP primary storage plugin’s snapshot, VM snapshot, and storage pool operations. It refines how CloudStack interacts with ONTAP for snapshot lifecycle (take/delete/revert), improves data LIF selection behavior, and adds consistency-group orchestration to support VM snapshots spanning multiple FlexVols.
Changes:
- Adjusts snapshot handling so ONTAP managed-primary volume snapshots remain on primary storage (skipping secondary-archive flows) and delegates ONTAP snapshot deletion to the strategy layer.
- Implements ONTAP VM snapshots using a temporary consistency group (two-phase start/commit) when VM volumes span multiple FlexVols; uses direct FlexVol snapshots for the single-FlexVol case.
- Improves operational robustness: idempotent “not found” handling for deletes, ONTAP-safe snapshot naming helpers, and node-affinity-aware data LIF selection with optional alerting.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | Skips secondary archiving for ONTAP managed-primary snapshots based on payload location. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java | Delegates ONTAP snapshot delete/revert orchestration to strategy and normalizes snapshot naming/metadata usage. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java | Adds temporary CG two-phase flow for multi-FlexVol VM snapshots; direct snapshot flow for single FlexVol; improves testability. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java | Adds connect(ops-only) mode, node-affinity-aware data LIF selection (returns Pair), job polling helpers, and FlexVol snapshot delete helper. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java | Makes export-policy delete idempotent on 404 and implements updateAccessGroup for export-policy client maintenance. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java | Updates NFS export policy rules on host connect and host removal for NFS pools. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java | Persists SVM UUID and processes data-LIF selection warnings (optionally sends alerts). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java | Adds ops-only strategy connect, ONTAP-safe snapshot naming helpers, and “object not found” detection. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java | Adds constants for SVM UUID, LIF/node-affinity fields, CG flows, and job polling. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java | Adds HostRuleAction (ADD/REMOVE) to drive export policy updates. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java | Adds ONTAP consistency-group and CG snapshot REST endpoints. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/AggregateFeignClient.java | Adds query-map support for requesting specific aggregate fields. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/IpInterface.java | Adds state/enabled/location fields for LIF selection and node affinity. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java | Improves enum JSON serialization/deserialization robustness. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Aggregate.java | Adds node/space setters and node model for aggregate node affinity. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ConsistencyGroup.java | Adds model for CG create/list requests. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ConsistencyGroupSnapshot.java | Adds model for CG snapshot start/commit and list responses. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ConsistencyGroupVolume.java | Adds model for CG volume members. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ConsistencyGroupVolumeProvisioningOptions.java | Adds model for CG volume provisioning options. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java | Expands VM snapshot unit tests to cover single-FlexVol and temporary-CG flows. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java | Adds tests for “object not found” error matching. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java | Adds coverage for updateAccessGroup behavior and 404 delete behavior. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java | Updates tests for new connect(false), LIF selection Pair, and new job/snapshot helpers. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java | Adds tests for data LIF warnings, failures, and alert integration. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java | Extends capabilities assertions and validates KVM format selection behavior. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java | Adds deleteStoragePool(uuid, details) override delegating to uuid-only delete. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java | Adjusts deleteStoragePool ordering and details-aware delete call. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sandeeplocharla
force-pushed
the
netapp-ontap-fixes
branch
from
August 17, 2026 16:42
5260d5f to
f615b6b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:1638
updateSnapshotPayloadis now called with 4 parameters, but the only method in this class still requires(storagePoolId, payload, isKvmAndFileBasedStorage, poolType, clusterId). This is a compile-time error; restore the missingpoolTypeargument (and keep the local variable if needed later).
boolean isKvmAndFileBasedStorage = isHypervisorKvmAndFileBasedStorage(volume, storagePool);
boolean backupSnapToSecondary = isBackupSnapshotToSecondaryForZone(volume.getDataCenterId());
updateSnapshotPayload(volume.getPoolId(), payload, isKvmAndFileBasedStorage, clusterId);
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13897 +/- ##
============================================
+ Coverage 20.66% 20.73% +0.06%
- Complexity 21720 21854 +134
============================================
Files 6492 6496 +4
Lines 590728 591525 +797
Branches 72448 72570 +122
============================================
+ Hits 122101 122664 +563
- Misses 455355 455527 +172
- Partials 13272 13334 +62
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sandeeplocharla
force-pushed
the
netapp-ontap-fixes
branch
from
August 17, 2026 17:48
f615b6b to
10e6f1c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java:1259
- The final
@Testmethod block is indented as if it were nested, which is inconsistent with the rest of the class and can trigger Checkstyle indentation rules.
@Test
void testDeleteFlexVolSnapshotForCloudStackVolume_Feign404_TreatedAsSuccess() {
FeignException notFoundException = mock(FeignException.class);
when(notFoundException.status()).thenReturn(404);
when(snapshotFeignClient.deleteSnapshot(anyString(), eq("fv-uuid-1"), eq("snap-uuid-1")))
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:215
- buildOntapSnapshotName(): when the uniqueness suffix is very long (suffix length >= MAX_SNAPSHOT_NAME_LENGTH), maxBaseLength becomes <= 0 and the code calls normalizedBase.substring(0, maxLength). If normalizedBase is shorter than maxLength (e.g., "snapshot"), this throws StringIndexOutOfBoundsException. This can surface unexpectedly if a caller passes a long suffix (UUIDs, paths, etc.).
int maxLength = OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH;
int maxBaseLength = maxLength - suffix.length();
if (maxBaseLength <= 0) {
return normalizedBase.substring(0, maxLength);
}
sandeeplocharla
force-pushed
the
netapp-ontap-fixes
branch
from
August 20, 2026 04:11
10e6f1c to
1eee0e2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (4)
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:536
- getNetworkInterface() dereferences
iface.getIp().getAddress()without guarding againstiface,iface.getIp(), or the address being null. A partial/unexpected ONTAP response would cause a NullPointerException and prevent pool creation.
for (IpInterface iface : response.getRecords()) {
if (!Boolean.TRUE.equals(iface.getEnabled()) || !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
continue;
}
if (!isIPv4Address(iface.getIp().getAddress())) {
continue;
}
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:449
- createExportPolicyRequest() can build an export client match of "null/32" when a Host has neither storageIpAddress nor privateIpAddress (because
ip + "/32"string-concats null). That would push an invalid client entry to ONTAP.
String hostStorageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : null;
String ip = (hostStorageIp != null && !hostStorageIp.isEmpty())
? hostStorageIp
: (host.getPrivateIpAddress() != null ? host.getPrivateIpAddress().trim() : null);
String ipToUse = ip + "/32";
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java:213
- hostDisconnected() is invoked by StorageManagerImpl.disconnectHostFromSharedPool (server/src/main/java/com/cloud/storage/StorageManagerImpl.java:1935), but this implementation currently does nothing and always returns false. This prevents proper detach/unmount cleanup on host disconnect (e.g., maintenance flows) and the in-code note claiming it isn't called is incorrect.
public boolean hostDisconnected(long hostId, long poolId) {
logger.info("hostDisconnected: Disconnecting host {} from pool {}", hostId, poolId);
// Note: This is not currently being called for NetApp ONTAP storage plugin.
return false;
}
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:213
- validateAndSelectAggregatesForVolumeCreation overwrites
this.aggregateson every eligible aggregate and ends up keeping only the last eligible entry. This prevents createStorageVolume() from selecting the best aggregate across all eligible candidates and can lead to an unintended aggregate being used for pool provisioning.
logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid());
Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid(),
Map.of(OntapStorageConstants.FIELDS, OntapStorageConstants.AGGREGATE_NODE
+ OntapStorageConstants.COMMA + OntapStorageConstants.AGGREGATE_SPACE
+ OntapStorageConstants.COMMA + OntapStorageConstants.STATE));
sandeeplocharla
force-pushed
the
netapp-ontap-fixes
branch
from
August 22, 2026 06:46
1eee0e2 to
12cb57f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:535
- getNetworkInterface assumes
iface.getIp()andiface.getIp().getAddress()are non-null. If ONTAP returns an interface record without an IP payload (or a null address), this will throw a NullPointerException before filtering.
if (!Boolean.TRUE.equals(iface.getEnabled()) || !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
continue;
}
if (!isIPv4Address(iface.getIp().getAddress())) {
continue;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:290
processDataLifSelectionassumeslifResultis non-null; ifgetNetworkInterface()ever returns null (or a future refactor changes behavior), this will throw a NullPointerException before producing the intended CloudRuntimeException. Guard against a null Pair explicitly.
private void processDataLifSelection(Pair<String, String> lifResult, Map<String, String> details,
String storagePoolName, Long zoneId, Long podId) {
String dataLIF = lifResult.first();
if (dataLIF == null || dataLIF.isEmpty()) {
throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP, cannot create primary storage");
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:277
- When catching
InterruptedException, the thread's interrupted status should be restored (Thread.currentThread().interrupt()), otherwise higher-level code may not observe the interrupt and shutdown/cancellation can be delayed.
} catch (InterruptedException ex) {
logger.warn("Interrupted while waiting for iSCSI device {} to become available", volumeUuid, ex);
return false;
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:213
- Inside
validateAndSelectAggregatesForVolumeCreation, once an eligible aggregate is found, the loop continues andthis.aggregateskeeps getting overwritten; the final selection therefore depends on the iteration order ofsvm.getAggregates(). Also, the method currently stores the unvalidatedaggrobject rather than the fully populated/validatedaggrRespreturned by ONTAP.
logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
this.aggregates = List.of(aggr);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:215
- buildOntapSnapshotName(): when suffix length is >= MAX_SNAPSHOT_NAME_LENGTH, maxBaseLength <= 0 and normalizedBase.substring(0, maxLength) can throw if normalizedBase is shorter than maxLength. Guard the substring to avoid StringIndexOutOfBoundsException.
int maxLength = OntapStorageConstants.MAX_SNAPSHOT_NAME_LENGTH;
int maxBaseLength = maxLength - suffix.length();
if (maxBaseLength <= 0) {
return normalizedBase.substring(0, maxLength);
}
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:128
- initialize(): validateInitializeInputs(...) is called twice with the same arguments (once before building parameters, and again immediately after). This is redundant and makes future changes riskier if the method gains side effects.
validateInitializeInputs(capacityBytes, podId, clusterId, zoneId, storagePoolName, providerName, managed, details);
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:801
- jobPollUntilSuccess(): the loop sleeps after every poll, even when the current poll already returned JOB_SUCCESS. This adds an unnecessary delay (one extra poll interval) to otherwise-fast operations. Return immediately when state becomes SUCCESS before sleeping.
jobRetryCount++;
Thread.sleep(sleepTimeInMilliSecs);
}
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:278
- waitForDiskToBecomeAvailable(): InterruptedException is caught and the method returns false, but the thread interrupt flag is not restored. This can break higher-level shutdown/cancellation logic.
try {
Thread.sleep(timeBetweenTries);
} catch (InterruptedException ex) {
logger.warn("Interrupted while waiting for iSCSI device {} to become available", volumeUuid, ex);
return false;
}
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java:957
- File ends with an extra closing brace (
}}), which will not compile.
}}
#82) …Deletion of CS side of snapshot should not fail on not finding ontap snapshot. 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 --> <!--- ******************************************************************************* --> - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] 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) - [ ] Major - [x] Minor - [ ] BLOCKER - [ ] Critical - [ ] Major - [x] Minor - [ ] Trivial Test -1: Ran VM snapshot delete operation when the respective snapshot is not available at ONTAP, it passed. Test -2: Ran VM snapshot delete operation when the respective snapshot is available at ONTAP; it passed Test -3: Ran cloudstack volume snapshot delete workflow when the respective snapshot is not available at ONTAP, it passed. Test -4: Ran cloudstack volume snapshot delete workflow when the respective snapshot is available at ONTAP, it passed. <!-- Please describe in detail how you tested your changes. --> <!-- Include details of your testing environment, and the tests you ran to --> 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 -->
…nux (#88) ### Description Subsequent VM Creation is failing for ISCSI Storage Pool on Oracle Linux <!--- 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) - [x] 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 - [x] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? Tested on both Oracle and Unbuntu and tried creating multiple VMs #### 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 --> --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
…throw an exception in case of invalid capacity bytes instead of adjusting to minimum requirement (#94) This PR corrects the minimum allowed storage pool size from 1.56GB to 20MB as per ONTAP guidelines for FlexVol. Also, this removes the behaviour to silently correct the invalid capacity bytes to min. requirement, instead it'll throw an exception to let the user decide on the corrective action. <!--- ******************************************************************************* --> <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. --> <!--- PLEASE PUT AN 'X' in only **ONE** box --> <!--- ******************************************************************************* --> - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [X] 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) - [ ] Major - [ ] Minor - [ ] BLOCKER - [ ] Critical - [X] Major - [ ] Minor - [ ] Trivial `Case-1: Storage Pool Capacity Bytes = 214B, less than the min. req of 20MB` <img width="1047" height="735" alt="Screenshot 2026-08-20 at 7 08 03 AM" src="https://github.com/user-attachments/assets/f09d23d8-4c74-4222-8b2e-e4b37b3de1b3" /> `Case-2: Storage Pool Capacity Bytes = 20971520B or 20MB` <img width="1047" height="735" alt="Screenshot 2026-08-20 at 7 15 58 AM" src="https://github.com/user-attachments/assets/8b4a47af-0d90-4636-bb48-5d41260a0fad" /> <img width="1047" height="735" alt="Screenshot 2026-08-20 at 7 16 27 AM" src="https://github.com/user-attachments/assets/f0033b69-0390-4fc6-8394-0e67247274be" /> <img width="1103" height="437" alt="Screenshot 2026-08-20 at 7 17 16 AM" src="https://github.com/user-attachments/assets/1bf51bd5-d746-43b6-b99e-fd87515db411" />
…cycle (#90) ### Description While implementing storage pool resize via the updateStoragePool API, StorageManagerImpl.updateStoragePool()updates the database but never calls PrimaryDataStoreLifeCycle.updateStoragePool() on the storage plugin. adding that call in here. 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) - [x] 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 - [x] 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 -->
### 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) - [x] 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 --> --------- Co-authored-by: Srivastava, Piyush <Piyush.Srivastava@netapp.com>
### Description OntapPrimaryDatastoreDriver now returns RAW format for KVM+iSCSI (was always QCOW2), and VolumeServiceImpl.managedCopyBaseImageCallback adds a volume.getFormat() != null guard to prevent the template's format from overwriting it. <img width="836" height="1614" alt="image" src="https://github.com/user-attachments/assets/f74184e8-82d8-4382-894f-e7fa7ad6ea57" /> 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) - [x] 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 - [x] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): <img width="1704" height="200" alt="Screenshot 2026-08-13 at 11 48 46 AM" src="https://github.com/user-attachments/assets/21de01f3-8ed4-4485-9b23-65d310e69440" /> ### How Has This Been Tested? Ran Iscsi automation suite: Everything is green <img width="2100" height="1520" alt="image" src="https://github.com/user-attachments/assets/bcb44bad-ce86-480c-9274-4f128817affa" /> <!-- 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 -->
… cleanup follows the normal delete path. (#127) ### Description This PR is to address a comment received on apache#13897 . `rollbackFlexVolSnapshot` now uses `deleteFlexVolSnapshotForCloudStackVolume` instead of issuing its own snapshot delete. <!-- 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) - [X] 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 - [X] 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 -->
…x for NFS Cancel maintenance (#131) ### Description This PR addresses the comments received on upstream PR apache#13897 to refactor the code written for fixing the Cancel Maintenance where the Pool wasn't getting mounted back in case of NFS. <!--- ******************************************************************************* --> <!--- 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) - [X] 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 - [X] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): <img width="1045" height="339" alt="Screenshot 2026-10-22 at 8 08 49 PM" src="https://github.com/user-attachments/assets/438b7f0e-8b45-4b39-a425-3fb4bd4451ab" /> <img width="1298" height="183" alt="Screenshot 2026-10-22 at 8 10 22 PM" src="https://github.com/user-attachments/assets/58507563-828d-4ff2-b50c-0decd4674578" /> <img width="1049" height="358" alt="Screenshot 2026-10-22 at 8 11 18 PM" src="https://github.com/user-attachments/assets/0ad746fd-137a-4e76-92c8-8bddf3d28692" /> <img width="1302" height="216" alt="Screenshot 2026-10-22 at 8 12 02 PM" src="https://github.com/user-attachments/assets/c0efeadf-a2b6-4328-adb3-928f6bfee618" /> <img width="1048" height="360" alt="Screenshot 2026-10-22 at 8 13 28 PM" src="https://github.com/user-attachments/assets/f6eae7fb-08fc-4d29-94b6-678371f0054c" /> <img width="1301" height="314" alt="Screenshot 2026-10-22 at 8 12 51 PM" src="https://github.com/user-attachments/assets/0eeba7e2-2e98-4e9c-b463-5606f1838776" /> ### How Has This Been Tested? - Created `Primary1` storage pool of `NetworkFileSystem` type using `DefaultPrimary` storage plugin. - Created `testPool` storage pool of `NetworkFileSystem` type using `NetApp ONTAP` storage plugin. - Enter Maintenance on both the pools. - Cancel Maintenance on both the pools. - At each stage, both `mount | grep nfs` and libvirtd's `virst pool-list --all` should reflect the pools appropriately. - Before Enter Maintenance, both the commands should have mount and pool listed respectively. - On Enter Maintenance, neither should be listed. - On Cancel Maintenance, both should be listed again.
Member
@sandeeplocharla ok, these commits were indeed manually tested.
|
sandeeplocharla
force-pushed
the
netapp-ontap-fixes
branch
from
October 28, 2026 01:42
38627db to
a6920de
Compare
Contributor
Author
Thankyou! they are incorporated!! |
winterhazel
approved these changes
Oct 28, 2026
Member
|
@blueorangutan package |
Contributor
Author
Sure, thanks! @winterhazel. |
Member
|
Trying again... @blueorangutan package |
|
@winterhazel 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19782 |
rajiv-jain-netapp
approved these changes
Oct 28, 2026
Contributor
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
winterhazel
added
status:waiting-for-smoketest-results
and removed
status:needs-testing
labels
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR has contributions from @rajiv-jain-netapp, @suryag1201, @piyush5netapp, @sathvikaragi, and @sandeeplocharla.
Summary
Bug Fixes
EnterMaintenanceandCancelMaintenanceoperations.PrimaryDataStoreLifeCycle.updateStoragePool()call required by theResize StoragePoolworkflow.ActiveIP addresses are selected during storage pool creation.Create on storage.QCOW2format.Fixes: #12690
Enhancements
Types of Changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Detailed Changes
Bug Fixes
1. NFS Storage Pool Usage After Maintenance Operations
NFS storage pools can now be used after executing the
EnterMaintenanceandCancelMaintenancecalls.Previously
Although the NFS mount was removed,
libvirtdstill retained the storage pool details.Now
2. Restore the
updateStoragePool()Lifecycle CallRestored the
PrimaryDataStoreLifeCycle.updateStoragePool()call inStorageManagerImpl.updateStoragePool().This call is required by the
Resize StoragePoolworkflow.Background
updateStoragePoolAPI call,StorageManagerImpl.updateStoragePool()updated the database but did not invokePrimaryDataStoreLifeCycle.updateStoragePool()on the storage plugin.3. Graceful Handling of 404 Exceptions During Delete Workflows
404 exceptions for
ExportPolicy, ONTAP volumes, and ONTAP snapshots are now handled gracefully during their respective delete workflows.4. Validate IP Address Status During Storage Pool Creation
Added validation to ensure that the IP address selected during storage pool creation is in the
Activestate.Previously, an IP address that was not in the
Activestate could be selected.5. Snapshots for Volumes Created with
Create on StorageVerified that users can take instance and volume snapshots for volumes created with the
Create on storageoption enabled.Test Setup
Created a new volume named
ADDN-DISKwithCreate on storageenabled and attached it to thetestvminstance.Additional Disk Created and Attached
Volume Snapshot Created for
ADDN-DISKCorresponding ONTAP Snapshot
6. Minimum Primary Storage Pool Capacity
Users can now create NFS3 or iSCSI primary storage pools with a minimum capacity of 20 MB.
Case 1: Capacity Less Than the Minimum
Storage Pool Capacity = 214 BCase 2: Capacity Exactly 20 MB
Storage Pool Capacity = 20,971,520 B (20 MB)7. Multiple VMs on Oracle Linux Hosts
Users can now create more than one VM on a host running Oracle Linux.
Previously
Now
8. QCOW2 Format for iSCSI Root Disks
The root disk created during instance creation on an iSCSI primary storage pool now always uses the
QCOW2format when theNetApp ONTAPstorage plugin is used.Enhancements
1. Host Access to Storage Pools Created Before the Host Was Added
A host added after storage pool creation can now access the storage pool and create volumes during instance creation.
Protocol-Specific Behavior
Test Steps
Only One Host Present in the Cluster
Created an NFS3 Storage Pool with Cluster Scope
ONTAP Volume Created for the Storage Pool
Export Policy Rule and Client
Added a New Host
Updated Client in the Export Policy
Removed the Host from the Cluster
Client Removed from the Export Policy
2. Instance Snapshots Across Multiple Storage Pools
Users can now take instance snapshots when the instance has volumes from different storage pools belonging to the same ONTAP cluster and SVM.
Test Steps
Created
ontapNFS3andontapiSCSIStorage PoolsThe storage pools use the NFS3 and iSCSI protocols, respectively.
Created an Instance with Volumes from Both Storage Pools
The root disk was created on
ontapiSCSI, and the data disk was created onontapNFS3.Created an Instance Snapshot
Corresponding ONTAP Snapshot for the Root Disk
Corresponding ONTAP Snapshot for the Data Disk
3. Data LIF Selection During Storage Pool Creation
Added best-effort data LIF selection during storage pool creation.
The selection priority is:
Based on the available free space, the plugin selects
sti246_vsim_ocvs040d_aggr1by default.Scenario 1:
pool_P1— No LIFs Were DownResult: The first available LIF with matching current and home nodes was selected.
Scenario 2:
pool_P2_1— LIFs on Node040dWere DownOne LIF had
040das its current node and040cas its home node.Scenario 3:
pool_P3— No LIFs on Node040dWere UpThe first available LIF was selected from node
040c.