Enhancing storagepool operations and snapshot related fixes in NetApp ONTAP storage plugin

[sandeeplocharla]

Description

This PR has contributions from @rajiv-jain-netapp, @suryag1201, @piyush5netapp, @sathvikaragi, and @sandeeplocharla.

Summary

Bug Fixes

• Fixed NFS storage pool usage after EnterMaintenance and CancelMaintenance operations.
• Restored the PrimaryDataStoreLifeCycle.updateStoragePool() call required by the Resize StoragePool workflow.
• Gracefully handled 404 exceptions during deletion of:
  • ExportPolicy
  • ONTAP volumes
  • ONTAP snapshots
• Added validation to ensure that only Active IP addresses are selected during storage pool creation.
• Enabled instance and volume snapshots for volumes created with Create on storage.
• Added support for creating NFS3 and iSCSI primary storage pools with a minimum capacity of 20 MB.
• Fixed the issue preventing multiple VMs from being created on Oracle Linux hosts.
• Ensured that iSCSI root disks created using the NetApp ONTAP storage plugin always use the QCOW2 format.

Fixes: #12690

Enhancements

• Hosts added after storage pool creation can now access the storage pool during instance creation.
• Instance snapshots can now include volumes from multiple storage pools belonging to the same ONTAP cluster and SVM.
• Added best-effort data LIF selection during storage pool creation.

Types of Changes

• Breaking change — Changes existing functionality
• New feature — Adds non-breaking functionality
• Bug fix — Fixes an existing issue
• Enhancement — Improves existing functionality
• Cleanup — Code refactoring and cleanup
• 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

---

Detailed Changes

Bug Fixes

1. NFS Storage Pool Usage After Maintenance Operations

NFS storage pools can now be used after executing the EnterMaintenance and CancelMaintenance calls.

Previously

Although the NFS mount was removed, libvirtd still retained the storage pool details.

Now

---

2. Restore the updateStoragePool() Lifecycle Call

Restored the PrimaryDataStoreLifeCycle.updateStoragePool() call in StorageManagerImpl.updateStoragePool().

This call is required by the Resize StoragePool workflow.

Background

• During the updateStoragePool API call, StorageManagerImpl.updateStoragePool() updated the database but did not invoke PrimaryDataStoreLifeCycle.updateStoragePool() on the storage plugin.
• The call was removed as part of Apache CloudStack PR #7889.
• This impacted storage vendors that implemented the API.
• This PR restores the missing lifecycle call.

---

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 Active state.

Previously, an IP address that was not in the Active state could be selected.

Refer to the screenshots in Data LIF Selection During Storage Pool Creation.

---

5. Snapshots for Volumes Created with Create on Storage

Verified that users can take instance and volume snapshots for volumes created with the Create on storage option enabled.

Test Setup

Created a new volume named ADDN-DISK with Create on storage enabled and attached it to the testvm instance.

Additional Disk Created and Attached

Volume Snapshot Created for ADDN-DISK

Corresponding 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 B

Case 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 QCOW2 format when the NetApp ONTAP storage 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

• NFS3: The host gets access to the storage pool immediately after being added to the cluster.
• iSCSI: The host gets access to the storage pool during instance creation.

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 ontapNFS3 and ontapiSCSI Storage Pools

The 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 on ontapNFS3.

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:

Home-node LIF of the selected aggregate > Active LIF on the home node of the selected aggregate > Active LIF on the selected aggregate

Note: The following screenshots were captured for NFS3. The same behavior applies to iSCSI.

Based on the available free space, the plugin selects sti246_vsim_ocvs040d_aggr1 by default.

Scenario 1: pool_P1 — No LIFs Were Down

Result: The first available LIF with matching current and home nodes was selected.

Scenario 2: pool_P2_1 — LIFs on Node 040d Were Down

One LIF had 040d as its current node and 040c as its home node.

Scenario 3: pool_P3 — No LIFs on Node 040d Were Up

The first available LIF was selected from node 040c.

[Copilot]

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.

[Copilot]

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

• updateSnapshotPayload is 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 missing poolType argument (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]

Codecov Report

❌ Patch coverage is 50.42493% with 525 lines in your changes missing coverage. Please review.
✅ Project coverage is 20.46%. Comparing base (68cff97) to head (1bc57d4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ck/storage/vmsnapshot/OntapVMSnapshotStrategy.java	60.35%	93 Missing and 18 partials ⚠️
...cloudstack/storage/listener/OntapHostListener.java	0.00%	80 Missing ⚠️
...hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java	0.00%	58 Missing ⚠️
...ck/storage/driver/OntapPrimaryDatastoreDriver.java	16.07%	44 Missing and 3 partials ⚠️
.../storage/feign/model/ConsistencyGroupSnapshot.java	24.59%	45 Missing and 1 partial ⚠️
...he/cloudstack/storage/service/StorageStrategy.java	70.00%	26 Missing and 19 partials ⚠️
...he/cloudstack/storage/utils/OntapStorageUtils.java	44.28%	20 Missing and 19 partials ⚠️
...om/cloud/storage/snapshot/SnapshotManagerImpl.java	0.00%	18 Missing ⚠️
...ache/cloudstack/storage/feign/model/Aggregate.java	50.00%	16 Missing ⚠️
...ain/java/com/cloud/storage/StorageManagerImpl.java	0.00%	14 Missing ⚠️
... and 10 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13897      +/-   ##
============================================
+ Coverage     20.21%   20.46%   +0.25%     
- Complexity    20750    21413     +663     
============================================
  Files          6426     6485      +59     
  Lines        580181   588834    +8653     
  Branches      71033    72143    +1110     
============================================
+ Hits         117287   120513    +3226     
- Misses       450180   455231    +5051     
- Partials      12714    13090     +376     

Flag	Coverage Δ
uitests	3.70% <ø> (+0.01%)	⬆️
unittests	21.77% <50.42%> (+0.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

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 @Test method 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);
        }

