Skip to content

Refactor code to choose aggregate, network interface and creating storage volume; Also, the corresponding UT changes #89

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sandeeplocharla wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor code to choose aggregate, network interface and creating storage volume; Also, the corresponding UT changes #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4bee534
Refactor code to choose aggregate, network interface and creating sto…
sandeeplocharla Aug 10, 2026
1205458
Added missing UTs
sandeeplocharla Aug 10, 2026
195a052
Addressed review comments
sandeeplocharla Aug 12, 2026
92bd9ce
Committed missed test files
sandeeplocharla Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 24 additions & 15 deletions ...src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.datastore.lifecycle.BasePrimaryDataStoreLifeCycleImpl;
import org.apache.cloudstack.storage.feign.model.Aggregate;
import org.apache.cloudstack.storage.feign.model.OntapStorage;
import org.apache.cloudstack.storage.feign.model.Volume;
import org.apache.cloudstack.storage.provider.StorageProviderFactory;
Expand All @@ -67,7 +68,6 @@
import com.cloud.storage.StorageManager;
import com.cloud.storage.StoragePool;
import com.cloud.storage.StoragePoolAutomation;
import com.cloud.utils.Pair;
import com.cloud.utils.exception.CloudRuntimeException;
import com.google.common.base.Preconditions;

Expand Down Expand Up @@ -143,10 +143,30 @@ public DataStore initialize(Map<String, Object> dsInfos) {
if (storageStrategy.getResolvedSvmUuid() != null && !storageStrategy.getResolvedSvmUuid().isEmpty()) {
details.put(OntapStorageConstants.SVM_UUID, storageStrategy.getResolvedSvmUuid());
}
Aggregate aggregate;
try {
aggregate = storageStrategy.chooseAggregate(capacityBytes);

@suryag1201 suryag1201 Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should have getAggrs method in strategy and have a selection logic here, same as getNetworkInterface

} catch (Exception e) {
logger.error("Exception occurred while choosing aggregate for pool: " + storagePoolName, e);
throw new CloudRuntimeException("Failed to choose ONTAP aggregate for pool: " + storagePoolName
+ ". Error: " + e.getMessage(), e);
}

Map<String, String> lifResult;
try {
lifResult = storageStrategy.getNetworkInterface(aggregate);
} catch (Exception e) {
logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e);
throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP: " + e.getMessage(), e);
}
String dataLif = lifResult.get(OntapStorageConstants.DATA_LIF);
String lifWarning = lifResult.get(OntapStorageConstants.LIF_WARNING);
processDataLifSelection(dataLif, lifWarning, details, storagePoolName, zoneId, podId);

@rajiv-jain-netapp rajiv-jain-netapp Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Execute this workflow using the direct SVM scenario as well and validate the behaviour. We need to confirm whether aggregate-related operations are handled correctly and whether the same set of APIs can be used without any issues.

@sandeeplocharla sandeeplocharla Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Rajiv.

logger.info("Creating ONTAP volume '" + storagePoolName + "' with size: " + capacityBytes + " bytes (" +
(capacityBytes / (1024 * 1024 * 1024)) + " GB)");
try {
Volume volume = storageStrategy.createStorageVolume(storagePoolName, capacityBytes);
Volume volume = storageStrategy.createStorageVolume(storagePoolName, capacityBytes, aggregate);
if (volume == null) {
logger.error("createStorageVolume returned null for volume: " + storagePoolName);
throw new CloudRuntimeException("Failed to create ONTAP volume: " + storagePoolName);
Expand All @@ -158,15 +178,6 @@ public DataStore initialize(Map<String, Object> dsInfos) {
logger.error("Exception occurred while creating ONTAP volume: " + storagePoolName, e);
throw new CloudRuntimeException("Failed to create ONTAP volume: " + storagePoolName + ". Error: " + e.getMessage(), e);
}

Pair<String, String> lifResult;
try {
lifResult = storageStrategy.getNetworkInterface();
} catch (Exception e) {
logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e);
throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP: " + e.getMessage(), e);
}
processDataLifSelection(lifResult, details, storagePoolName, zoneId, podId);
} else {
throw new CloudRuntimeException("ONTAP details validation failed, cannot create primary storage");
}
Expand Down Expand Up @@ -282,18 +293,16 @@ private long validateInitializeInputs(Long capacityBytes, Long podId, Long clust
return capacityBytes;
}

