sandeeplocharla commented on code in PR #13897:
URL: https://github.com/apache/cloudstack/pull/13897#discussion_r4278500509
##########
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:
Yes this is planned to be used in the future.
##########
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:
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)
##########
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:
Yes, will be used in the subsequent PRs.
##########
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:
After internal reviews, we had good amount of refactoring done in this piece
code and have enhanced it further. Will be raising it as part of subsequent PRs.
--
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]
