Copilot commented on code in PR #13897:
URL: https://github.com/apache/cloudstack/pull/13897#discussion_r3835437884
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -307,10 +442,10 @@ private ExportPolicy
createExportPolicyRequest(AccessGroup accessGroup,String sv
List<ExportRule.ExportClient> exportClients = new ArrayList<>();
List<HostVO> hosts = accessGroup.getHostsToConnect();
for (HostVO host : hosts) {
- String hostStorageIp = host.getStorageIpAddress();
+ String hostStorageIp = host.getStorageIpAddress() != null ?
host.getStorageIpAddress().trim() : null;
String ip = (hostStorageIp != null && !hostStorageIp.isEmpty())
? hostStorageIp
- : host.getPrivateIpAddress();
+ : (host.getPrivateIpAddress() != null ?
host.getPrivateIpAddress().trim() : null);
String ipToUse = ip + "/32";
Review Comment:
createExportPolicyRequest can throw a NullPointerException when a host has
neither storageIpAddress nor privateIpAddress: `ip` becomes null and `ip +
"/32"` will NPE. This can break pool registration / export-policy creation for
partially configured hosts.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -209,19 +262,23 @@ private void rescanIscsiSessions(String iqn, String host,
int port) {
}
}
- private void waitForDiskToBecomeAvailable(String volumeUuid,
KVMStoragePool pool) {
+ private boolean waitForDiskToBecomeAvailable(String volumeUuid,
KVMStoragePool pool) {
int numberOfTries = 10;
int timeBetweenTries = 1000;
+ long deviceSize = 0;
- while (getPhysicalDisk(volumeUuid, pool).getSize() == 0 &&
numberOfTries > 0) {
+ while ((deviceSize = getPhysicalDisk(volumeUuid, pool).getSize()) == 0
&& numberOfTries > 0) {
numberOfTries--;
try {
Thread.sleep(timeBetweenTries);
- } catch (Exception ex) {
- // don't do anything
+ } catch (InterruptedException ex) {
+ logger.warn("Interrupted while waiting for iSCSI device {} to
become available", volumeUuid, ex);
+ return false;
Review Comment:
waitForDiskToBecomeAvailable swallows InterruptedException without restoring
the thread interrupt flag. This can interfere with higher-level
shutdown/timeout handling and leave the thread in an unexpected state.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -178,23 +178,76 @@ boolean handleNodeCreateResult(String result, String
volumeUuid) {
}
/**
- * Checks the result of an iscsiadm login command.
- * Returns true if the login succeeded or session already exists, false on
failure.
+ * Checks existing session state, performs login, and rescans only if the
session already existed.
+ *
+ * Login is always attempted (idempotent). A pre-login session check is
required on Oracle,
+ * where re-login often exits 0; Ubuntu may instead return
ISCSI_ERR_SESS_EXISTS (15).
+ * Session-preexisted must be treated as success first: on Ubuntu,
re-login exits 15 with a
+ * non-null error message that would otherwise be treated as failure.
+ *
+ * @return true if login succeeded (and rescan ran when needed), false on
login failure
*/
- boolean handleLoginResult(String result, String volumeUuid) {
- if (result == null) {
- logger.debug("Successfully logged in to iSCSI target {}",
volumeUuid);
+ private boolean loginOrRescanExistingSession(String iqn, String host, int
port, String volumeUuid) {
+ boolean sessionAlreadyActive = isIscsiSessionActive(iqn, host, port);
+ logger.debug("iSCSI session active check for target {} at {}:{}: {}",
iqn, host, port, sessionAlreadyActive);
+
+ Script iScsiAdmCmd = new Script(true, "iscsiadm", 0, logger);
+ iScsiAdmCmd.add("-m", "node");
+ iScsiAdmCmd.add("-T", iqn);
+ iScsiAdmCmd.add("-p", host + ":" + port);
+ iScsiAdmCmd.add("--login");
+
+ String result = iScsiAdmCmd.execute();
+ boolean sessionPreExisted = (iScsiAdmCmd.getExitValue() ==
ISCSI_SESSION_EXISTS_CODE) || sessionAlreadyActive;
+
+ if (sessionPreExisted) {
+ logger.debug("iSCSI session for target {} at {}:{} pre-existed,
performing rescan", iqn, host, port);
+ rescanIscsiSessions(iqn, host, port);
return true;
}
- String msg = result.toLowerCase();
- if (msg.contains("already present") || msg.contains("already logged
in") || msg.contains("session exists")) {
- logger.debug("iSCSI session already exists for target {},
proceeding", volumeUuid);
+ if (result == null) {
+ logger.debug("Successfully logged in to iSCSI target {}",
volumeUuid);
return true;
}
logger.debug("Failed to log in to iSCSI target {}: {}", volumeUuid,
result);
return false;
}
+ /**
+ * Checks whether a session to the given target and portal is already
established.
+ *
+ * "iscsiadm -m session" exits with ISCSI_ERR_NO_OBJS_FOUND when no
session exists, which is a
+ * normal outcome here. Any other non-zero exit is logged and treated as
not confirmed active.
+ */
+ private boolean isIscsiSessionActive(String iqn, String host, int port) {
+ Script sessionCmd = new Script(true, "iscsiadm", 0, logger);
+ sessionCmd.add("-m", "session");
+
+ OutputInterpreter.AllLinesParser parser = new
OutputInterpreter.AllLinesParser();
+ sessionCmd.executeIgnoreExitValue(parser, ISCSI_ERR_NO_OBJS_FOUND);
+ int exitValue = sessionCmd.getExitValue();
+ if (exitValue != 0 && exitValue != ISCSI_ERR_NO_OBJS_FOUND) {
+ logger.warn("Unable to determine iSCSI session state for target {}
at {}:{}: 'iscsiadm -m session' exited with {}",
+ iqn, host, port, exitValue);
+ return false;
+ }
+
+ String sessions = parser.getLines();
+ if (StringUtils.isBlank(sessions)) {
+ return false;
+ }
+ // AllLinesParser uses BufferedReader.readLine() (strips \n, \r\n, and
\r) and then
+ // appends "\n" after each session. split("\n") depends on that
separator to walk
+ // one session per line when multiple sessions are listed.
+ for (String line : sessions.split("\n")) {
+ if (line.contains(iqn) && line.contains(host)) {
+ return true;
+ }
+ }
Review Comment:
isIscsiSessionActive only checks `line.contains(host)` and ignores the
portal port, which can return a false-positive when the same target IQN is
logged in to the same host on a different port. That can cause an unnecessary
rescan path and mask a real login failure.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -131,54 +156,67 @@ public boolean connect() {
logger.error("ISCSI protocol is not enabled on SVM " +
svmName);
throw new CloudRuntimeException("ISCSI protocol is not enabled
on SVM " + svmName);
}
- List<Aggregate> aggrs = svm.getAggregates();
- if (aggrs == null || aggrs.isEmpty()) {
- logger.error("No aggregates are assigned to SVM " + svmName);
- throw new CloudRuntimeException("No aggregates are assigned to
SVM " + svmName);
- }
- // Collect all online aggregates assigned to the SVM.
Capacity-based selection is
- // intentionally deferred to createStorageVolume(name, size),
which validates the
- // available space against the actual requested volume size.
- 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());
- if (aggrResp == null) {
- logger.warn("Aggregate details response is null for
aggregate " + aggr.getName() + ". Skipping.");
- continue;
- }
- if (!Objects.equals(aggrResp.getState(),
Aggregate.StateEnum.ONLINE)) {
- logger.warn("Aggregate " + aggr.getName() + " is not in
online state. Skipping this aggregate.");
- continue;
- }
- logger.debug("Aggregate " + aggr.getName() + " is online and
eligible for volume operations.");
- eligibleAggregates.add(aggr);
- }
- if (eligibleAggregates.isEmpty()) {
- logger.error("No suitable aggregates found on SVM " + svmName
+ " for volume operations.");
- throw new CloudRuntimeException("No suitable aggregates found
on SVM " + svmName + " for volume operations.");
+ this.resolvedSvmUuid = svm.getUuid();
+
+ if (validateAggregatesForVolumeCreation) {
+ validateAndSelectAggregatesForVolumeCreation(authHeader,
svmName, svm.getAggregates());
+ } else {
+ logger.debug("Skipping aggregate capacity validation — not
required for existing-volume operations");
}
- this.aggregates = eligibleAggregates;
- logger.info("Found " + eligibleAggregates.size() + " online
aggregate(s) on SVM " + svmName + " for volume operations.");
logger.info("Successfully connected to ONTAP cluster and validated
ONTAP details provided");
+ } catch (CloudRuntimeException e) {
+ throw e;
} catch (FeignException.Unauthorized e) {
- logger.error("Authentication failed while connecting to ONTAP
cluster at " + storage.getStorageIP() +
- ". Please verify the username and password.", e);
- throw new CloudRuntimeException("Authentication failed: Invalid
credentials for ONTAP cluster at " +
- storage.getStorageIP() + ". Please verify the username and
password.");
- } catch (FeignException.Forbidden e) {
- logger.error("Authorization failed while connecting to ONTAP
cluster at " + storage.getStorageIP() +
- ". The user does not have sufficient privileges.", e);
- throw new CloudRuntimeException("Authorization failed: User does
not have sufficient privileges on ONTAP cluster at " +
- storage.getStorageIP() + ". Please verify user
permissions.");
+ String msg = "Authentication failed: Invalid credentials. Please
verify the username and password.";
+ logger.error(msg, e);
+ throw new CloudRuntimeException(msg, e);
} catch (Exception e) {
logger.error("Failed to connect to ONTAP cluster: " +
e.getMessage(), e);
throw new CloudRuntimeException("Failed to connect to ONTAP
cluster: " + e.getMessage(), e);
}
return true;
}
+ /**
+ * ONTAP SVM UUID resolved during the last successful {@link
#connect(boolean)} call.
+ */
+ public String getResolvedSvmUuid() {
+ return resolvedSvmUuid;
+ }
+
+ 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);
+ }
+ for (Aggregate aggr : aggrs) {
+ 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));
+ if (aggrResp == null) {
+ logger.warn("Aggregate details response is null for aggregate
" + aggr.getName() + ". Skipping.");
+ continue;
+ }
+ if (!Objects.equals(aggrResp.getState(),
Aggregate.StateEnum.ONLINE)) {
+ logger.warn("Aggregate " + aggr.getName() + " is not in online
state. Skipping this aggregate.");
+ continue;
+ } else if (aggrResp.getSpace() == null ||
aggrResp.getAvailableBlockStorageSpace() == null ||
+ aggrResp.getAvailableBlockStorageSpace() <=
storage.getSize().doubleValue()) {
+ logger.warn("Aggregate " + aggr.getName() + " does not have
sufficient available space. Skipping this aggregate.");
+ continue;
+ }
+ logger.info("Selected aggregate: " + aggr.getName() + " for volume
operations.");
+ this.aggregates = List.of(aggr);
+ }
Review Comment:
validateAndSelectAggregatesForVolumeCreation overwrites `this.aggregates` on
every eligible aggregate, ending up with the *last* eligible aggregate only.
That changes aggregate selection from “max available space”
(createStorageVolume’s behavior) to an arbitrary last match, which can lead to
provisioning on a suboptimal aggregate and incorrect chosenAggregateNode for
LIF affinity.
--
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]
