winterhazel commented on code in PR #13897:
URL: https://github.com/apache/cloudstack/pull/13897#discussion_r4271665135
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -457,10 +457,12 @@ public synchronized boolean
deleteStoragePool(StoragePoolType type, String uuid)
public boolean deleteStoragePool(StoragePoolType type, String uuid,
Map<String, String> details) {
StorageAdaptor adaptor = getStorageAdaptor(type);
+ // For NetworkFilesystem, libvirt will take care of unmounting the nfs
mount. If nfs mount has been removed before libvirt's pool
+ // delete, libvirt will throw an error.
+ boolean deleteStatus = adaptor.deleteStoragePool(uuid, details);
Review Comment:
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?
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java:
##########
Review Comment:
```suggestion
storageStrategy.deleteFlexVolSnapshotForCloudStackVolume(detail.flexVolUuid,
detail.snapshotUuid, detail.snapshotName);
```
Replacing lines 1062-1065 with the new method
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -453,36 +513,93 @@ public String getNetworkInterface() {
throw new CloudRuntimeException("Unsupported protocol:
" + storage.getProtocol());
}
}
- queryParams.put(OntapStorageConstants.FIELDS,
OntapStorageConstants.IP_ADDRESS);
+ queryParams.put(OntapStorageConstants.FIELDS,
+ OntapStorageConstants.IP_ADDRESS +
OntapStorageConstants.COMMA
+ + OntapStorageConstants.STATE + OntapStorageConstants.COMMA
+ + OntapStorageConstants.LIF_ENABLED +
OntapStorageConstants.COMMA
+ + OntapStorageConstants.LIF_LOCATION_HOME_NODE +
OntapStorageConstants.COMMA
+ + OntapStorageConstants.LIF_LOCATION_NODE);
queryParams.put(OntapStorageConstants.RETURN_RECORDS,
OntapStorageConstants.TRUE);
OntapResponse<IpInterface> response =
networkFeignClient.getNetworkIpInterfaces(authHeader,
queryParams);
- if (response != null && response.getRecords() != null &&
!response.getRecords().isEmpty()) {
- IpInterface ipInterface = null;
- // For simplicity, return the first interface's name (Of IPv4
type for NFS3)
- if (storage.getProtocol() == ProtocolType.ISCSI) {
- ipInterface = response.getRecords().get(0);
- } else if (storage.getProtocol() == ProtocolType.NFS3) {
- for (IpInterface iface : response.getRecords()) {
- if (iface.getIp().getAddress().contains(".")) {
- ipInterface = iface;
- break;
+ if (response == null || response.getRecords() == null ||
response.getRecords().isEmpty()) {
+ throw new CloudRuntimeException("No network interfaces found
for SVM " + storage.getSvmName() +
+ " for protocol " + storage.getProtocol());
+ }
+
+ IpInterface currentNodeInterface = null;
+ IpInterface fallbackInterface = null;
+
+ for (IpInterface iface : response.getRecords()) {
+ if (iface == null || !Boolean.TRUE.equals(iface.getEnabled())
|| !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
+ continue;
+ }
+ if (iface.getIp() == null || iface.getIp().getAddress() ==
null) {
+ continue;
+ }
+ 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;
}
}
}
+ if (fallbackInterface == null) {
+ fallbackInterface = iface;
+ }
+ }
- logger.info("Retrieved network interface: " +
ipInterface.getIp().getAddress());
- return ipInterface.getIp().getAddress();
- } else {
- throw new CloudRuntimeException("No network interfaces found
for SVM " + storage.getSvmName() +
- " for protocol " + storage.getProtocol());
+ if (currentNodeInterface == null && fallbackInterface == null) {
+ throw new CloudRuntimeException("No operationally UP and
enabled LIF found for SVM '"
+ + storage.getSvmName() + "' with protocol " +
storage.getProtocol()
+ + " — all " + response.getRecords().size() + " LIF(s)
are either administratively disabled or operationally down");
}
- } catch (FeignException.FeignClientException e) {
+
+ if (currentNodeInterface != null) {
+ String ip = currentNodeInterface.getIp().getAddress();
+ String warning = "No home-node LIF found for aggregate node '"
+ chosenAggregateNode
+ + "'; using LIF '" + ip + "' currently running on that
node (home node LIF may be down).";
+ logger.warn(warning);
+ return new Pair<>(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
+ + "'; 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);
+ } catch (Exception e) {
logger.error("Exception while retrieving network interfaces: ", e);
throw new CloudRuntimeException("Failed to retrieve network
interfaces: " + e.getMessage());
}
}
Review Comment:
```suggestion
return selectDataLif(response.getRecords());
} catch (Exception e) {
logger.error("Exception while retrieving network interfaces: ",
e);
throw new CloudRuntimeException("Failed to retrieve network
interfaces: " + e.getMessage());
}
}
/**
* Classifies the eligible LIFs and selects the best one using the
node-affinity tiers documented in
* {@link #getNetworkInterface()}.
*/
private Pair<String, String> selectDataLif(List<IpInterface> records) {
IpInterface currentNodeInterface = null;
IpInterface fallbackInterface = null;
for (IpInterface iface : records) {
if (!isEligibleDataLif(iface)) {
continue;
}
// LIF is homed on the aggregate's node.
if (chosenAggregateNode != null &&
chosenAggregateNode.equals(lifHomeNode(iface))) {
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 tier 1 match is found.
if (currentNodeInterface == null &&
isRunningOnChosenNode(iface)) {
currentNodeInterface = iface;
}
if (fallbackInterface == null) {
fallbackInterface = iface;
}
}
return buildDataLifResult(records.size(), currentNodeInterface,
fallbackInterface);
}
private boolean isRunningOnChosenNode(IpInterface iface) {
return chosenAggregateNode != null &&
chosenAggregateNode.equals(lifCurrentNode(iface));
}
private boolean isEligibleDataLif(IpInterface iface) {
return iface != null
&& Boolean.TRUE.equals(iface.getEnabled())
&&
OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())
&& iface.getIp() != null
&& iface.getIp().getAddress() != null
&& isIPv4Address(iface.getIp().getAddress());
}
private String lifHomeNode(IpInterface iface) {
return iface.getLocation() != null &&
iface.getLocation().getHomeNode() != null
? iface.getLocation().getHomeNode().getName() : null;
}
private String lifCurrentNode(IpInterface iface) {
return iface.getLocation() != null && iface.getLocation().getNode()
!= null
? iface.getLocation().getNode().getName() : null;
}
private Pair<String, String> buildDataLifResult(int totalRecords,
IpInterface currentNodeInterface,
IpInterface
fallbackInterface) {
if (currentNodeInterface == null && fallbackInterface == null) {
throw new CloudRuntimeException("No operationally UP and enabled
LIF found for SVM '"
+ storage.getSvmName() + "' with protocol " +
storage.getProtocol()
+ " — all " + totalRecords + " LIF(s) are either
administratively disabled or operationally down");
}
if (currentNodeInterface != null) {
String ip = currentNodeInterface.getIp().getAddress();
String warning = "No home-node LIF found for aggregate node '" +
chosenAggregateNode
+ "'; using LIF '" + ip + "' currently running on that
node (home node LIF may be down).";
logger.warn(warning);
return new Pair<>(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
+ "'; 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);
}
```
This section could be extracted in a few methods to improve legibility
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java:
##########
@@ -877,6 +1193,69 @@ static class FlexVolGroupInfo {
}
}
+ /**
+ * Identifies the ONTAP cluster management endpoint and SVM for CG
operations.
+ */
+ static class ConsistencyGroupScope {
+ final String storageIp;
+ final String svmName;
+ final String svmUuid;
+
+ ConsistencyGroupScope(String storageIp, String svmName, String
svmUuid) {
+ this.storageIp = storageIp;
+ this.svmName = svmName;
+ this.svmUuid = svmUuid;
+ }
+
+ boolean matches(ConsistencyGroupScope other) {
+ return other != null && identityKey().equals(other.identityKey());
+ }
+
+ String identityKey() {
+ if (svmUuid != null && !svmUuid.isEmpty()) {
+ return storageIp + "|" + svmUuid;
+ }
+ return storageIp + "|" + svmName;
+ }
+
+ Map<String, Object> toOntapSvmReference() {
Review Comment:
This method is not used. Can we remove it, or do you plan to use it in the
future?
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java:
##########
@@ -66,11 +67,20 @@ public class OntapStorageConstants {
public static final String INITIATORS = "initiators";
public static final String AGGREGATES = "aggregates";
public static final String STATE = "state";
+ public static final String AGGREGATE_NODE = "node";
+ public static final String AGGREGATE_SPACE = "space";
public static final String DATA_NFS = "data_nfs";
public static final String DATA_ISCSI = "data_iscsi";
public static final String IP_ADDRESS = "ip.address";
+ public static final String LIF_ENABLED = "enabled";
+ public static final String LIF_STATE_UP = "up";
+ public static final String LIF_LOCATION_HOME_NODE =
"location.home_node.name";
+ public static final String LIF_LOCATION_NODE = "location.node.name";
+ public static final String LIF_WARNING = "ONTAP_LIF_WARNING";
public static final String SERVICES = "services";
public static final String RETURN_RECORDS = "return_records";
+ public static final String SVM = "svm";
+ public static final String VOLUMES = "volumes";
Review Comment:
These two are not used. Can we remove it, or do you plan to use it in the
future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
