Add agent-side proxy failover for high availability#223
Merged
Conversation
Records the Phase 1 decisions: an agent.proxy.endpoints list alongside a comma-accepting --proxy/PROXY_HOSTNAME, rotation that advances on connect failure and resets to the head of the list on a dropped-but-established connection, unchanged pacing, and an app-level cursor rather than a gRPC NameResolver. Also records the constraints that research verified against the code, several of which contradict the original proposal: a standby proxy returns 404 rather than 503 for an unregistered path, http_sd_config silently deletes a standby's targets so the HA recipe must mandate static_config, and reference.conf must default the new list or every existing config fails to load. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Declares an ordered endpoints list on agent.proxy and regenerates ConfigVals.
Nothing reads it yet -- the rotation logic follows.
The reference.conf default is load-bearing, not cosmetic. tscfg emits an
unguarded c.getList("endpoints") for a list-typed key, in contrast to the
hasPathOrNull check it generates for the sibling hostname/port scalars, so
without agent.proxy.endpoints = [] every config written before this feature
existed would fail to load with ConfigException.Missing. Two ConfigVals tests
pin that, one of them shaped like an existing deployment's config; both fail if
the reference.conf line is removed.
That same asymmetry broke three test fixtures that built ConfigVals from a bare
parseString. They now merge the reference config the way BaseOptions.readConfig
does in production, which also immunizes them against the next list-typed key.
ProxyServiceImplTest was among them despite being proxy-only, because ConfigVals
eagerly constructs both trees.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The agent now accepts an ordered list of proxy endpoints and tries them in order. --proxy and PROXY_HOSTNAME take a comma-separated value, and agent.proxy.endpoints supplies the same list from a config file; a single value resolves to a one-element list and behaves exactly as before. Rotation is driven by one bit already present in the retry loop: agentId is non-empty at the top of an iteration only if the previous attempt actually connected. A connection that came up and then dropped resets to the head of the list, so a recovered primary is re-probed -- that is the whole failback mechanism, with no prober, timer, or per-endpoint state. An attempt that never connected advances to the next endpoint. The channel rebuild is the load-bearing part rather than an incidental one. A ManagedChannel is bound to its target address, and connectToProxy only rebuilt after a *successful* connection, so a run of failures reused one channel against one address. Without changing that guard a cursor would advance an index nothing reads. The rebuild on the advance path is conditional on the endpoint actually changing, so a single-endpoint agent keeps reusing its channel and leaning on gRPC's own backoff instead of gaining channel churn on every failed retry. Rotation never builds channels itself -- only resetGrpcStubs does -- so the shutDownRequested latch keeps winning and a rotation cannot resurrect a channel after a requested stop. A test pins that. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Adversarial review of the rotation change confirmed three defects. The rotation logic itself held up -- both the "does failover happen" and "is the single-endpoint path unchanged" lenses came back clean -- but the config resolution path did not. IPv6 endpoints were silently corrupted. parseHostPort strips the brackets off an IPv6 literal, and AgentOptions normalizes every configured endpoint by rendering the parsed HostPort back to "$host:$port" and re-parsing it. Without brackets that string re-parses as a BARE IPv6 address with no port, so the agent dialed a garbage authority on the default port and simply never connected -- no exception, no clue. HostPort.spec now re-brackets any host containing a colon, making the render the exact inverse of the parse and leaving IPv4/DNS output byte-identical. The hostname half of this round trip predates the branch; the endpoints key extended it. Agent.proxyHost composed two independent reads of the volatile cursor, one for host and one for port, so a rotation landing between them could make the /debug servlet print a host:port pair that was never configured. A single-read currentEndpoint accessor removes the window. A scheme-only entry such as "http://" passed the blank filter on its raw text and then failed with a message naming neither the entry nor the list. The gap that hid the IPv6 bug is closed too: assignConfigVals' endpoint resolution was entirely untested because every existing AgentOptionsTest case passes --proxy, which skips that block. A dedicated test config now drives it, IPv6 entry included. All five new tests fail if HostPort.spec is reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Two proxies run simultaneously on distinct gRPC ports; the agent is pointed at both, must start on the first, and must land on the second once the first stops. Netty rather than in-process is a hard requirement, not a preference: GrpcDsl ignores hostName and port entirely when an in-process server name is supplied, so an in-process failover spec would build the same channel whichever endpoint the cursor selected and would pass without any rotation occurring. Distinct ports matter for the same reason -- they make this a test of endpoint selection rather than of a name resolving to a new address, which ContainersReconnectTest already covers. The path lives in pathConfigs rather than being registered by the test, because a runtime registration does not survive a reconnect; only a config-driven path proves the agent re-registered on the endpoint it failed over TO. The assertion scrapes proxy B's own HTTP port, which can only succeed if the agent registered there, and additionally checks the cursor moved and that B issued a new agentId. Verified the test bites: with advanceEndpoint() forced to return false, it fails. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
proxyContainer unconditionally claimed the shared proxy-host alias, so two instances landed behind one name and Docker's embedded DNS round-robined between them non-deterministically. It now takes an alias (mirroring agentContainer and metricsStub) plus a log label, so an HA pair is addressable and its logs are distinguishable. The new spec runs both proxies on distinct aliases and asserts the agent resumes serving through the standby's OWN alias after the active proxy stops -- which is only possible if the agent selected B's endpoint and re-registered there. Distinct aliases are what make this a failover test: ContainersReconnectTest replaces a proxy at one alias and therefore proves DNS re-resolution, so a spec copied from it would pass even with rotation unimplemented. Verified both directions against real containers: passes as written, and fails when advanceEndpoint() is forced to return false. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Covers the feature in the README CLI table, the agent configuration page, the production HA guide, CHANGELOG, and release notes, and marks Feature 2 as implemented with as-built notes recording where it diverged from the proposal. Two pre-existing doc problems are fixed rather than worked around. Proxy.kt's KDoc already asserted that agents "automatically reconnect to available proxies" -- on a Dokka-published type -- while production.md said the opposite; this release makes the claim true, so both now describe the same mechanism and state plainly that proxies share no state. And the proposal's assertion that a standby returns 503 for an unregistered path was simply wrong: it returns 404, since a null agent context routes to invalidPathResponse. Prometheus scores both as up=0, so the conclusion held, but the code would have propagated into alert rules. The http_sd_config hazard is called out prominently in all three operator-facing places. A standby returns an empty discovery list, which Prometheus treats as target deletion rather than a failed scrape -- so the series vanish with no up=0 and no alert, defeating the point of running a pair. The HA recipe mandates static_config. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
proxyImage()/agentImage() were functions used as default parameter values, so every container constructed its own ImageFromDockerfile -- and Testcontainers gives each instance its own random localhost/testcontainers/<hash>:latest tag. ContainersScalingTest builds one agent container per agent, so a single run minted a tag per agent plus one per proxy. The layers are content-hashed and shared, so this costs no build time, but the tags accumulate: they are reaped only by a JVM shutdown hook that never fires when a run is killed or times out. On this machine that had grown to 5800 tags resolving to 357 distinct images, 60.7GB with 85% reclaimable. Sharing one lazily-built instance per image is the documented way to build once and reuse: the value is a Future<String> resolved on first use. Verified against real containers -- a run with two proxy containers and one agent now creates two tags rather than three, and the failover spec still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
registerAgent sent agentHostName -- the proxy endpoint the agent had dialed -- in the request's host_name field. The proxy stores that on AgentContext and publishes it as the reserved `hostName` service-discovery label, which is documented in Proxy.kt's KDoc and service-discovery.md as "hostnames of agents serving this path" and which agents are explicitly forbidden from overriding. The proxy's own tests encode that meaning: AgentContextTest stubs the request field as "agent-host", and ProxyTest asserts the emitted label is "internal.host.com". So every discovered target carried the same value -- all agents connect to the same proxy -- and a label meant to identify which internal host serves a path identified nothing. Fixed now rather than filed, because failover makes it actively harmful rather than merely useless: hostName participates in Prometheus target identity, so a value tracking the current endpoint would change on every failover and churn the series each time. That would be a regression introduced by this branch. The nearby channel(hostName = agentHostName, ...) is untouched -- there the name correctly means "the host to dial". Operator-visible, so CHANGELOG and RELEASE_NOTES carry an upgrade note: the label value changes, causing a one-time series churn. Covered by a test that captures the outbound request and fails if the proxy endpoint is sent again. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The field was declared and never read or written anywhere in the codebase -- its only occurrence was the declaration itself. That makes it a trap: it reads like a server-driven redirect mechanism, which does not exist. Agent-side proxy failover is driven by the agent's own ordered endpoint list, not by anything the proxy returns. Both the number and the name are reserved so neither can be reused with different semantics later. No wire-compatibility concern: nothing ever populated the field, so no serialized message has ever carried it, and proto3 readers ignore unknown fields regardless. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Review found one real defect: currentEndpoint exists so the volatile cursor is read once, but the channel-construction site read it twice (hostName = agentHostName, port = agentPort), contradicting the invariant the accessor was added for. It is under grpcLock so it could not actually interleave there, but the shape was wrong; it now binds once. Reuse: AgentProxyFailoverTest hand-rolled proxy and agent startup because TestUtils.startProxy hardcodes the port and unconditionally adds --config, which is a single-valued JCommander parameter and so cannot be overridden through args. Both helpers gained defaulted proxyPort/configArgs parameters -- source-compatible with all nine existing callers -- and the private copies are gone. Removed over-engineering added by this branch: proxyContainer's logLabel parameter, whose only two callers passed exactly the default, and the widened PROXY_ALIAS visibility, whose only external reference was a comment. DEFAULT_GRPC_PORT moves to AgentOptions so the public options type stops importing from the internals of the gRPC client it configures. The two AgentOptions fallback branches were one expression written twice and had already drifted -- only the list branch applied stripScheme/trim. Collapsed to one, which means agent.proxy.hostname now tolerates a scheme prefix where it previously threw at startup. Tests: six rotation specs called a bare shutDown(), so a failed assertion leaked a channel into the next spec; they share a try/finally helper now. The tscfg reference-merge rationale was copy-pasted into three fixtures and is now one documented testConfigVals helper. Two redundant specs folded, harness cleanup made symmetric. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #223 +/- ##
==========================================
+ Coverage 91.95% 92.02% +0.06%
==========================================
Files 45 45
Lines 2922 2947 +25
Branches 530 541 +11
==========================================
+ Hits 2687 2712 +25
Misses 107 107
Partials 128 128
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Feature 2, Phase 1 from
docs/FEATURE_PROPOSALS_JULY_2026.md. The proxy was a single point of failure for the entire metrics plane — if it went down, every target behind every agent disappeared from Prometheus at once. Agents can now be given an ordered list of proxy endpoints and fail over between them.hostNameservice-discovery label changes valueIf you use
http_sd_configand referencehostName, expect a one-time series churn.The label is documented as the hostname of the agent serving a path, and the proxy reserves it so agents cannot override it — but the agent was sending the proxy endpoint it had dialed. Every discovered target therefore carried the same value. It now reports the agent's own hostname, as documented.
This is fixed here rather than filed because failover makes the old behavior actively harmful:
hostNameparticipates in Prometheus target identity, so a value tracking the current endpoint would change on every failover and churn the series repeatedly.Usage
Or comma-separated via
--proxy/PROXY_HOSTNAME. A single value resolves to a one-element list and behaves exactly as before, so existing configurations need no change.Scrape the pair with
static_config, nothttp_sd_config— a standby returns an empty discovery list, which Prometheus treats as target deletion rather than a failed scrape, so series vanish with noup=0and no alert. Documented prominently in the release notes, production guide, and agent config page.Design
ManagedChannelis bound to its target address, andconnectToProxypreviously rebuilt only after a successful connection — so a run of failures reused one channel against one address. Without changing that guard a cursor would advance an index nothing reads. The rebuild on the advance path is conditional on the endpoint actually changing, so single-endpoint agents keep reusing their channel and leaning on gRPC's own backoff.NameResolver. A custom provider would break silently in the fat JAR, sincesrc/shadow/resources/META-INF/services/pins the provider list — it would pass every unit and harness test and fail only in the container suite.reconnectPauseSecslimiter.Diverges from the written proposal in three places (no
--proxiesflag, nofailoverPauseSecs, failback rather than sticky) — all recorded in the as-built notes.Testing
Unit coverage for endpoint parsing,
HostPort.specround-tripping, rotation, and thereference.confregression, plus a Netty harness spec and a two-proxy container spec.Both end-to-end specs were verified to fail when
advanceEndpoint()is forced to return false. This mattered: a container spec copied fromContainersReconnectTestwould reuse one network alias and so prove DNS re-resolution rather than endpoint selection — passing even with rotation unimplemented. Both use distinct aliases/ports and assert they reached the standby specifically.An in-process harness spec cannot cover this at all —
GrpcDslignores host and port when an in-process server name is supplied.Review found and fixed
Adversarial review confirmed the rotation logic itself, but caught real bugs in config resolution: IPv6 endpoints were silently corrupted (
parseHostPortstrips brackets, and the naive re-render was not its inverse, so the agent dialed a garbage authority and never connected), a tornhost:portread on/debug, and an untested resolution path that had hidden the IPv6 bug.Separable commit
50ea363(build each container image once per JVM) is unrelated test infrastructure — it surfaced while debugging a scaling-test failure caused by 5,800 leaked Testcontainers image tags. Happy to split it out if preferred.🤖 Generated with Claude Code