Copilot commented on code in PR #13345:
URL: https://github.com/apache/cloudstack/pull/13345#discussion_r4037511071
##########
agent/src/main/java/com/cloud/agent/AgentShell.java:
##########
@@ -424,11 +434,15 @@ public void init(String[] args) throws
ConfigurationException {
_properties.put(cmdLineProp.getKey(), cmdLineProp.getValue());
}
- LOGGER.info("Defaulting to the constant time backoff algorithm");
- _backoff = new ConstantTimeBackoff();
- Map<String, Object> map = new HashMap<>();
- map.put("seconds", _properties.getProperty("backoff.seconds"));
- _backoff.configure("ConstantTimeBackoff", map);
+ try {
+ LOGGER.info("Creating backoff delay algorithm implementation");
+ setBackoffAlgorithm(BackoffFactory.create(_properties));
+ LOGGER.info("Created {} delay algorithm implementation",
getBackoffAlgorithm().getClass().getName());
+ } catch (RuntimeException e) {
+ String msg = String.format("Failed to create backoff with provided
properties %s, failing back to default", _properties);
Review Comment:
The fallback warning interpolates the entire `Properties` object, which can
include the keystore passphrase and other secrets. A backoff configuration
error should not cause unrelated agent credentials to be written to a warning
log.
##########
engine/components-api/src/main/java/com/cloud/agent/AgentManager.java:
##########
@@ -178,4 +179,6 @@ enum TapAgentsAction {
boolean transferDirectAgentsFromMS(String fromMsUuid, long fromMsId, long
timeoutDurationInMs, boolean excludeHostsInMaintenance);
int getHostSshPort(HostVO host);
+
+ List<String> getAvoidMsList();
Review Comment:
This adds a new abstract method to `AgentManager`, but
`engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/DirectAgentManagerSimpleImpl.java`
implements that interface without a `getAvoidMsList()` override. Compiling
that integration-test module will fail; update the implementation (or make the
interface method default if that is the intended contract).
##########
utils/src/main/java/com/cloud/utils/nio/Link.java:
##########
@@ -354,16 +376,28 @@ public InetSocketAddress getSocketAddress() {
return _addr;
}
+ public Integer getLocalPort() {
+ return _localPort;
+ }
+
public String getIpAddress() {
return _addr.getAddress().toString();
}
public synchronized void terminated() {
+ if (LOGGER.isTraceEnabled()) {
+ LOGGER.trace("Terminating connection to {}", _addr);
+ }
_key = null;
}
+ public boolean isTerminated() {
+ return _key == null;
+ }
Review Comment:
`isTerminated()` reads `_key` without synchronization, while `setKey()` and
`terminated()` write it under the instance monitor. The reconnect and NIO
threads can therefore observe a stale non-null key and process a link that has
already been terminated. Make this read synchronized or make `_key` safely
visible.
##########
framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletImpl.java:
##########
@@ -146,15 +159,14 @@ private String executePostMethod(final
CloseableHttpClient client, final HttpPos
result = EntityUtils.toString(httpResponse.getEntity());
profiler.stop();
if (logger.isDebugEnabled()) {
- logger.debug("POST " + serviceUrl + " response :" + result
+ ", responding time: " + profiler.getDurationInMillis() + " ms");
+ logger.debug("POST {} request: {}, response :{},
responding time: {} ms", serviceUrl, request, result,
profiler.getDurationInMillis());
Review Comment:
This debug message logs the raw URL-encoded PDU request and response.
`gsonPackage` contains arbitrary serialized agent commands and can carry
sensitive values, so enabling debug logging exposes them in management-server
logs. Log only method/sequence/timing metadata or redact both payloads.
--
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]
