Copilot commented on code in PR #13345:
URL: https://github.com/apache/cloudstack/pull/13345#discussion_r4046648818
##########
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 message formats the entire properties object, which can include
the persisted keystore passphrase. This leaks the same credential on the WARN
path when backoff configuration is invalid; do not include `_properties` in the
message.
##########
engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java:
##########
@@ -52,11 +73,12 @@ public class AgentManagerImplTest {
@Before
public void setUp() throws Exception {
- host = Mockito.spy(new HostVO("some-Uuid"));
- Mockito.when(host.getId()).thenReturn(1L);
+ host = new HostVO("some-Uuid");
+ Mockito.when(host.getId()).thenReturn(1L)
+ FieldUtils.writeField(host, "id", HOST_ID, true);
Review Comment:
`host` is now a real `HostVO`, not a Mockito mock or spy, so this
`when(...)` call throws `MissingMethodInvocationException`; as written it also
lacks a semicolon and prevents compilation. The following reflection write
already sets the ID, so remove the stubbing line.
##########
utils/src/main/java/com/cloud/utils/nio/HandlerFactory.java:
##########
@@ -32,6 +32,6 @@ default int getMaxConcurrentNewConnectionsCount() {
default int getNewConnectionsCount() {
return 0;
}
- default void registerNewConnection(SocketAddress address) {}
- default void unregisterNewConnection(SocketAddress address) {}
+ default void registerNewConnection(InetSocketAddress address) {}
+ default void unregisterNewConnection(InetSocketAddress address) {}
Review Comment:
`HandlerFactory` is a public interface and previously accepted the general
`SocketAddress`; narrowing these default-method parameters to
`InetSocketAddress` breaks existing implementations that override the old
signatures (their methods will no longer override and the new defaults will
run). Keep the public parameter type as `SocketAddress` and narrow only at the
implementation boundary.
##########
framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java:
##########
@@ -64,6 +65,11 @@ public void invalidateRunSession(long id, long runid) {
}
}
+ @Override
+ public List<ManagementServerHostVO> findAllIncludingRemoved() {
+ return listIncludingRemovedBy(ActiveSearch.create());
Review Comment:
`ActiveSearch` filters `removed IS NULL` and requires a recent
`lastUpdateTime`, so this method does not return “all management servers
(including down and removed)” as its new contract says. It excludes removed and
stale/down rows; use the inherited `listAllIncludingRemoved()` instead.
##########
plugins/maintenance/src/main/java/org/apache/cloudstack/maintenance/ManagementServerMaintenanceManagerImpl.java:
##########
@@ -466,9 +466,20 @@ public ManagementServerMaintenanceResponse
prepareForMaintenance(PrepareForMaint
boolean ignoreMaintenanceHosts =
ManagementServerMaintenanceIgnoreMaintenanceHosts.value();
if (indirectAgentLB.haveAgentBasedHosts(msHost.getMsid(),
ignoreMaintenanceHosts)) {
List<String> indirectAgentMsList =
indirectAgentLB.getManagementServerList();
- indirectAgentMsList.remove(msHost.getServiceIP());
- List<String> nonUpMsList = msHostDao.listNonUpStateMsIPs();
- indirectAgentMsList.removeAll(nonUpMsList);
+ // Remove current server by both hostname and IP since the list
could contain either
+ if (msHost.getName() != null) {
+ indirectAgentMsList.remove(msHost.getName());
+ }
+ if (msHost.getServiceIP() != null) {
+ indirectAgentMsList.remove(msHost.getServiceIP());
+ }
Review Comment:
`indirectAgentMsList` contains configured addresses, but this removes the
persisted canonical hostname/IP. For hostname aliases neither removal is
guaranteed to match, so maintenance can send agents a list that still contains
the server being put into maintenance. Resolve the current node to its
configured address before removing it.
##########
framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletHttpHandler.java:
##########
@@ -113,6 +117,14 @@ private void writeResponse(HttpResponse response, int
statusCode, String content
response.setEntity(body);
}
+ private void logRequest(HttpRequest request, String requestBody) {
+ Optional<HttpRequest> requestOpt = Optional.ofNullable(request);
+ Optional<RequestLine> requestLineOpt =
requestOpt.map(HttpRequest::getRequestLine);
+ String method =
requestLineOpt.map(RequestLine::getMethod).orElse(null);
+ String uri = requestLineOpt.map(RequestLine::getUri).orElse(null);
+ logger.debug("{} {} {}", method, uri, requestBody);
Review Comment:
This logs the complete cluster POST body, including the `gsonPackage`
command/answer JSON. Commands on this channel can contain credential fields, so
the new DEBUG statement creates a plaintext secret sink in management-server
logs; log request metadata or redact the payload.
##########
agent/src/main/java/com/cloud/agent/ServerAttache.java:
##########
@@ -0,0 +1,493 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.agent;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.Command;
+import com.cloud.agent.transport.Request;
+import com.cloud.agent.transport.Response;
+import com.cloud.exception.AgentUnavailableException;
+import com.cloud.exception.CloudException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.cloud.utils.nio.Link;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.cloudstack.threadcontext.ThreadContextCommandUtil;
+import
org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.ThreadContext;
+
+import java.io.IOException;
+import java.nio.channels.ClosedChannelException;
+import java.security.SecureRandom;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Random;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * ServerAttache provides basic server communication commands to be
implemented.
+ *
+ * @author mprokopchuk
+ */
+public class ServerAttache {
+ private static final Logger logger =
LogManager.getLogger(ServerAttache.class);
+
+ private static final ScheduledExecutorService s_listenerExecutor =
Executors.newScheduledThreadPool(10,
+ new NamedThreadFactory("ListenerTimer"));
+
+ protected static Comparator<Request> s_reqComparator = (o1, o2) -> {
+ long seq1 = o1.getSequence();
+ long seq2 = o2.getSequence();
+ if (seq1 < seq2) {
+ return -1;
+ } else if (seq1 > seq2) {
+ return 1;
+ } else {
+ return 0;
+ }
+ };
+
+ protected static Comparator<Object> s_seqComparator = (o1, o2) -> {
+ long seq1 = ((Request) o1).getSequence();
+ long seq2 = (Long) o2;
+ if (seq1 < seq2) {
+ return -1;
+ } else if (seq1 > seq2) {
+ return 1;
+ } else {
+ return 0;
+ }
+ };
+
+ private static final Random s_rand = new SecureRandom();
+ protected String _name;
+ private Link _link;
+ protected ConcurrentHashMap<Long, ServerListener> _waitForList;
+ protected ConcurrentHashMap<Long, java.util.concurrent.ScheduledFuture<?>>
_alarmFutures;
+ protected LinkedList<Request> _requests;
+ protected Long _currentSequence;
+ protected long _nextSequence;
+
+ protected ServerAttache(Link link) {
+ _name = link.getIpAddress();
+ _link = link;
+ _waitForList = new ConcurrentHashMap<>();
+ _alarmFutures = new ConcurrentHashMap<>();
+ _requests = new LinkedList<>();
+ _nextSequence = Long.valueOf(s_rand.nextInt(Short.MAX_VALUE)) << 48;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("ServerAttache %s",
ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this,
+ "_name"));
+ }
+
+ public synchronized long getNextSequence() {
+ return ++_nextSequence;
+ }
+
+ protected synchronized void addRequest(Request req) {
+ int index = findRequest(req);
+ assert (index < 0) : "How can we get index again? " + index + ":" +
req.toString();
+ _requests.add(-index - 1, req);
+ }
+
+ protected void cancel(Request req) {
+ cancel(req.getSequence());
+ }
+
+ protected synchronized void cancel(long seq) {
+ logger.debug(log(seq, "Cancelling."));
+
+ ServerListener listener = _waitForList.remove(seq);
+ if (listener != null) {
+ listener.processDisconnect();
+ }
Review Comment:
On an asynchronous send failure, `cancel(seq)` removes the listener but
leaves its scheduled alarm in `_alarmFutures`; unlike `unregisterListener`, it
does not cancel the future. Repeated connection failures can retain one delayed
task per request until the timeout, so cancel the corresponding future here as
well.
##########
core/src/main/java/org/apache/cloudstack/threadcontext/ThreadContextCommandUtil.java:
##########
@@ -0,0 +1,63 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.threadcontext;
+
+import com.cloud.agent.api.Command;
+import com.cloud.utils.StringUtils;
+import org.apache.logging.log4j.ThreadContext;
+
+/**
+ * Utility class for Command-specific MDC operations.
+ * This class handles propagation of MDC values to and from Command objects.
+ *
+ * @author mprokopchuk
+ */
+public class ThreadContextCommandUtil {
+
+ /**
+ * Propagate UUID and log context ID from Command trace context to MDC.
+ *
+ * @param cmd the command containing trace context parameters
+ */
+ public static void propagateContextFromCommand(Command cmd) {
+ if (cmd != null) {
+
ThreadContextUtil.setLogContextId(cmd.getTraceContextParam(ThreadContextUtil.CONTEXT_LOG_ID_KEY));
+
ThreadContextUtil.setUuid(cmd.getTraceContextParam(ThreadContextUtil.CONTEXT_UUID_KEY));
Review Comment:
These helpers only write non-empty values, and agent worker threads do not
clear MDC between requests. A command without trace context can therefore
inherit the UUID or log context from the previous command, misattributing logs
and any subsequently propagated command; clear each MDC key when its command
parameter is absent.
##########
server/src/main/java/org/apache/cloudstack/agent/lb/IndirectAgentLBServiceImpl.java:
##########
@@ -331,7 +332,9 @@ public void propagateMSListToAgents(boolean triggerHostLB) {
zoneHostIds.addAll(hostIds);
}
zoneHostIds.sort(Comparator.comparingLong(x -> x));
- final List<String> avoidMsList = mshostDao.listNonUpStateMsIPs();
+
+ final List<String> avoidMsList = agentManager.getAvoidMsList();
Review Comment:
Hostname mode sends the configured address list to agents, but this avoid
list is built from `mshost.name`, which is the canonical hostname persisted by
ClusterManager. With a configured CNAME/alias that differs from that canonical
name, the down server's name is not present in the agent's list, so it is not
avoided during rebalancing. Build the avoid list in the same configured-address
namespace.
##########
core/src/main/java/com/cloud/agent/api/AgentConnectStatusAnswer.java:
##########
@@ -0,0 +1,65 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package com.cloud.agent.api;
+
+import com.cloud.host.HostVO;
+import com.cloud.host.Status;
+import com.cloud.utils.db.GlobalLock;
+
+/**
+ * Answer for {@link AgentConnectStatusCommand}.
+ *
+ * @author mprokopchuk
+ */
+public class AgentConnectStatusAnswer extends Answer {
+
+ /**
+ * {@link Boolean#TRUE} means host has {@link GlobalLock#lock(int)}
acquired, otherwise {@link Boolean#FALSE},
+ * and null if there is an error during executing {@link
AgentConnectStatusCommand}.
Review Comment:
This documentation reverses the value returned by the implementation:
`AgentManagerImpl` sets it from `GlobalLock.isLockAvailable()`, which is true
when `IS_FREE_LOCK` reports that no server holds the lock. `HostStatusTask`
also treats true as permission to send startup. Document true as “lock
available,” not “lock acquired,” to avoid unsafe changes based on this contract.
--
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]