private void processDataLifSelection(Pair<String, String> lifResult, Map<String, String> details,
private void processDataLifSelection(String dataLIF, String lifWarning, 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");
}
logger.info("Using Data LIF for storage access: " + dataLIF);
details.put(OntapStorageConstants.DATA_LIF, dataLIF);

// Persist LIF warning as a pool detail and fire a storage alert so the user is informed
if (lifResult.second() != null) {
String lifWarning = lifResult.second();
if (lifWarning != null) {
details.put(OntapStorageConstants.LIF_WARNING, lifWarning);
logger.warn("LIF selection warning for pool '" + storagePoolName + "': " + lifWarning);
String alertSubject = "ONTAP Storage Pool '" + storagePoolName + "': "
Expand Down
159 changes: 96 additions & 63 deletions ...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import com.cloud.utils.Pair;
import com.cloud.utils.exception.CloudRuntimeException;

import feign.FeignException;
Expand All @@ -77,12 +76,6 @@ public abstract class StorageStrategy {

protected OntapStorage storage;

/**
* Holds the node name of the aggregate chosen during createStorageVolume().
* Used by getNetworkInterface() to prefer a LIF homed on the same node.
*/
private String chosenAggregateNode;

/**
* Presents aggregate object for the unified storage, not eligible for disaggregated
*/
Expand Down Expand Up @@ -220,19 +213,16 @@ private void validateAndSelectAggregatesForVolumeCreation(String authHeader, Str
// Common methods like create/delete etc., should be here

/**
* Creates ONTAP Flex-Volume
* Eligible only for Unified ONTAP storage
* throw exception in case of disaggregated ONTAP storage
* Selects the best aggregate for a volume of the given size from candidates populated by
* {@link #connect(boolean)} with aggregate validation enabled.
*
* @param volumeName the name of the volume to create
* @param size the size of the volume in bytes
* @return the created Volume object
* <p>Picks the online aggregate with the largest available block space that can fit
* {@code size}. The returned aggregate includes node information for LIF affinity.</p>
*
* @param size requested volume size in bytes
* @return the chosen aggregate detail response
*/
public Volume createStorageVolume(String volumeName, Long size) {
logger.info("Creating volume: " + volumeName + " of size: " + size + " bytes");

this.chosenAggregateNode = null;

public Aggregate chooseAggregate(Long size) {
String svmName = storage.getSvmName();
if (aggregates == null || aggregates.isEmpty()) {
logger.error("No aggregates available to create volume on SVM " + svmName);
Expand All @@ -243,18 +233,6 @@ public Volume createStorageVolume(String volumeName, Long size) {
}

String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword());

// Generate the Create Volume Request
Volume volumeRequest = new Volume();
Svm svm = new Svm();
svm.setName(svmName);
Nas nas = new Nas();
nas.setPath(OntapStorageConstants.SLASH + volumeName);

volumeRequest.setName(volumeName);
volumeRequest.setSvm(svm);

// Pick the best aggregate for this specific request (largest available, online, and sufficient space).
long maxAvailableAggregateSpaceBytes = -1L;
Aggregate aggrChosen = null;
for (Aggregate aggr : aggregates) {
Expand Down Expand Up @@ -298,13 +276,55 @@ public Volume createStorageVolume(String volumeName, Long size) {
logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation.");
throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume operations.");
}
logger.info("Selected aggregate: " + aggrChosen.getName() + " for volume operations.");
if (aggrChosen.getNode() == null || aggrChosen.getNode().getName() == null
|| aggrChosen.getNode().getName().isEmpty()) {
logger.error("Selected aggregate " + aggrChosen.getName() + " does not have a node name.");
throw new CloudRuntimeException("Selected aggregate " + aggrChosen.getName()
+ " does not have a node name required for LIF affinity.");
}
logger.info("Selected aggregate: " + aggrChosen.getName() + " on node "
+ aggrChosen.getNode().getName() + " for volume operations.");
return aggrChosen;
}

this.chosenAggregateNode = aggrChosen.getNode() != null ? aggrChosen.getNode().getName() : null;
/**
* Creates ONTAP Flex-Volume on the given aggregate.
* Eligible only for Unified ONTAP storage
* throw exception in case of disaggregated ONTAP storage
*
* @param volumeName the name of the volume to create
* @param size the size of the volume in bytes
* @param aggregate the aggregate previously selected via {@link #chooseAggregate(Long)}
* @return the created Volume object
*/
public Volume createStorageVolume(String volumeName, Long size, Aggregate aggregate) {
logger.info("Creating volume: " + volumeName + " of size: " + size + " bytes");

String svmName = storage.getSvmName();
if (size == null || size <= 0) {
throw new CloudRuntimeException("Invalid volume size provided: " + size);
}
if (aggregate == null || aggregate.getName() == null || aggregate.getUuid() == null) {
throw new CloudRuntimeException("Aggregate is required to create volume on SVM " + svmName);
}

String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword());

// Generate the Create Volume Request
Volume volumeRequest = new Volume();
Svm svm = new Svm();
svm.setName(svmName);
Nas nas = new Nas();
nas.setPath(OntapStorageConstants.SLASH + volumeName);

volumeRequest.setName(volumeName);
volumeRequest.setSvm(svm);

logger.info("Creating volume on aggregate: " + aggregate.getName() + " for volume operations.");

Aggregate aggr = new Aggregate();
aggr.setName(aggrChosen.getName());
aggr.setUuid(aggrChosen.getUuid());
aggr.setName(aggregate.getName());
aggr.setUuid(aggregate.getUuid());
volumeRequest.setAggregates(List.of(aggr));
volumeRequest.setSize(size);
volumeRequest.setNas(nas);
Expand Down Expand Up @@ -480,19 +500,26 @@ public String getStoragePath() {

/**
* Selects the best available data LIF for storage I/O, preferring one homed on the same node
* as the chosen aggregate to avoid inter-node traffic.
* as the given aggregate to avoid inter-node traffic.
*
* <p>Selection order:</p>
* <ol>
* <li>LIF whose {@code location.home_node} matches the chosen aggregate's node — no warning</li>
* <li>LIF whose {@code location.home_node} matches the aggregate's node — no warning</li>
* <li>LIF currently running on that node (e.g. after failover) — returned with a warning</li>
* <li>Any UP and enabled LIF — returned with a warning when aggregate node is known</li>
* <li>Any UP and enabled LIF — returned with a warning</li>
* </ol>
*
* @return {@link Pair} where {@code first()} is the LIF's IP address and {@code second()} is
* a warning message (null when no warning)
*/
public Pair<String, String> getNetworkInterface() {
* @param aggregate the aggregate previously selected via {@link #chooseAggregate(Long)};
* must include a node name for LIF affinity
* @return map with {@link OntapStorageConstants#DATA_LIF} set to the LIF IP address, and
* optionally {@link OntapStorageConstants#LIF_WARNING} when a non-ideal LIF was selected
*/
public Map<String, String> getNetworkInterface(Aggregate aggregate) {
if (aggregate == null || aggregate.getNode() == null || aggregate.getNode().getName() == null
|| aggregate.getNode().getName().isEmpty()) {
throw new CloudRuntimeException("Aggregate with a node name is required to select a network interface");
}
String aggregateNode = aggregate.getNode().getName();
String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword());
try {
Map<String, Object> queryParams = new HashMap<>();
Expand Down Expand Up @@ -534,21 +561,19 @@ public Pair<String, String> getNetworkInterface() {
if (!isIPv4Address(iface.getIp().getAddress())) {
continue;
}
if (chosenAggregateNode != null) {
// LIF is homed on the aggregate's node
String homeNode = iface.getLocation() != null && iface.getLocation().getHomeNode() != null
? iface.getLocation().getHomeNode().getName() : null;
if (chosenAggregateNode.equals(homeNode)) {
return new Pair<>(iface.getIp().getAddress(), null);
}
// LIF has failed over and is currently running on the aggregate's node
// (home_node differs). Keep as a candidate; returned with a warning if no match is found earlier.
if (currentNodeInterface == null) {
String currentNode = iface.getLocation() != null && iface.getLocation().getNode() != null
? iface.getLocation().getNode().getName() : null;
if (chosenAggregateNode.equals(currentNode)) {
currentNodeInterface = iface;
}
// LIF is homed on the aggregate's node
String homeNode = iface.getLocation() != null && iface.getLocation().getHomeNode() != null
? iface.getLocation().getHomeNode().getName() : null;
if (aggregateNode.equals(homeNode)) {
return networkInterfaceResult(iface.getIp().getAddress(), null);
}
// LIF has failed over and is currently running on the aggregate's node
// (home_node differs). Keep as a candidate; returned with a warning if no match is found earlier.
if (currentNodeInterface == null) {
String currentNode = iface.getLocation() != null && iface.getLocation().getNode() != null
? iface.getLocation().getNode().getName() : null;
if (aggregateNode.equals(currentNode)) {
currentNodeInterface = iface;
}
}
if (fallbackInterface == null) {
Expand All @@ -564,27 +589,35 @@ public Pair<String, String> getNetworkInterface() {

if (currentNodeInterface != null) {
String ip = currentNodeInterface.getIp().getAddress();
String warning = "No home-node LIF found for aggregate node '" + chosenAggregateNode
String warning = "No home-node LIF found for aggregate node '" + aggregateNode
+ "'; using LIF '" + ip + "' currently running on that node (home node LIF may be down).";
logger.warn(warning);
return new Pair<>(ip, warning);
return networkInterfaceResult(ip, warning);
}

String ip = fallbackInterface.getIp().getAddress();
if (chosenAggregateNode == null) {
return new Pair<>(ip, null);
}
String warning = "No operational LIF found on aggregate's home node '" + chosenAggregateNode
String warning = "No operational LIF found on aggregate's home node '" + aggregateNode
+ "'; using fallback LIF '" + ip + "' on a different node."
+ " I/O will traverse an inter-node path, increasing latency.";
logger.warn(warning);
return new Pair<>(ip, warning);
return networkInterfaceResult(ip, warning);
} catch (CloudRuntimeException e) {
throw e;
} catch (Exception e) {
logger.error("Exception while retrieving network interfaces: ", e);
throw new CloudRuntimeException("Failed to retrieve network interfaces: " + e.getMessage());
}
}

private Map<String, String> networkInterfaceResult(String address, String warning) {
Map<String, String> result = new HashMap<>();
result.put(OntapStorageConstants.DATA_LIF, address);
if (warning != null) {
result.put(OntapStorageConstants.LIF_WARNING, warning);
}
return result;
}

/**
* Returns true if the given IP address string is an IPv4 address.
* IPv6 addresses contain colons; IPv4 addresses do not.
Expand Down
Loading
Loading