Copilot commented on code in PR #12391:
URL: https://github.com/apache/cloudstack/pull/12391#discussion_r2672231523
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/client/ScaleIOGatewayClientImpl.java:
##########
@@ -281,7 +284,11 @@ private <T> T get(final String path, final Class<T> type,
final boolean renewAnd
HttpResponse response = null;
boolean responseConsumed = false;
try {
- while (authenticating); // wait for authentication request (if
any) to complete (and to pick the new session key)
+ while (authenticating) { // wait for authentication request (if
any)
+ // to complete (and to pick the new session key)
+ Thread.yield();
Review Comment:
The busy-wait loop with Thread.yield() can cause excessive CPU usage when
waiting for authentication to complete. Thread.yield() is a hint to the
scheduler but provides no guaranteed waiting period, so the loop may spin very
rapidly consuming CPU cycles. Consider using a small Thread.sleep() instead, or
better yet, using proper synchronization primitives like CountDownLatch or
wait/notify to eliminate busy-waiting entirely.
```suggestion
try {
Thread.sleep(10L);
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
break;
}
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/manager/ScaleIOSDCManagerImpl.java:
##########
@@ -425,20 +424,26 @@ public String getConnectedSdc(Host host, DataStore
dataStore) {
@Override
public boolean isHostSdcConnected(String sdcId, DataStore dataStore, int
waitTimeInSecs) {
long poolId = dataStore.getId();
- logger.debug(String.format("Waiting (for %d secs) for the SDC %s of
the pool %s to connect",
- waitTimeInSecs, sdcId, dataStore));
+ logger.debug("Waiting (for {} secs) for the SDC {} of the pool {} to
connect", waitTimeInSecs, sdcId, dataStore);
int timeBetweenTries = 1000; // Try more frequently (every sec) and
return early if connected
- while (waitTimeInSecs > 0) {
+ for (int i = 0; i < waitTimeInSecs; i++) {
+ logger.debug("Attempt {} of {} for the SDC {} of the pool {} to
connect", i + 1, waitTimeInSecs, sdcId, dataStore);
if (isHostSdcConnected(sdcId, poolId)) {
+ logger.debug("Attempt {} of {} successful for the SDC {} of
the pool {} to connect", i + 1, waitTimeInSecs, sdcId, dataStore);
return true;
}
- waitTimeInSecs--;
try {
Thread.sleep(timeBetweenTries);
} catch (Exception ignore) {
}
}
- return isHostSdcConnected(sdcId, poolId);
+ boolean isConnected = isHostSdcConnected(sdcId, poolId);
+ if (isConnected) {
+ logger.debug("Final attempt succeeded the SDC {} of the pool {} to
connect", sdcId, dataStore);
+ } else {
+ logger.debug("Final attempt failed the SDC {} of the pool {} to
connect", sdcId, dataStore);
Review Comment:
The error message contains grammatical issues. "Final attempt succeeded the
SDC" should be "Final attempt succeeded for the SDC" or "Final attempt to
connect the SDC succeeded". Similarly, "Final attempt failed the SDC" should be
"Final attempt failed for the SDC" or "Final attempt to connect the SDC failed".
```suggestion
logger.debug("Final attempt to connect the SDC {} of the pool {}
succeeded", sdcId, dataStore);
} else {
logger.debug("Final attempt to connect the SDC {} of the pool {}
failed", sdcId, dataStore);
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/client/ScaleIOGatewayClientImpl.java:
##########
@@ -316,7 +323,10 @@ private <T> T post(final String path, final Object obj,
final Class<T> type, fin
HttpResponse response = null;
boolean responseConsumed = false;
try {
- while (authenticating); // wait for authentication request (if
any) to complete (and to pick the new session key)
+ while (authenticating) { // wait for authentication request (if
any)
+ // to complete (and to pick the new session key)
+ Thread.yield();
Review Comment:
The busy-wait loop with Thread.yield() can cause excessive CPU usage when
waiting for authentication to complete. Thread.yield() is a hint to the
scheduler but provides no guaranteed waiting period, so the loop may spin very
rapidly consuming CPU cycles. Consider using a small Thread.sleep() instead, or
better yet, using proper synchronization primitives like CountDownLatch or
wait/notify to eliminate busy-waiting entirely.
```suggestion
try {
Thread.sleep(10L);
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
break;
}
```
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java:
##########
@@ -400,6 +400,13 @@ protected T valueInGlobalOrAvailableParentScope(Scope
scope, Long id) {
return value();
}
+ /**
+ * Use {@link ConfigKey#valueInScope(Scope, Long)} instead.
+ */
+ public T valueInDomain(Long domainId) {
+ return valueInScope(Scope.Domain, domainId);
+ }
Review Comment:
The documentation comment is misleading. It states "Use
ConfigKey#valueInScope(Scope, Long) instead" which suggests this method is
deprecated, but there is no @Deprecated annotation. If this is a deprecated
method meant to guide users to the preferred API, it should have the
@Deprecated annotation. If it's just a convenience method, the documentation
should clarify that rather than suggesting users should use a different method.
##########
plugins/storage/volume/scaleio/src/test/java/org/apache/cloudstack/storage/datastore/client/ScaleIOGatewayClientImplTest.java:
##########
@@ -110,25 +108,31 @@ public void testClientAuthSuccess() {
client.listStoragePools();
+ wireMockRule.verify(getRequestedFor(urlEqualTo("/api/login"))
+ .withBasicAuth(new BasicCredentials(username, password)));
+
wireMockRule.verify(getRequestedFor(urlEqualTo("/api/types/StoragePool/instances"))
.withBasicAuth(new BasicCredentials(username, sessionKey)));
}
@Test(expected = CloudRuntimeException.class)
public void testClientAuthFailure() throws Exception {
+ Assert.assertNotNull(client);
+
wireMockRule.stubFor(get("/api/login")
.willReturn(unauthorized()
.withHeader("content-type",
"application/json;charset=UTF-8")
.withBody("")));
- new ScaleIOGatewayClientImpl("https://localhost/api";, username,
password, false, timeout, maxConnections);
+ client.listStoragePools();
+
+ wireMockRule.verify(getRequestedFor(urlEqualTo("/api/login"))
+ .withBasicAuth(new BasicCredentials(username, password)));
Review Comment:
The verification call at lines 129-130 is unreachable code. Since the test
expects a CloudRuntimeException to be thrown at line 127, execution will not
reach the verification statement. This verification should be removed as it
serves no purpose and creates misleading test code.
```suggestion
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/util/ScaleIOUtil.java:
##########
@@ -226,14 +231,10 @@ public static boolean removeMdms(String... mdmAddresses) {
}
} else {
String command = String.format(REMOVE_MDM_CMD_TEMPLATE,
mdmAddress, DRV_CFG_FILE);
- String stdErr = Script.executeCommand(command).second();
- if(StringUtils.isEmpty(stdErr)) {
- // restart SDC needed only if configuration file modified
manually (not by CLI)
- restartSDC = true;
- changesApplied = true;
- } else {
- LOGGER.error(String.format("Failed to remove MDM %s from
%s: %s", mdmAddress, DRV_CFG_FILE, stdErr));
- }
+ Script.runSimpleBashScript(command, DEFAULT_TIMEOUT_MS);
+ // restart SDC needed only if configuration file modified
manually (not by CLI)
+ restartSDC = true;
+ changesApplied = true;
Review Comment:
The error handling from the original code has been removed. Previously, the
code checked if stderr was empty to determine success, and logged an error if
the command failed. Now the code assumes the command always succeeds and sets
changesApplied to true unconditionally. This means failures in the sed command
will go undetected, and the method may incorrectly report success when the MDM
was not actually removed from the configuration file.
```suggestion
String result = Script.runSimpleBashScript(command,
DEFAULT_TIMEOUT_MS);
if (result == null) {
LOGGER.error("Failed to remove MDM [{}] from
configuration file using command: {}", mdmAddress, command);
} else {
// restart SDC needed only if configuration file
modified manually (not by CLI)
restartSDC = true;
changesApplied = true;
}
```
##########
plugins/storage/volume/scaleio/src/test/java/org/apache/cloudstack/storage/datastore/client/ScaleIOGatewayClientImplTest.java:
##########
@@ -138,19 +142,23 @@ public void testRequestTimeout() {
.withBody("")));
client.listStoragePools();
+
+ wireMockRule.verify(getRequestedFor(urlEqualTo("/api/login"))
+ .withBasicAuth(new BasicCredentials(username, password)));
Review Comment:
The verification call at lines 146-147 is unreachable code. Since the test
expects a ServerApiException to be thrown at line 144, execution will not reach
the verification statement. This verification should be removed as it serves no
purpose and creates misleading test code.
```suggestion
```
--
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]
