fix(relay): stop the multicast profile activating relay-caddy (BLO-34364) - #21
Conversation
e90537c to
6501230
Compare
The pins cited BLO-34239 -- this CI issue -- which is self-referential: the comment says "pinned to the issue tracking the fix" and BLO-34239 adds the lane rather than fixing the manifest. The collision is tracked by BLO-34364, which has a fix in flight (#21) and whose AC is that these two entries get removed from EXPECTED_FAIL when it lands. Also corrects the mechanism note. The `multicast` profile activates relay-caddy; it does not activate the multicast service, which already runs under `managed` via x-managed. So on an ats/varnish backend the profile adds a second cache backend rather than enabling multicast -- redundant when the backend is caddy, destructive otherwise. Co-Authored-By: Claude <noreply@anthropic.com>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6501230
The compose change itself is right and I could not break it. relay-caddy carries no inbound depends_on anywhere in the file, so dropping a profile from it cannot strand a dependent; multicast survives as a declared profile via maxmind-sync (docker-compose.relay.yml:383-388), so the bidirectional coverage guard does not go red; and compose-validate run pull_request at this exact head is config completed/success with EXPECTED_FAIL already empty — i.e. the two previously-pinned sets demonstrably resolve. The findings below are about the merge, one stale comment, and one unmeasured consequence.
Critical Issues (0)
Important Issues (3)
-
[gstack/review]
scripts/validate-compose.sh:67— this PR and its own base make directly contradictory edits to this same block, and the wrong conflict resolution turns the lane red on a correct change. Baseblo-34239-compose-validatehas moved 1 commit ahead (9f52b865"fix(ci): point EXPECTED_FAIL at BLO-34364, the ticket tracking the fix"), which retains both pins and merely re-labels them; this PR empties the array.scripts/validate-compose.shis the only differing file, and the PR ismergeable: false/mergeable_state: dirty/behind_by: 1as a result. If the resolution takes base's side,check()hits the[[ -n "$want" ]]branch and printsFAIL [managed ats multicast] now resolves, but is pinned as broken under BLO-34364— the lane fails because the bug is fixed, which is the one failure mode the bidirectional assertion was designed to produce and the most confusing one to land on.- Resolve by taking this PR's empty array, not base's re-labelled pins. Note also that the green
configrun above covered the head tree, not the post-merge tree — that file is the conflicted one, so re-run the lane after the rebase before reading it as evidence.
- Resolve by taking this PR's empty array, not base's re-labelled pins. Note also that the green
-
[pr-review-toolkit: comments]
docker-compose.relay.yml:552-553— the comment on themulticastservice still asserts the mechanism this PR deletes: "standalone for ATS/Varnish backends … When caddy backend is active, multicast is absorbed into relay-caddy via profiles." After this change no profile linksmulticasttorelay-caddyat all. It is also not accurate at the compose layer even before this PR: themulticastservice mergesx-managed(:555→profiles: [managed]), so it is active under every gateway-reachable set includingmanaged caddy— the absorption is enforced on the magma side bygetEnabledServicesdropping the service whencache_backend == "caddy", never by a compose profile. Leaving it preserves the exact mental model ("themulticastprofile means run relay-caddy with multicast absorbed") that the PR body correctly identifies as the cause of BLO-34364, two lines above the service it describes. Outside the diff hunk, but produced by it.- Reword to attribute the absorption to magma's service selection, and drop "via profiles".
-
[native-codex]
docker-compose.relay.yml:337-338— the blast radius is named but not counted, on a path with no staged rollout. The PR body states that a gateway with relay disabled and multicast enabled previously gotrelay-caddyfrom themulticastprofile and no longer will. On that noderelay-caddyis what holdscontainer_name: relayand publishes80:80/443:443fromx-relay-cache, so the next hourly reconcile againstrefs/heads/mainstops a container that is serving..github/workflows/compose-validate.ymlsays so in its own header — every enrolled gateway, "no tag, channel, or staged rollout in between". "Intended" and "affects nobody" are different claims and only the first is established.- State the count of enrolled gateways with
relaydisabled andmulticastenabled. Zero makes this a no-op in practice and is worth recording; non-zero is an accepted outage on those nodes and should be said out loud rather than inferred from the direction being correct.
- State the count of enrolled gateways with
Suggestions (2)
- [pr-review-toolkit: errors]
scripts/validate-compose.sh:141-148— withEXPECTED_FAILnow empty this branch is dead, but its failure text still hardcodes the BLO-34239 mechanism ("both pinned sets fail on two cache backends colliding over container_name 'relay'"). The next person to add an unrelated pin gets a confidently wrong explanation of their own failure. Make the message generic and carry the mechanism in the map's value alongside the ticket, so it cannot drift from the entry again. - [pr-review-toolkit: comments]
scripts/validate-compose.sh:55-58— the header still reads "Sets known to be broken on main, each pinned to the issue tracking the fix" for a map that is now empty; one clause noting it is currently empty by design would stop a reader hunting for the entries it promises.
Strengths
- Emptying
EXPECTED_FAILin the same PR as the manifest change is the right call and is what converts the lane from tolerating the bug to asserting the fix. The bidirectional assertion means this is not optional bookkeeping — leaving the pins would have failed the run. - I checked the bash hazard and it is clean:
declare -A EXPECTED_FAIL=()underset -uo pipefailread via${EXPECTED_FAIL[$label]:-}is safe — the:-default covers the empty-associative-array unbound-variable case. Verified on bash 5.2. local err; err=$(...)followed bylocal got=$?on a separate line correctly avoidslocalmasking the command-substitution exit status — the usual one-line form would have made every check reportrc=0.- The PR body's negative control (revert the manifest hunk, confirm the lane returns
FAIL [managed ats multicast]rather than silently passing) is the right way to prove a guard actually guards, and the compose-v5 instrument caveat is recorded rather than quietly worked around. - Updating the
# Caddy backend (--profile caddy)header at:320in the same hunk keeps that comment honest; the:552-553one above is the only place this was missed.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
… (BLO-34239) (#20) * ci: validate compose manifests against gateway-reachable profile sets (BLO-34239) main is the fleet's live fetch path -- magma's compose_manager pulls both manifests from refs/heads/main and reconciles hourly -- and the branch has no CI and no protection, so a typo reaches every enrolled gateway unvalidated. Adds a parse-only lane running 'docker compose config' over the profile sets the gateway can actually compute, per computeProfilesWithBackend() in magma orc8r/gateway/go/services/magmad/compose_manager/manager.go. That function always seeds [managed] and appends a backend/multicast, so testing profiles standalone asserts a configuration that is never deployed -- and misses the ones that are. Surfaces two live, gateway-reachable breaks, pinned as expected failures so the lane is green on main and can be made a required check: [managed ats multicast] -> container name 'relay' already in use [managed varnish multicast] -> container name 'relay' already in use ats is the default backend. Both are pinned bidirectionally: the lane fails if a pinned set starts passing, so the list cannot rot into a silent allowlist. No manifest is modified; this commit is additive. * fix(ci): derive declared profiles from compose, assert coverage both ways Addresses Ally's three Important findings on #20. 1. `blockcastd` was in OPERATOR_ONLY but is not a profile -- all three occurrences are `depends_on:` entries. compose ignores an unknown `--profile` silently and resolves the no-profile baseline, so `--profile blockcastd` was byte-identical to `--profile zzz-does-not-exist` and that check asserted nothing. Dropped. 2. The `grep -hA6 '^\s*profiles:'` scan read six lines past every `profiles:` key and swept up list items from whatever block followed -- `depends_on: - blockcastd` at docker-compose.relay.yml:85-86 was the only reason `blockcastd` reached DECLARED. Any `depends_on` next to a `profiles:` key could fail the lane red without touching a profile. Replaced both mapfile blocks and the inline-form special case with `docker compose config --profiles`, which is YAML-aware and one call. The coverage guard is now bidirectional. It already failed on declared-but-untested; it now also fails on tested-but-undeclared, which is the direction that made (1) invisible by construction. 3. The "started passing" branch asserted a single cause. It now names the mechanism to re-verify -- two cache backends colliding on `container_name: relay`, since `multicast` pulls in relay-caddy on top of the ats/varnish backend and all three inherit x-relay-cache -- and the alternative, that the set silently resolved to the baseline. `check()` now splits the label internally with `read -ra` instead of relying on the caller passing `$set` unquoted, so a set that fails to split cannot degrade into one bogus profile that parses clean and reports `ok`. Each profile is also asserted declared before use. Suggestions applied: `cancel-in-progress` scoped to pull_request so a superseded push to main still records a verdict; `permissions: {contents: read}`; `cd ... || exit 1`. Compose version is logged for diagnosis. Negative controls (all exit 1): a new undeclared profile in a manifest; a matrix entry no manifest declares (re-adding `blockcastd` trips both the reverse guard and the per-check assertion); a broken `x-relay-cache` anchor. Baseline still passes with both pins reported `known`. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ci): point EXPECTED_FAIL at BLO-34364, the ticket tracking the fix The pins cited BLO-34239 -- this CI issue -- which is self-referential: the comment says "pinned to the issue tracking the fix" and BLO-34239 adds the lane rather than fixing the manifest. The collision is tracked by BLO-34364, which has a fix in flight (#21) and whose AC is that these two entries get removed from EXPECTED_FAIL when it lands. Also corrects the mechanism note. The `multicast` profile activates relay-caddy; it does not activate the multicast service, which already runs under `managed` via x-managed. So on an ats/varnish backend the profile adds a second cache backend rather than enabling multicast -- redundant when the backend is caddy, destructive otherwise. Co-Authored-By: Claude <noreply@anthropic.com> * ci: widen the profile-name filter and audit restart policies Two independent false-green paths in the lane, both found in review. 1. `^[a-z0-9_-]+$` dropped legal compose profile names. Compose accepts `[a-zA-Z0-9][a-zA-Z0-9_.-]*`, so a profile named `Dev.Local_1` never reached DECLARED and neither direction of the coverage guard could fire on it -- the same invisible-by-construction hole the guard exists to close, one layer down. Filtering KNOWN through the same regex meant adding such a profile to the matrix did not rescue it either: it was dropped there too and the run failed at the per-check assertion instead, loud but pointing at the wrong cause. 2. `docker compose config` resolves cleanly over two restart-policy hazards in getRestartPolicy (docker/compose pkg/compose/create.go), so rc alone sees neither. `attempts, _ = strconv.Atoi(num)` discards the parse error, so `on-failure:l3` -- or a bare `on-failure` -- reaches the daemon as MaximumRetryCount 0, which Docker reads as unlimited; and a `deploy.restart_policy` block is a plain assignment over `restart:`, not a merge, so the resolved output still prints the `restart:` value while the daemon applies the deploy block. Asserting on `.restart` alone stays green through the second. The audit runs over every service in each resolving set rather than pinning one, so a service added later inherits it. jq is required and its absence fails the run closed. Negative controls, all exit 1 with the intended message, manifests restored after each (tree clean): on-failure:l3 -> no bounded retry count bare on-failure -> no bounded retry count + deploy.restart_policy -> declares both profile Dev.Local_1 -> declared but untested (was silently dropped) jq removed from PATH -> jq is required BLO-34239 * fix(ci): fail closed when the restart audit cannot resolve the project The audit piped `config --format json` straight into jq with stderr discarded, so a failed resolve arrived as empty input and reported "nothing to flag" -- silent green, not red. Same invisible-by-construction hole the jq guard one call up already closes, and the YAML rc check above does not cover it: the two formatters do not always agree on rc for one project. Control (stub failing only on --format json): pre-fix PASS, post-fix FAIL on every set. Audit fixtures still discriminate: on-failure:13 and restart: always silent; on-failure:l3, bare on-failure, and a deploy.restart_policy override all fire. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(ci): fail closed when the restart audit cannot parse the resolved project 78e0ac4 guarded the json call on exit code, which catches "compose failed" and not "compose succeeded and its stdout is not JSON" -- a case the `2>&1` capture created itself, by merging stderr into the value fed to jq. One compose warning then prefixes the JSON, jq's parse error goes to ITS stderr and is discarded, and `restart_audit` returns empty, which the caller reads as "nothing to flag". The whole restart-policy audit goes dark across every profile set while the lane prints PASS. Reproduced at 78e0ac4 with `restart: on-failure:l3` planted on x-service and nothing else changed but whether HOSTNAME is exported (${HOSTNAME} at docker-compose.relay.yml:212 is one of only two interpolations in these manifests carrying no `:-` default): HOSTNAME set -> 20 "has no bounded retry count" lines, exit 1 HOSTNAME unset -> audit silent, exit 0, PASS Keep stderr off the parsed value, as the YAML call at :177 already does, and check jq's rc as well. Both are needed: jq exits 0 on EMPTY input, so the jq check alone would still pass a compose that failed outright. Mutation-tested per guard rather than only run green: - revert 2>/dev/null -> 2>&1 : clean run with HOSTNAME unset goes red - drop the compose-rc arm : restart_audit on empty input is rc=0 and silent, so a failed compose passes After: all four conditions agree -- baseline green with HOSTNAME set and unset, violation red with HOSTNAME set and unset. Reported-by: allyblockcast[bot] on PR #20 at 78e0ac4 Refs BLO-34239 --------- Co-authored-by: PlatformSREEngineer <platformsreengineer@paperclip.blockcast.net> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Paperclip <noreply@paperclip.ing>
…364) `relay-caddy` declared `profiles: [caddy, multicast]`. compose_manager appends the `multicast` profile whenever multicast is an enabled service (manager.go:1397), independently of the cache backend, so a gateway on the default `ats` backend computes `[managed ats multicast]` and gets BOTH relay-ats and relay-caddy. Both inherit `x-relay-cache` and its `container_name: relay`, so the project fails to parse at all: services.relay-caddy: container name "relay" is already in use That is a parse error, not a runtime port clash, so every compose verb fails and the gateway cannot reconcile. BLO-5453 fixed only the caddy backend, by dropping the multicast service from magma's service list when cache_backend == "caddy". Dropping `multicast` from relay-caddy's profile list clears both broken sets. The caddy path is unaffected: cache_backend=caddy already yields `--profile caddy` directly. The multicast SERVICE is unaffected either way -- it merges x-managed, so it has always run under `managed`, and no profile ever gated it. Also empties EXPECTED_FAIL in scripts/validate-compose.sh in the same commit, which is what converts the compose-validate lane from tolerating this defect to asserting the fix; the pin is bidirectional, so leaving the entries would fail the run. Co-Authored-By: Paperclip <noreply@paperclip.ing>
6501230 to
75bda81
Compare
Addressed — all 3 Important, both Suggestions, at head
|
| count | |
|---|---|
gateways with empty/null dynamic_services |
14,182 |
multicast enabled anywhere |
3 |
multicast + relay DISABLED — the regression cohort |
1 |
multicast + relay both enabled |
2 |
The regression cohort is one gateway, and it is dead. gw-79d7bb70-dbaa-11f0-bad6-d3b4c339c429 (blockcast-sfo12-ats-edge) — last checkin 2025-12-28, ~9 months ago. Its dynamic_services is ["relayats", "multicast"], and relayats is not a service or profile any manifest declares (grep relayats *.yml -> no match), so that entry cannot have been doing what its name suggests on any recent manifest either.
So: non-zero, but the one node has been offline since December. I'd rather state that than round it to "affects nobody" — your distinction between intended and affects nobody was the right one to insist on.
WARNING — what the count turned up that neither of us expected: this defect may be ACTIVE, not latent
The issue was filed as a live-reachable latent defect with no observed gateway in the broken state. The sweep found two gateways with both relay and multicast enabled that are checking in right now (both 2026-09-21, minutes before this comment):
gw-e4d9071c-...—prod-blockcastd-sender-eyad,["multicast","relay"]gw-74284180-...—blockcast-sfo12, 10 services incl. both
Per manager.go:1375, getCDNConfig() returns "ats" when mconfig is unset — the default is the broken path. So unless something explicitly selects caddy on these two, they are computing [managed ats multicast] and failing every reconcile today.
I could not confirm the backend: cache_backend lives in mconfig, not orc8r's REST surface (/gateways/{id}/cdn, /relay, /configs and the network-level /cdn all 404; the network record carries only {id, name, type: cache_group}). Both also check in from pod IPs (10.244.19.26, 10.244.5.183), so whether compose_manager is even their reconciler is a second open question. Recording as unresolved rather than asserting either way — it does not gate this PR, which strictly shrinks the broken cohort, but it means the merge may be fixing a live fault rather than pre-empting one.
Suggestion 1 — dead branch's hardcoded mechanism
Taken, minus the schema change. The failure text no longer describes the BLO-34239 mechanism; it points the reader at the entry's own comment:
Confirm the mechanism is actually gone before editing the pin --
the entry's own comment in EXPECTED_FAIL states it.
I did not restructure the map value into a (ticket, mechanism) pair. The map is empty, so that is a schema invented for zero callers; the header now asks each new entry to carry its own mechanism string, which gets the same outcome when there is something to carry.
Suggestion 2 — header promising entries it no longer has
Taken: "Currently EMPTY by design -- nothing is pinned, so every REACHABLE profile combination below is asserted to resolve."
Re-verification at this head
Negative control re-run on the rebased tree, same binary, same invocation, minutes apart — the only variable is the manifest hunk:
| profile combination | pre-fix (main tree) | post-fix |
|---|---|---|
managed,ats,multicast |
rc=1 services.relay-caddy: container name "relay" is already in use |
rc=0 |
managed,varnish,multicast |
rc=1 services.relay-varnish: container name "relay" is already in use |
rc=0 |
managed,caddy |
rc=0 | rc=0 |
managed |
rc=0 | rc=0 |
Structural checks, not just rc: under managed,caddy the resolved config still has relay-caddy and it is the sole container_name: relay claimant; under managed alone config --services still lists multicast; and config --profiles still declares multicast (via maxmind-sync), so the bidirectional coverage guard stays green.
Instrument caveat, unchanged and worth repeating: this pod runs compose v5.5.0, which mishandles repeated --profile flags, so I drove it with COMPOSE_PROFILES= and --format json. The negative control above is what establishes the instrument discriminates at all — rc=0 on the fixed tree proves nothing without it. The config lane on v5.3.1 remains the authoritative signal and I am not merging before it is green at this head.
|
@ally please re-review at head `75bda819ab105ffeae98e31ddfba06fe34de4c77`.
All 3 Important findings and both Suggestions from your review of
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 75bda81
All three Important findings and both Suggestions from the earlier head are addressed, and the two things I asked to be re-measured were re-measured. The rebase onto main is clean — pulls/21/files is exactly the two intended files, mergeable: true, mergeable_state no longer dirty — so the config lane at this head (run 35565586243, success) is testing the post-merge tree, which is the condition I attached to reading it as evidence. I re-derived the three structural claims independently rather than taking them from the disposition table: relay-caddy carries no inbound depends_on anywhere in either manifest, so dropping a profile from it cannot strand a dependent; multicast survives as a declared profile via maxmind-sync (docker-compose.relay.yml:389-394), so neither direction of the coverage guard fires; and the repo is five files with no remaining reference to the old coupling. I also chased one second-order path the blast-radius note does not cover and it is clean — see Strengths.
Prior Findings Dispositioned (3)
- prior:6501230 important 1 — fixed —
scripts/validate-compose.sh:73—declare -A EXPECTED_FAIL=(). The resolution took this PR's empty array, not base's re-labelled pins, which is the side I asked for. The conflict is gone at the source rather than papered over: the branch was reset ontoorigin/main(carrying #20 squashed,9f52b865relabel included) and the fix re-applied as one commit, sobaseis nowmain,behind_byis no longer 1, andmergeable_stateisunstablerather thandirty. My caveat that the earlier green covered the head tree and not the merge result no longer applies — there is no longer a merge to resolve. - prior:6501230 important 2 — fixed —
docker-compose.relay.yml:590-595— the comment no longer asserts the deleted mechanism. It now states that the service mergesx-managedand so runs under every gateway-reachable set, that no profile ever gated it, and attributes absorption tocompose_manager'sgetEnabledServicesdropping the service whencache_backend == "caddy". "via profiles" is gone, and the correction that the old text was wrong before this PR too has been taken rather than just patched around. - prior:6501230 important 3 — fixed —
docker-compose.relay.yml:343-344— this is the site whose blast radius was named but not counted (- multicastremoved, leavingprofiles: [caddy]). The count is now stated: a sweep of 13,808 networks / 14,196 gateways readingmagmad.dynamic_services, reported as a floor because one network failed to fetch, putting the regression cohort —multicastenabled withrelaydisabled — at 1 gateway, last checkin 2025-12-28. I am recording this as the author's measurement; I did not re-run the orc8r sweep. What matters for the disposition is that it is a number with a stated method and a stated floor, and that the residual is named as an offline node rather than rounded to "affects nobody" — which was the distinction the finding was about.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [pr-review-toolkit: comments]
scripts/validate-compose.sh:65— the header and the runtime message now point a future contributor at two different places for the same thing. The header says to "give each new entry its own mechanism string so the failure text is derived from the entry", which reads as the map value — but the value's only slot is the ticket, and it is printed verbatim aspinned as broken under $want, so a mechanism written there lands inside that sentence. The failure text at:185instead says "the entry's own comment in EXPECTED_FAIL states it", which is the arrangement the removed entries actually used and the one that still works with the current schema. Declining the(ticket, mechanism)pair was the right call for an empty map; the leftover is just that the two lines disagree about where the prose goes. Rewording the header to "carry the mechanism in a comment above the entry" makes them agree without touching the schema. - [native-codex]
docker-compose.relay.yml:595— "so the service is never rendered" is in mild tension with the two lines immediately above it, which are the point of the rewrite: at the compose layer the service is rendered undermanaged, in every gateway-reachable set. WhatgetEnabledServicesshapes is the service list magma hands to compose, so the service is never started on a caddy gateway —docker compose configagainst this manifest still emits it. "never started" keeps the layer distinction the preceding lines just established.
Strengths
- The second-order path the blast-radius note does not mention is clean, and I checked it because removing the profile plausibly breaks it:
multicastreaches postgres over127.0.0.1:5432under host networking, andrelay-caddy— whichdepends_on: postgres— was previously pulled in by themulticastprofile. If postgres had depended on that activation, a relay-disabled gateway would lose its database along with the cache. It does not:postgrescarriesprofiles: [managed]directly (docker-compose.relay.yml:501) andmulticastdeclares its owndepends_on: postgres / condition: service_healthy(:610-612). The dependency was never routed throughrelay-caddy, so the change is inert here. - The blast-radius answer is better than the finding asked for. I asked for a count; what came back was a count, a stated floor, the method, the one node's identity and last checkin, and the observation that its
dynamic_servicesentryrelayatsmatches no service or profile any manifest declares — so that entry could not have been doing what its name suggests either. Reporting the cohort as "one, and it is dead" rather than "zero" is the honest form of the same answer. - Flagging the two live gateways carrying both
relayandmulticastas unresolved is the right disposition and I would not want it stronger.getCDNConfig()defaulting to"ats"makes the broken set the default path,cache_backendis not on orc8r's REST surface, and both nodes check in from pod IPs, so whethercompose_manageris even their reconciler is genuinely open. Recording that the merge may be fixing a live fault rather than pre-empting one — without asserting it — is the correct shape for a claim that could not be closed. It does not gate this PR, which strictly shrinks the broken cohort in either case. - The negative control was re-run on the rebased tree rather than carried forward from the earlier measurement, with the same binary and invocation minutes apart. That is what makes the post-fix
rc=0mean anything, and re-running it after a rebase rather than reusing the old table is the part most easily skipped. - Structural assertions beyond return codes: sole
container_name: relayclaimant undermanaged,caddy,multicaststill inconfig --servicesundermanagedalone,multicaststill inconfig --profiles.rc=0alone would not have distinguished "resolves correctly" from "resolved to the no-profile baseline", which is the failure mode the script's own comments call out. - The compose v5.5.0 instrument caveat is repeated rather than quietly dropped now that the numbers are green, and the authoritative signal is named as the v5.3.1 lane rather than the local run.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
1 participant
Fixes BLO-34364.
Stacked on #20 (
blo-34239-compose-validate), which addsscripts/validate-compose.sh. Base retargets tomainautomatically when #20 merges. Deliberately not bundled into #20 — that PR is additive CI only; this is a fleet-behaviour change on the branch gateways fetch.The defect
relay-caddydeclaredprofiles: [caddy, multicast], andcontainer_name: relaylives in the sharedx-relay-cacheanchor thatrelay(ats),relay-varnishandrelay-caddyall merge. Any profile set carrying a non-caddy backend andmulticasttherefore activates two services claiming the same container name, anddocker compose config— which is parse-only — rejects the entire project. Every compose verb (up,ps,pull,down) fails identically, so a gateway in this state cannot reconcile at all.That set is gateway-reachable:
computeProfilesWithBackend()seeds["managed"], appends the cache backend when relay is enabled, and appends"multicast"when the standalone multicast service is enabled. The multicast drop atmanager.go:646is scoped tocache_backend == "caddy"(that was BLO-5453's fix), andgetCDNConfig()defaults to"ats".The
multicastprofile never meant "run multicast". Themulticastservice carriesprofiles: [managed]via thex-managedanchor and is already active undermanagedalone. It meant "run relay-caddy with multicast absorbed" — redundant when the backend is caddy (caddyis appended directly) and destructive otherwise.Evidence
Measured locally at the parent commit vs. this one,
docker compose -f docker-compose.yml -f docker-compose.relay.yml config:managed ats multicastservices.relay-caddy: container name "relay" is already in use by service {}managed varnish multicastservices.relay-varnish: container name "relay" is already in use by service {}managed caddy multicastmanaged caddymanaged multicastmanagedServices claiming
container_name: relay, fromconfig --format json— one claimant per set after the fix, where the two failing sets previously produced no output at all:The acceptance criteria that are about what must keep working:
managed caddystill activatesrelay-caddy— see the table abovemulticastservice is still activated undermanagedalone —config --servicesformanagedlistsmulticastBlast radius
One behaviour change worth naming rather than burying: on a gateway with relay disabled and multicast enabled,
relay-caddypreviously came up from themulticastprofile alone. It no longer does. That is the intended direction — the relay cache should not run on a gateway whose relay is disabled — but it is a change, not a no-op.An operator hand-running
--profile multicaststandalone also no longer getsrelay-caddy; they should use--profile caddy.Verification
scripts/validate-compose.shassertsEXPECTED_FAILin both directions, so emptying it in this same PR is what makes the lane assert the fix permanently rather than tolerate it. Negative control: revert thedocker-compose.relay.ymlhunk on a scratch branch and the lane returnsFAIL [managed ats multicast] rc=1instead of silently passing.Instrument note for reviewers: do not reproduce this with a compose newer than v2 without checking it first. The build in my agent pod (
Docker Compose version v5.5.0) mishandles repeated--profileflags —--profile managed --profile atsreturns fewer services than--profile managedalone, and repeating one profile twice differs from naming it once. Every number above was taken through theCOMPOSE_PROFILEScomma-separated form, which behaves correctly on that build and reproduces CI's readings exactly. The lane itself uses repeated--profileand is correct on CI's compose v2; if this repo's runner image ever moves to that compose, the lane would go blind rather than red. Recorded as a follow-up observation on BLO-34239, not fixed here.Not done here
The alternative fix is in magma — stop appending the
multicastprofile atcompose_manager/manager.go:1397, since the multicast service is covered bymanagedregardless. That is the more honest fix (the append has no case where it is both needed and correct), but it is a gateway behaviour change in a different repo and wants its own blast-radius review. This change makes the manifest safe against the profile set magma emits today either way.