Indirect agent connection improvements - #13345
Conversation
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13345 +/- ##
============================================
+ Coverage 19.73% 19.78% +0.04%
- Complexity 19955 20034 +79
============================================
Files 6371 6382 +11
Lines 575784 577437 +1653
Branches 70478 70661 +183
============================================
+ Hits 113644 114231 +587
- Misses 449788 450782 +994
- Partials 12352 12424 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR is a broad refactor/enhancement of indirect agent connection handling across both Management Server and Agent sides. It introduces a host-side connect/status-check process, adds configurable backoff and host-status heuristics, and expands logging/lock-check mechanisms to reduce reconnect storms and improve observability.
Changes:
- Adds agent-side connection orchestration (status polling + startup submission) and a server-side
AgentConnectStatus*command/answer pair to coordinate connect progress. - Introduces configurable backoff (factory + exponential-with-jitter implementation) and propagates configuration from Management Server to Agent during startup.
- Refactors NIO connection lifecycle and adds supporting utilities/logging, plus DB lock availability checks and DB indexes for performance.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/src/test/java/com/cloud/utils/testcase/NioTest.java | Test logging cleanup (exception logging). |
| utils/src/test/java/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java | Updates config key name for constant backoff. |
| utils/src/main/java/com/cloud/utils/nio/NioServer.java | NIO server init tweaks and docstring. |
| utils/src/main/java/com/cloud/utils/nio/NioConnection.java | Connection lifecycle refactor, selector loop/logging, reject logic. |
| utils/src/main/java/com/cloud/utils/nio/NioClient.java | Client connect/handshake logging and expanded cleanup. |
| utils/src/main/java/com/cloud/utils/nio/Link.java | Adds local-port tracking, richer toString, termination helpers/logging changes. |
| utils/src/main/java/com/cloud/utils/nio/HandlerFactory.java | Changes new-connection registration API to InetSocketAddress. |
| utils/src/main/java/com/cloud/utils/net/NetUtils.java | Null guard in hostname-to-IP resolution helper. |
| utils/src/main/java/com/cloud/utils/LogUtils.java | Adds host logging helper with optional reverse lookup. |
| utils/src/main/java/com/cloud/utils/exception/CSExceptionErrorCode.java | Adds error-code mapping for ConnectionException. |
| utils/src/main/java/com/cloud/utils/DateUtil.java | Adds formatMillis duration formatter. |
| utils/src/main/java/com/cloud/utils/backoff/impl/ExponentialWithJitterBackoffMBean.java | New MBean interface for exponential-jitter backoff. |
| utils/src/main/java/com/cloud/utils/backoff/impl/ExponentialWithJitterBackoff.java | New exponential-with-jitter backoff implementation. |
| utils/src/main/java/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java | Migrates to namespaced config keys + exposes configuration. |
| utils/src/main/java/com/cloud/utils/backoff/BackoffFactory.java | New backoff factory for selecting/configuring algorithms. |
| utils/src/main/java/com/cloud/utils/backoff/BackoffAlgorithm.java | Adds getConfiguration() API for propagating settings. |
| server/src/main/java/org/apache/cloudstack/agent/lb/IndirectAgentLBServiceImpl.java | Adds notes/timeout tweaks around agent migration command dispatch. |
| framework/db/src/main/java/com/cloud/utils/db/GlobalLock.java | Refactors lock bookkeeping/logging and adds lock-availability query. |
| framework/db/src/main/java/com/cloud/utils/db/DbUtil.java | Adds IS_FREE_LOCK helper and improves logging around lock DB connections. |
| framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKeyUtil.java | New utility to parse key=value;... configuration strings. |
| framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java | Adds DAO method to list all MS hosts including removed. |
| framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java | Adds DAO API for “including removed” listing. |
| framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletImpl.java | Adds request logging and more detailed RemoteException messages. |
| framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletHttpHandler.java | Adds debug logging of inbound request line/body. |
| engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql | Adds indexes via idempotent add-index procedure calls. |
| engine/schema/src/main/resources/META-INF/db/procedures/cloud.idempotent_add_index.sql | Adds/defines IDEMPOTENT_ADD_INDEX procedure. |
| engine/orchestration/src/test/java/com/cloud/agent/manager/ClusteredAgentManagerImplTest.java | Expands tests for disconnect broadcast behavior and lock usage. |
| engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java | Adds tests for config keys and deregister/disconnect behaviors. |
| engine/orchestration/src/main/java/com/cloud/agent/manager/ClusteredAgentManagerImpl.java | Refactors attache creation/removal and event handling behavior. |
| engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java | Major connect/disconnect refactor, new config keys, status checks, backoff propagation. |
| engine/orchestration/src/main/java/com/cloud/agent/manager/AgentAttache.java | Cancels listener alarm futures on unregister/cancel paths. |
| core/src/main/java/org/apache/cloudstack/threadcontext/ThreadContextUtil.java | New helper for propagating Log4j ThreadContext across threads. |
| core/src/main/java/com/cloud/resource/ServerResource.java | Changes default isExitOnFailures() behavior. |
| core/src/main/java/com/cloud/agent/transport/Request.java | Improves JSON deserialization error logging. |
| core/src/main/java/com/cloud/agent/api/StartupAnswer.java | Adds params + agent-side status-check delay transport fields. |
| core/src/main/java/com/cloud/agent/api/AgentConnectStatusCommand.java | New command for host connect-status checks. |
| core/src/main/java/com/cloud/agent/api/AgentConnectStatusAnswer.java | New answer carrying lock availability and host status. |
| agent/src/test/java/com/cloud/agent/HostConnectProcessTest.java | New test around scheduling the agent connect process. |
| agent/src/test/java/com/cloud/agent/AgentTest.java | Updates tests for new link/logging and reconnect helpers. |
| agent/src/main/java/com/cloud/agent/SynchronousListener.java | New blocking listener for synchronous waits on answers. |
| agent/src/main/java/com/cloud/agent/ServerListener.java | New listener interface for agent-side ServerAttache callbacks. |
| agent/src/main/java/com/cloud/agent/ServerAttache.java | New agent-side counterpart to MS Attache for command/answer flow. |
| agent/src/main/java/com/cloud/agent/properties/AgentProperties.java | Adds new agent properties for async timeouts and status-check delay. |
| agent/src/main/java/com/cloud/agent/IAgentShell.java | Adds setter for dynamically updated backoff algorithm. |
| agent/src/main/java/com/cloud/agent/HostConnectProcess.java | New connect/status-check orchestration loop on the agent side. |
| agent/src/main/java/com/cloud/agent/AgentShell.java | Uses backoff factory + persists configuration; relaxes version handling. |
| agent/src/main/java/com/cloud/agent/Agent.java | Major reconnect/startup refactor; integrates HostConnectProcess and ServerAttache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18146 |
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18149 |
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18154 |
| /** | ||
| * Command to check status of {@link StartupCommand} from the Agent. | ||
| * | ||
| * @author mprokopchuk | ||
| */ |
There was a problem hiding this comment.
These @author fields are not necessary, right?
| /** | |
| * Command to check status of {@link StartupCommand} from the Agent. | |
| * | |
| * @author mprokopchuk | |
| */ | |
| /** | |
| * Command to check status of {@link StartupCommand} from the Agent. | |
| * | |
| */ |
There was a problem hiding this comment.
@bernardodemarco I think, it's ok to have it. we've author mentioned in few other classes as well. (check with '@author' in the code base).
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
d5a6d20 to
22e832c
Compare
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18190 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
moving to 4.24.0 |
| /** | ||
| * ServerAttache provides basic server communication commands to be implemented. | ||
| * | ||
| * @author mprokopchuk |
There was a problem hiding this comment.
Nit: leftover personal @author mprokopchuk javadoc tag (recurs in several of the other new files here too: SynchronousListener.java, AgentConnectStatusAnswer.java, AgentConnectStatusCommand.java, ThreadContextUtil.java, ConfigKeyUtil.java, BackoffFactory.java, ExponentialWithJitterBackoff.java). Probably copied over from wherever this was originally authored, worth stripping.
| logger.debug(log(seq, "Unable to find listener.")); | ||
| } else { | ||
| processed = monitor.processAnswers(seq, answers); | ||
| logger.trace(log(seq, (processed ? "" : " did not ") + " processed ")); |
There was a problem hiding this comment.
Nit: this produces "did not processed" (double space, missing verb) when not processed.
| listener.processTimeout(_seq); | ||
| } | ||
| } catch (Exception e) { | ||
| ServerAttache.logger.warn("Exception ", e); |
There was a problem hiding this comment.
Nit: vague log message ("Exception " with a trailing space), no context on what actually failed.
| shell.init(args); | ||
| shell.start(); | ||
| } catch (ConfigurationException e) { | ||
| LOGGER.fatal(e.getMessage(), e); |
There was a problem hiding this comment.
Nit: this now logs the same message twice, LOGGER.fatal here followed by System.out.println right below with the same e.getMessage().
| } | ||
|
|
||
| /** | ||
| * Task wait for the Host to be available to connect to submit {@link StartupCommand}. |
There was a problem hiding this comment.
Nit: javadoc typo, "Task wait for the Host" should be "waits".
| * @return | ||
| */ | ||
| public static Runnable wrapThreadContext(Runnable delegate) { | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Nit: @SuppressWarnings("unchecked") here (and again below) looks unnecessary, no actual unchecked cast on this line since ThreadContext.getContext() is already generically typed. Also the javadoc above has empty @param/@return tags with no description.
| // add alert to the Host here | ||
| if (e instanceof ConnectionException) { | ||
| ConnectionException ce = (ConnectionException) e; | ||
| // XXX: in case of Storage Pool issue we are ending up here |
There was a problem hiding this comment.
Nit: leftover XXX comment carried over in the moved code.
|
|
||
| return answer; | ||
|
|
||
| // FIXME: There are a lot of result != null checks in callers. Should this method trow exception? |
There was a problem hiding this comment.
Nit: leftover FIXME comment carried over in the moved code.
| } else { | ||
| // take ownership temporarily to prevent others enter into stage of acquiring DB lock | ||
| ownerThread = Thread.currentThread(); | ||
| // XXX: do we need it here (???) |
There was a problem hiding this comment.
Nit: leftover casual "XXX: do we need it here (???)" comment carried over in the moved code.
| break; | ||
| } | ||
|
|
||
| // FIXME: it is fire and forget, Management Server will never know if task failed |
There was a problem hiding this comment.
Nit: new FIXME comment, might be worth a tracked follow-up instead of an inline note.
…ements. - Enhances the Host connecting logic to avoid connecting storm (where Agent opens multiple sockets against Management Server). - Implements HostConnectProcess task where Host upon connection checks whether lock is available, traces Host connecting progress, status and timeout. - Introduces AgentConnectStatusCommand, where Host checks whether lock for the Host is available (i.e. "previous" connect process is finished). - Implementes logic to check whether Management Server has lock against Host (exposed MySQL DB lock presence via API) - Removes synchronization on Host disconnect process, double-disconnect logic in clustered Management Server environment, added early removal from ping map (in case of combination ping timeout delay + synchronized disconnect process the Agent Manager submits more disconnect requests) - Introduces parameterized connection and status check timeouts - Implements backoff algorithm abstraction - can be used either constant backoff timeout or exponential with jitter to wait between connection Host attempts to Management Server - Implements ServerAttache to be used on the Agent side of communication (similar to Attache on Management Server side) - Enhances/Adds logs significantly to Host Agent and Agent Manager logic to trace Host connecting and disconnecting process, including ids, names, context UUIDs and timings (how much time took overall initialization/deinitialization) - Adds logs to communication between Management Servers (PDU requests) - Adds DB indexes to improve search performance, uses IDEMPOTENT_ADD_INDEX for safer DB schema updates
- Bug 1 fix (AgentManagerImpl.java) — GlobalLock.isLockAvailable() now only runs when the host status is not alive. This eliminates one IS_FREE_LOCK DB query per ping per healthy host, which is the direct cause of listHosts/listNetworks degradation. - Bug 2 fix (HostConnectProcess.java) — shutdown() → shutdownNow(). Old thread pools from prior connect cycles are now interrupted immediately instead of draining their queued tasks, preventing thread accumulation during reconnect storms. - Bug 3 fix (AgentManagerImpl.java) — Lock timeout in handleDisconnectWithoutInvestigation now logs a warn instead of silently discarding the disconnect event. - Bug 4 fix (ServerAttache.java) — Alarm ScheduledFuture handles are now tracked in _alarmFutures and cancelled when the corresponding listener is unregistered or all commands are cancelled on disconnect. - Bug 5 fix (AgentAttache.java) - Fix Alarm ScheduledFuture handles in AgentAttache as well
Dedupes agent connection requests. Observed scenarios where on agent with old agent code send a storm of connection requests. In such a case, to ensure other agents are not impacted, deduping the connection requests.
520bc0e to
8a090ca
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 8 comments.
Suppressed comments (3)
agent/src/main/java/com/cloud/agent/Agent.java:745
- The condition
firstRefuseLoopHostRef != nullis always true because the AtomicReference itself is never null. As written, this branch will run on every non-refusal error and will log/clear the loop state even when no refuse loop started. It should checkfirstRefuseLoopHostinstead.
} else if (!skipTimeout && (firstRefuseLoopHostRef != null || ignoreAvoidMsHostListRef.get())) {
logger.debug("Broke refuse loop for host {} by {}", firstRefuseLoopHost, host);
firstRefuseLoopHostRef.set(null);
ignoreAvoidMsHostListRef.set(false);
agent/src/main/java/com/cloud/agent/HostConnectProcess.java:228
- The startup command generation ignores the
connectionTransferflag when calling the resource. SinceServerResourcehas aninitialize(boolean isTransferredConnection)overload, use it so resources that care about transferred connections can adjust initialization accordingly.
ServerResource serverResource = _agent.getResource();
StartupCommand[] startup = serverResource.initialize();
if (ArrayUtils.isEmpty(startup)) {
core/src/main/java/com/cloud/resource/ServerResource.java:89
- Changing the default
isExitOnFailures()tofalsealters the behavior of everyServerResourceimplementation that does not override it. That’s a potentially breaking operational change (agents may keep running when they previously exited). Please confirm this is intended and consider gating it behind configuration or documenting it clearly for operators.
default boolean isExitOnFailures() {
// true would cause unnecessary Agent service restart, don't want it by default
return false;
}
| try { | ||
| stopAndCleanupConnection(); | ||
| } catch (Exception ex) { | ||
| logger.warn("Got an exception during stop and cleanup connection", e); |
| if (skipTimeout && firstRefuseLoopHost == null) { | ||
| firstRefuseLoopHostRef.set(host); | ||
| ignoreAvoidMsHostListRef.set(false); | ||
| logger.debug("Started refuse loop for host {}", firstRefuseLoopHost); |
| boolean reconnectForCurrentLink = link == this.link; | ||
| boolean currentLinkTerminated = this.link != null && this.link.isTerminated(); | ||
| boolean reconnectForNewHost = this.hostname != null && this.hostname.equals(preferredHost); | ||
| // if none of the above is true |
| /** | ||
| * {@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}. | ||
| */ |
| } catch (RuntimeException e) { | ||
| LOGGER.warn("Failed to persist backoff properties"); | ||
| } |
| 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); | ||
| } |
| // FIXME: it is fire and forget, Management Server will never know if task failed | ||
| migrateAgentsExecutorService.submit(new MigrateAgentConnectionTask(fromMsId, hostId, dc.getId(), orderedHostIdList, avoidMsList, lbCheckInterval, lbAlgorithm, lbAlgorithmChanged)); | ||
| } |
| // ask Host to reconnect to another Management Server | ||
| final MigrateAgentConnectionCommand cmd = new MigrateAgentConnectionCommand(msList, avoidMsList, lbAlgorithm, lbCheckInterval); | ||
| // timeout 1 minute (FIXME: should it be configurable?) | ||
| cmd.setWait(60); | ||
| final Answer answer = agentManager.easySend(hostId, cmd); //may not receive answer when the agent disconnects immediately and try reconnecting to other ms host |
Allows the management.server.address config to be a list of hostnames instead of static IPs. Rebalancing, cluster propagation, alerts, and stats collection all detect the format at runtime via ManagementServerAddressUtil and use the matching lookup (listNonUpStateMsHostnames vs listNonUpStateMsIPs) when computing the "avoid" list sent to agents. Hostnames survive MS restarts and IP reassignments where static IPs do not, which is required in deployment environments where management server instances can be rescheduled or replaced.
Description
Continued from #13028
This PR improves the Indirect agent connection handling, has the following improvements.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Checked the indirect agents connections (and re-connections) with KVM hosts, SSVM & CPVM.
How did you try to break this feature and the system with this change?