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 7 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
7 commits
Select commit Hold shift + click to select a range
c68c746
Refactor code to choose aggregate, network interface and creating sto…
sandeeplocharla Aug 10, 2026
a888647
Added missing UTs
sandeeplocharla Aug 10, 2026
457eb48
Addressed review comments
sandeeplocharla Aug 12, 2026
c767c30
Committed missed test files
sandeeplocharla Aug 12, 2026
2e324d8
Addressed review comments
sandeeplocharla Oct 7, 2026
47f6fb7
Brought back the missing changes
sandeeplocharla Oct 8, 2026
6ecdda0
pre-check failure fix
sandeeplocharla Oct 8, 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 @@ -68,7 +69,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 @@ -147,10 +147,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(storageStrategy.getAggregates(), capacityBytes);
} 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);

@suryag1201 suryag1201 Oct 7, 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.

If volume creation fails, the user already has an alert for a pool that was never created.


Comment thread
sandeeplocharla marked this conversation as resolved.
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 @@ -162,15 +182,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 @@ -289,18 +300,16 @@ private void validateInitializeInputs(Long capacityBytes, Long capacityIops, Lon
}
}

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
177 changes: 112 additions & 65 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 @@ -19,6 +19,7 @@

package org.apache.cloudstack.storage.service;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -62,7 +63,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 Down Expand Up @@ -92,12 +92,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 @@ -355,11 +349,23 @@ public String getResolvedSvmUuid() {
return resolvedSvmUuid;
}

/**
* Aggregates eligible for new FlexVol creation, populated by {@link #connect(boolean)} with aggregate validation enabled.
*/
public List<Aggregate> getAggregates() {
return aggregates;
}

public void setAggregates(List<Aggregate> aggregates) {
this.aggregates = aggregates;
}

private void validateAndSelectAggregatesForVolumeCreation(String authHeader, String svmName, List<Aggregate> aggrs) {
if (aggrs == null || aggrs.isEmpty()) {
logger.error("No aggregates are assigned to SVM " + svmName);
throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName);
}
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(),
Expand All @@ -379,30 +385,28 @@ private void validateAndSelectAggregatesForVolumeCreation(String authHeader, Str
continue;
}
logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
this.aggregates = List.of(aggr);
eligibleAggregates.add(aggr);
}
if (this.aggregates == null || this.aggregates.isEmpty()) {
if (eligibleAggregates.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.");
}
setAggregates(eligibleAggregates);
}

// 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 the given candidate aggregates.
*
* @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 aggregates candidate aggregates, for example {@link #getAggregates()}
* @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(List<Aggregate> aggregates, Long size) {
String svmName = storage.getSvmName();
if (aggregates == null || aggregates.isEmpty()) {
logger.error("No aggregates available to create volume on SVM " + svmName);
Expand All @@ -413,18 +417,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 @@ -452,7 +444,7 @@ public Volume createStorageVolume(String volumeName, Long size) {
final long availableBytes = aggrResp.getAvailableBlockStorageSpace().longValue();
logger.debug("Aggregate " + aggr.getName() + " available bytes=" + availableBytes + ", requested=" + size);

if (availableBytes < size) {
if (availableBytes <= size) {
logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Required=" +
size + " bytes, available=" + availableBytes + " bytes. Skipping this aggregate.");
continue;
Expand All @@ -468,13 +460,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;
}

/**
* 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);

this.chosenAggregateNode = aggrChosen.getNode() != null ? aggrChosen.getNode().getName() : null;
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 @@ -668,19 +702,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)
* @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 Pair<String, String> getNetworkInterface() {
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 @@ -722,21 +763,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 @@ -752,27 +791,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