[Copilot]

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 against iface, 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.aggregates on 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));

[Copilot]

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() and iface.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;

[Copilot]

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

• processDataLifSelection assumes lifResult is non-null; if getNetworkInterface() 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 and this.aggregates keeps getting overwritten; the final selection therefore depends on the iteration order of svm.getAggregates(). Also, the method currently stores the unvalidated aggr object rather than the fully populated/validated aggrResp returned by ONTAP.

            logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
            this.aggregates = List.of(aggr);
        }

[Copilot]

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.

    }}

[sandeeplocharla]

@sandeeplocharla @chank257 can you please rebase to the latest main branch?

Done! @nvazquez

[chank257]

@sandeeplocharla - anything you need to fix for the above 3 failures?

@nvazquez , @winterhazel - If the above failures are not relevant for this PR, can we get this merged today?

[nvazquez]

Hi @chank257 @sandeeplocharla - smoke tests are looking good but the PR is not ready yet. There is a pending review from @winterhazel - we can check again after his review

[winterhazel]

Overall looks good. I left a few refactoring suggestions and a question.

[winterhazel]

Why was this moved to before the removal from the HA monitor? Can't it result in a situation where the HA monitor has a reference to a storage, but the pool was already unmounted?

[sandeeplocharla]

For NFS, libvirt owns the mount, so deleteStoragePool destroys the pool and libvirt unmounts it. removeStoragePool in haMonitor just runs umount and then drops the pool from the HA map irrespective of the success/failure of the command.

If removeStoragePool in haMonitor runs first, the mount is already gone when libvirt tries to destroy the pool. Destroy fails, the pool stays defined, and when Cancel Maintenance reuses it, the mount fails. So, the pool would be unusable.

This way libvirt could handle the unmount and mount properly. And while deleting the entry from the HA map in haMonitor, the `umount' could be a backup.

(This fixes #12690)

[winterhazel]

@sandeeplocharla I think this specific change (the unmount cancel fix) needs more discussion and testing, as it impacts other storage pool types as well. It fixes the NFS case, but seems to break SharedMountPoint storages. With this change, SMPs will be unmounted by the HA monitor on the next heartbeat after being put into maintenance, resulting in a "Up but unmounted" storage after the maintenance is canceled, since CloudStack doesn't remount them automatically.

Could you remove the changes of #14187 from this PR? Then it's good to merge from my side, and we can discuss this fix in #14187.

[sandeeplocharla]

Hi @winterhazel, _haMonitor.removeStoragePool(uuid) is only called for NetworkFilesystem, so that line is not what unmounts a SharedMountPoint pool. The behaviour change comes from the new LibvirtStorageAdaptor.deleteStoragePool(String, Map) override added in the file below plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java.

I see that NetworkFilesystem, SharedMountPoint, Gluster, Filesystem, and RBD all use LibvirtStorageAdaptor. Before this override, that call used the StorageAdaptor default, which returns true and does not destroy the libvirt pool. The override forwards all five types to deleteStoragePool(uuid), so entering maintenance now destroys the SharedMountPoint libvirt pool too. The pool stays in the HA monitor, and the next heartbeat unmounts it.

The default is not enough for NetworkFilesystem, because libvirt has to handle the pool. The real delete can stay limited to NetworkFilesystem, with the other types left on the old no-op:

@Override public boolean deleteStoragePool(String uuid, Map<String, String> details) { try { KVMStoragePool pool = getStoragePool(uuid); if (pool.getType() == StoragePoolType.NetworkFilesystem) { return deleteStoragePool(uuid); } } catch (CloudRuntimeException e) { logger.debug("Unable to fetch libvirt pool " + uuid + " before delete: " + e); } return StorageAdaptor.super.deleteStoragePool(uuid, details); }

OR

@Override public boolean deleteStoragePool(String uuid, Map<String, String> details) { try { KVMStoragePool pool = getStoragePool(uuid); if (pool.getType() == StoragePoolType.NetworkFilesystem) { return deleteStoragePool(uuid); } } catch (CloudRuntimeException e) { logger.debug("Unable to fetch libvirt pool " + uuid + " before delete: " + e); } return true; }

What do you think?

[sandeeplocharla]

Hi @winterhazel, Thankyou for your review, addressed your comments.

[winterhazel]

@blueorangutan package

[blueorangutan]

@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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19585

[nvazquez]

@blueorangutan test

[blueorangutan]

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-17155)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58360 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13897-t17155-kvm-ol8.zip
Smoke tests completed. 157 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[nvazquez]

Hi @winterhazel is there anything pending to address from your review? Otherwise it looks like it can be merged

[winterhazel]

Hi @winterhazel is there anything pending to address from your review? Otherwise it looks like it can be merged

@nvazquez sorry for the delay, but yes. Please see https://github.com/apache/cloudstack/pull/13897/changes#r4319963488.

[nvazquez]

No problem, thanks @winterhazel!

@sandeeplocharla @chank257 can you please address this comment? https://github.com/apache/cloudstack/pull/13897/changes#r4319963488

[winterhazel]

Also, isn't it better to call this method after pool.setCapacityBytes, pool.setCapacityIops and _storagePoolDao.update, so that .updateStoragePools that fetch the pool's details get the updated values?

[winterhazel]

Suggested change

	details.put(PrimaryDataStoreLifeCycle.CAPACITY_BYTES, String.valueOf(updatedCapacityBytes));
	details.put(PrimaryDataStoreLifeCycle.CAPACITY_BYTES, String.valueOf(updatedCapacityBytes));

Fixing the number of spaces here