wantAbs was just wantSource under a different name, left over from an
earlier revision. Use wantSource directly.
(glours review on #14234)
Signed-off-by: Nicolas De Loof <[email protected]>
BindMountSource kept iterating after locating the bind mount at the
expected destination, so a hypothetical second bind entry at the same
target would trip a spurious mismatch even though the first entry
already confirmed the source. Break out of the loop once a match is
found, per docker-agent review feedback on #14234.
Signed-off-by: Nicolas De Loof <[email protected]>
.Mounts also lists named volumes and tmpfs entries at the same
Destination; matching on Destination alone let those shadow the
intended bind mount, either failing with a confusing volume Source or
silently passing against an empty tmpfs Source. Filter to Type ==
"bind" before comparing, as the function's name and doc promise.
Reported by docker-agent.
Signed-off-by: Nicolas De Loof <[email protected]>
filepath.Abs resolves a relative wantSource against the test process
cwd, not any project directory -- silently comparing against the
wrong base for a future caller passing a relative path, even though
the doc comment frames this as a general check. Reject a relative
wantSource explicitly instead.
(docker-agent review on #14234)
Signed-off-by: Nicolas De Loof <[email protected]>
TestOciRemoteProjectDirectory publishes a project whose only service
mounts a relative volume, then runs `up` on the oci:// artifact with an
explicit --project-directory pointing elsewhere. The relative volume
must resolve against that directory.
It currently fails against the pinned compose-go: LoadConfigFiles
defaults the working dir to the downloaded artifact's own cache
directory whenever a remote resource loader is involved, silently
overriding the explicit --project-directory the same way it would a
mere default -- docker/compose#14224. On Docker Desktop this surfaces
as a hard failure (the cache directory isn't a shared mount); on Linux
it silently mounts the wrong directory, matching the original report.
New BindMountSource check (pkg/e2e/checks.go) pins a service's bind
mount source to an exact expected path, for tests that need to verify
which working directory a relative volume path resolved against.
Needs the compose-go fix (github.com/ndeloof/compose-go@a626c70,
branch 14224-working-dir) to pass -- not yet reflected in go.mod here.
Signed-off-by: Nicolas De Loof <[email protected]>
glours' review on this PR (pullrequestreview-5277692679): stopLateStarter
only ever got armed inside stopOnFirstExit's own closure, so it caught a
service racing the on-exit cascade's sweep but not one racing
gracefulTeardown's (Ctrl+C/SIGTERM) identical one-shot stopApplication
sweep -- same root cause, same file, just the other trigger. Worse for
that path: Ctrl+C can land at t~=0, before monitor.Start even starts and
while s.start() (its own uncancelable context) is still climbing the
dependency graph, so most services may still be created, not running.
The only recovery today is a second manual Ctrl+C.
u.isTerminated already flags "termination is underway" and was already
set by gracefulTeardown (just not consulted for late-starter catching,
and set after stopApplication rather than before it). Extract the
late-starter watch out of stopOnFirstExit into its own listener,
stopLateStarters, armed on u.isTerminated regardless of which path set
it, and register it unconditionally (Ctrl+C works regardless of the
on-exit policy, unlike stopOnFirstExit's own listener). stopOnFirstExit
now sets isTerminated itself before its sweep, symmetric with
gracefulTeardown (also reordered to set it before stopApplication, not
after).
isLateStarter is extracted as a pure predicate, following the existing
shouldFollowStartEvent precedent, and unit-tested (TestIsLateStarter):
the previous fix (ca867482a) shipped only against a flaky test hitting
the on-exit case by chance, with no dedicated regression test at all.
Signed-off-by: Nicolas De Loof <[email protected]>
TestUpExitCodeFromContainerKilled hung for its full 2-minute budget on
CI, and its event transcript shows why: 'Aborting on container exit'
sweeps the application while the start phase — which runs on a
deliberately uncancelable context for SIGTERM management — was still
starting services; test-1 came up AFTER its stop and stayed up, so the
monitor never drained and up never returned.
The abort listener now watches the events stream past its trigger: any
container started after the sweep is a late starter from that race, and
gets stopped as it appears. Event-driven, so there is no listing window
to miss; idempotent stops make duplicates harmless. The exit code
semantics are preserved: the late starter's own 143 flows through
captureExitCodeFrom exactly as when the sweep wins the race.
TestUpExitCodeFrom* pass 5/5 locally.
Signed-off-by: Nicolas De Loof <[email protected]>
TestWatch/debian flakes on loaded runners during 'writing to a file
until Compose watch is up and running': that bootstrap loop ran under
poll.WaitOn's default 10s budget, which must absorb image pull/build,
container start and watcher initialization. Only the bootstrap gets the
2-minute budget — every later step keeps the sharp default so a real
sync regression still fails fast.
Signed-off-by: Nicolas De Loof <[email protected]>
TestAttachRestart kept failing residually after the re-attach anchoring
fix, and its new daemon-view dump told exactly why: the daemon holds
all three 'world' lines while compose printed two. The monitor returns
on the final die event — an events-channel fact — and up canceled the
global context on the spot, killing the re-attach log streams with the
last run's line still in flight: the exit notice outran the output that
preceded it.
Re-attach streams are now counted in a WaitGroup, and the monitor
wrapper waits for them to reach their natural EOF — guaranteed once the
containers exited — before canceling, under a bound that only protects
against a wedged daemon and is skipped entirely when the context is
already down (Ctrl-C). AttachRestart passes 5/5 locally with this
drain; it failed within 2 CI attempts without it.
Signed-off-by: Nicolas De Loof <[email protected]>
Wait listed running containers only, so a service that finished between
up and the listing — a fast run, or a service long done by the time the
user types the command — failed with 'no containers for project' in a
few milliseconds instead of returning its recorded exit code. This is
also the root cause of the TestWaitAndDrop flake: its 'faster' service
sleeps 2 seconds, less than the harness latency between the two steps.
The condition wait observes (container no longer running) already holds
for such a target: fall back to a full listing only when no container
is running, and let ContainerWait return the recorded status
immediately. Scoping the fallback to the previously-erroring path keeps
every other semantics intact — in particular a stale exited one-off can
never short-circuit a wait that has live containers to observe.
Exit-code propagation verified end to end: wait on an already-failed
service returns its code (7), not an error.
Signed-off-by: Nicolas De Loof <[email protected]>
Review finding on runEndTracker.Observe: recording nothing for an exit
event without a timestamp is the deliberate choice — the anchor is
evaluated by the daemon against its own log clock, so substituting the
local clock would introduce real skew mis-anchoring to paper over a
hypothetical daemon quirk, while dropping only degrades that container
to the pre-tracker fallback.
Signed-off-by: Nicolas De Loof <[email protected]>
The FinishedAt anchor did not cure the third-run loss on CI (still 2
worlds for 3 exit notices after a full minute, oldstable runner). The
remaining suspects are on both sides of the API: a line the daemon
never captured (copier torn down before a millisecond-lived run's
output) or a line compose still fails to relay. On timeout the test
now dumps `docker logs` for the container — ground truth that
discriminates the two on the next CI occurrence.
Signed-off-by: Nicolas De Loof <[email protected]>
The CI hardening in the previous commit turned TestAttachRestart's
flake into a reliable detector, and what it detected is a real loss:
re-attaching with since=StartedAt drops a fast run's first lines
forever, because the daemon starts copying stdout before it records
StartedAt.
Anchoring on the inspected FinishedAt fixes the common case but leaves
a narrower race CI still caught: when the new run itself finishes
before compose reacts to its start event, the inspected FinishedAt is
already the NEW run's own end, and the log window drops everything the
run printed — two worlds for three exit notices, the third never
arriving no matter how long you wait.
Both re-attach sites (attached up, logs --follow) therefore anchor on
the session's own record of the container's previous exit
(runEndTracker): the monitor delivers events in order, so the anchor
captured synchronously at start-event time is necessarily the previous
run's end — nanosecond-precise, immune to how fast the new run dies.
The inspected FinishedAt remains the fallback for a container the
session never saw exit, and a fresh container keeps no lower bound. A
unit test pins the ordering contract, including the fast-run sequence
CI caught.
Signed-off-by: Nicolas De Loof <[email protected]>
Three tests failed 9 CI runs across 5 branches this week, all on
asynchronous-observation races, none reproducible locally:
- RequireServiceState asserted on a single `compose ps` snapshot; the
daemon reports state transitions asynchronously from everything else
a test observes (TestUpDependenciesNotStopped saw 'created' while
the container's logs were already flowing). It now polls until the
state converges (15s bound).
- TestAttachRestart counted restart log lines in a snapshot taken as
soon as the third exit notice appeared; exit notices come from the
events monitor while log lines come from the re-attached logs
stream — two channels with no ordering between them. The count is
now awaited like the exit notices already were; a genuinely lost
line still fails, by timeout.
- TestUpExitCodeFromContainerKilled ran a full up+abort cycle under a
60s ceiling, once exceeded on a loaded oldstable runner; raised to
120s.
Signed-off-by: Nicolas De Loof <[email protected]>
The plan documented in both PRs had #14215 land first, then #14093's
compose-go bump delete the transient TestServiceHashContinuity. Merged
in the opposite order, main's pkg/compose tests no longer even compile:
richServiceFixture sets fields the container-spec layering turned into
promoted embeds (struct literals require go1.27 for that), and once
compilable two tests fail by construction.
- richServiceFixture is built by assignment;
- TestServiceHashContinuity is deleted, exactly as its own comment
prescribes: the layered compose-go is the reorder it existed to
outlive, its proof duty (pinned == historical bytes) is done, and
TestHashGoldenValues carries the continuity contract alone — it
PASSES on the layered compose-go, confirming the pinning preserved
every historical hash through the layering;
- pull_refresh_after, newly exposed at the service root, is appended
to serviceHashKeyOrder: the fallback already emitted it in that
exact position, so no hash moves — listing it only freezes the
layout and satisfies the coverage walker.
Signed-off-by: Nicolas De Loof <[email protected]>
The trimming of hash-excluded fields becomes trimServiceHashFields,
shared between ServiceHash and the continuity test so the exclusion set
cannot silently drift; pinRootKeyOrder reuses the package's sortedKeys
helper; the key-order coverage walker now follows encoding/json
promotion rules for non-struct embeds and unexported fields.
Golden hashes unchanged.
Signed-off-by: Nicolas De Loof <[email protected]>
ServiceHash digests json.Marshal of types.ServiceConfig, which couples
every recorded config-hash to the DECLARATION ORDER of compose-go
struct fields: encoding/json emits struct fields in that order and
flattens embedded structs at their embedding position. Any compose-go
refactoring that moves a field — such as the upcoming container-spec
layering, which regroups the whole struct — would change the bytes, and
with them the hash, of configurations that did not change at all: every
container recreated on the first `up` after an upgrade.
The hash now re-emits the marshaled object with its ROOT keys in a
frozen list reproducing the historical order (generated by reflection
over the last pre-layering compose-go), values byte-verbatim. For
today, the output is byte-identical to the direct marshal — proven by a
continuity test — so every existing container stamp stays valid: no
migration, no recreation, full backward compatibility. From the first
struct reorder on, the frozen list alone carries that continuity,
locked by golden-value tests; a root attribute added later is appended
in sorted order and, thanks to omitempty, only moves the hash of
configurations that use it — exactly like a field addition always did.
A reflection test fails when compose-go grows a root attribute missing
from the list, so extending the hash surface stays a reviewed decision.
Nested objects keep their own struct marshal: a reorder inside one of
them would still move hashes — the golden tests exist to turn that into
a caught, reviewed event rather than a silent side effect. Network and
volume hashes are unchanged (their structs are not being reordered) and
gain the same golden locks.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
compose-go's WithImagesResolved no longer special-cases a type=image
volume source that happens to match another service's name -- per
compose-spec/compose-go#929, such a source is always resolved as a
plain docker image reference, never treated as an implicit reference
to that service's locally built image. This surfaced as a real CI
failure after bumping the compose-go pin: the mock in
TestResolveImageDigests only expected calls for nginx/someimage/
hookimage and panicked on the now-unexpected lookup for "builder".
Update the test to expect the volume source to resolve like any other
image reference, and drop the stale comment in resolveServiceImageDigests
describing the old, now-removed behavior.
Signed-off-by: Nicolas De Loof <[email protected]>
compose-spec/compose-go#931 is merged: TestBuildAdditionalContextDisabledService,
red until now by design, passes for real against the released compose-go pin.
Signed-off-by: Nicolas De Loof <[email protected]>
docker-agent review: without --build, an image tagged
e2e-additional-context-base:latest left over from a previous run would
let up skip the build entirely, passing the test without exercising
the additional_contexts-to-a-disabled-service path it exists to lock
in.
Signed-off-by: Nicolas De Loof <[email protected]>
A service reachable only as another service's build additional_contexts
must still build even when its own profile is inactive: it's referenced
for its image, not started as a workload -- the same tolerance the
depends_on consistency check already grants a disabled, non-required
dependency, and the same case addBuildDependencies already special-cases
on the docker/compose side.
compose-go's own consistency check doesn't grant that tolerance yet: it
rejects the reference outright, failing the whole project load before
addBuildDependencies (or anything else, up included) ever runs -- for
up, build and up --no-build alike, exactly as reported.
This is expected to fail until compose-go bumps to include
compose-spec/compose-go#931.
Signed-off-by: Nicolas De Loof <[email protected]>
The previous fix only covered create/start. projectOrName backs every
other service-targeting command too (stop, kill, pause, unpause, logs,
rm, down, ps, events), so they still surfaced the same two problems: a
raw "no such service" error for a job name, and -- when
COMPOSE_PROJECT_NAME is set -- a silent no-op, since projectOrName's
label-driven fallback swallows the load error entirely.
Fold the check into projectOrName so every caller is fixed at once,
instead of duplicating the wiring per command. start.go's own
job-handling code, now redundant, is removed; jobTargetErr and
unselectedJobs (cmd/compose/run.go) are unchanged, just called from one
more place.
Adds unit coverage for projectOrName itself (including the
COMPOSE_PROJECT_NAME path and the typo-correlation edge case) and e2e
coverage for two more callers (stop, down) to confirm the fix lives in
the shared helper rather than being re-implemented per command.
Signed-off-by: Nicolas De Loof <[email protected]>
projectOrName falls back to a label-driven, file-less project on any
load failure when COMPOSE_PROJECT_NAME is set -- swallowing the load
error entirely, including "no such service" for a target that's
actually a declared job. Since that branch returns err == nil,
jobTargetErr (gated on err != nil) and rejectScheduledJobs (gated on
project != nil) were both skipped: `docker compose start migrate`
fell through to a label-driven start that finds no container for a
job that was never run, and exited 0 having silently done nothing.
Refuse a named job explicitly on that fallback path too, reusing
unselectedJobs from the previous commit.
Also adds the test coverage this and the previous fix (job-target
error messages on create/start) were entirely missing: create.go and
start.go had no unit tests at all, and pkg/e2e/jobs_test.go only
exercised up/run.
Signed-off-by: Nicolas De Loof <[email protected]>
jobTargetErr was called with the full [SERVICE...] argument list, but
compose-go's selection error only ever names the one target it
actually failed on. The loop matched any name in the list against the
job set, so `docker compose create totally-bogus migrate` (a real
typo plus a legitimate job) reported the job error instead of the
real "no such service: totally-bogus", masking the typo.
Match the name the error actually names, not any job name that
happens to share the invocation. unselectedJobs is extracted out of
jobTargetErr so start's own project-resolution fallback (next commit)
can reuse the same unselected-reload logic.
Signed-off-by: Nicolas De Loof <[email protected]>
materializeJobClosure walked job-typed depends_on edges without
re-checking triggers.manual: false on anything but the top-level run
target, so `docker compose run A` where A depends_on job B (manual:
false) silently ran B anyway.
manual: false declares a job harmful to trigger outside its
schedule; depends_on doesn't change who caused the execution or when
-- pulling B in to satisfy A's dependency is still the run command
causing that out-of-schedule execution, one hop removed. Refuse it
the same way the top-level check already does, before anything in the
closure is created.
Signed-off-by: Nicolas De Loof <[email protected]>
docker compose create <job> / start <job> failed with the raw
compose-go error "no such service: <name>", since project loading
rejects the job name before rejectScheduledJobs or any job-aware
logic ever runs -- unlike run, neither command knows how to detect
that a name is a declared job. The result gave no hint that jobs
exist or that run is the right command.
Jobs stay run-only by design; this only replaces the error message
when the target is a declared job, via a small helper (jobTargetErr)
shared by both commands, reloading unselected exactly like run's own
job-detection fallback. Any other "no such service" (a real typo)
keeps the original, precise error.
Signed-off-by: Nicolas De Loof <[email protected]>
Only the interpolated path (imagesOnly) and pkg/compose/publish.go
walked project.Jobs the same way they walk project.Services. Three
sibling paths in config.go were missed:
- runConfigImages (--images) only printed service images.
- lockModel (--no-interpolate --lock-image-digests) dropped the
top-level "jobs" key entirely along with every other non-services
key, instead of filtering it down to image/type:image-volumes like
services.
- resolveImageDigests (--no-interpolate --resolve-image-digests) only
built its pseudo-project from model["services"], so job images and
type: image volume sources never resolved -- confirmed by a new test
that failed against the first attempt at this fix (job image stayed
unpinned) until dressed as its own pseudo-project the same way
generateImageDigestsOverride in publish.go already does, since
WithImagesResolved only walks a project's Services, never its Jobs.
The extra complexity that last one picked up is split into
resolveServiceImageDigests (services, unchanged from before this
commit) and resolveJobImageDigests (new), called from a small
dispatcher.
Signed-off-by: Nicolas De Loof <[email protected]>
The fallback reload runProject uses to detect a job target passed
warnUnsupportedAttributes, the same option as the first, selective
load right before it -- which had already fired every warning once.
For the common case where the target is neither a service nor a job
(a plain typo), every finding got logged twice before falling through
to the original, correct error anyway.
skipUnsupportedAttributesWarning already exists for exactly this
secondary-load case (see its other call sites in compose.go and
completion.go); use it here too.
Signed-off-by: Nicolas De Loof <[email protected]>
Rebasing onto main pulled in the relay work (added after this branch
was cut), whose tests build ServiceConfig/PreStart literals against
the flat, pre-layering shape: DependsOn/Networks set directly, and
PreStart typed as []ServiceHook. Both are incompatible with the
ContainerSpec/WorkloadSpec split this branch adopts -- the promoted
DependsOn/Networks fields need go1.27 to set via a flat literal (this
module stays on 1.26.3), and PreStart is now []PreStartHook, a full
container specification, not a plain hook.
Signed-off-by: Nicolas De Loof <[email protected]>
Every publish safeguard and image-pinning path used to walk only
project.Services, leaving jobs invisible:
- the sensitive-data checks (literal environment values, env_file
scans, bind-mount warnings, build-only rejection) now cover jobs — a
job declaring AWS_SECRET_ACCESS_KEY=... was published without the
guard rail a service gets;
- the image-digest override pins job images too (jobs dressed as
services run through the exact WithImagesResolved semantics), so the
published artifact is reproducible for jobs as well;
- the application index references job images;
- `config --images` lists them.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
compose-go resolves each hook against its service at load time: the
model itself carries the full container specification a hook runs with,
and the runtime consumes it as-is through the standard create path —
the runtime-side merge helper goes away.
Consuming the full spec means honoring ALL of it:
- service references it may carry — volumes_from entries, and
service:-scoped network_mode/ipc/pid, inherited or declared — resolve
to live container IDs exactly like the service create path does; the
daemon knows nothing about service names and rejected them, failing
up for any service combining volumes_from with a pre_start hook
(locked by an e2e scenario).
- hook labels — declared or inherited — merge into the container's
labels, the runtime identification set winning on conflicts.
- the <API 1.44 network-connect fallback joins the HOOK's networks,
not the parent service's: a hook overriding networks was connected
to the wrong ones.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
Per the spec, any job can be triggered manually by an explicit run
command, its automated triggers notwithstanding — so run now accepts
scheduled jobs too. The exception is a job explicitly declared with
triggers.manual: false, which run rejects: meant for scheduled jobs
whose out-of-schedule execution would be harmful.
compose-go is bumped to the jobs-branch commit making Manual tri-state
(*bool) and allowing manual and schedule to be combined.
Signed-off-by: Nicolas De Loof <[email protected]>
Scenario-DSL coverage for the jobs entry points:
- up refuses a project declaring an active scheduled job before creating
any resource;
- run executes a manual job like a service, starting its depends_on
services first, and refuses a schedule-only job;
- a job's own env_file feeds its environment through run — the
materialization happens before environment resolution;
- a job depending on another job runs the dependency to completion
first, through the exact machinery a service dependency uses.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
pre_start hooks carry the full container specification, but the
init-container runner only consumed a handful of attributes (image,
command, user, env, workdir) — everything else was silently dropped.
Instead of wiring attributes one by one, the hook's specification is
merged over the service's ContainerSpec through compose-go's own file
merge machinery (override.Merge on the canonical yaml tree): command
and entrypoint replace, environment merges per key with the hook
winning, extra_hosts and dns accumulate entries, ulimits merge — the
exact per-attribute rules of multi-file compose, maintained in one
place. Every ContainerSpec attribute inherits this way, current and
future, with zero attribute-specific code.
The merged spec then runs through the standard create path
(getCreateConfigs) as a service-shaped one-off, so resources,
capabilities, dns, sysctls, logging... materialize exactly as they
would for a service container. Hook containers keep their minimal
labels and carry no container-number, so tooling telling replicas
apart does not count them.
The only deliberate exception is volumes: mounts inherit at runtime
through volumes_from — the only mechanism that shares the service's
anonymous and image volumes — and the hook's own volume declarations,
materialized by the standard path, take precedence per target. This is
what lets an init container get read-write access to a volume the
service mounts read-only (fixes: see PR).
e2e scenarios lock extra_hosts inheritance and accumulation, volume
override and completion, and the unit tests pin the merge rules.
Signed-off-by: Nicolas De Loof <[email protected]>
`docker compose run <job>` materializes the job — a ContainerSpec +
WorkloadSpec, the same layers a service is made of — as a service for
the one-off machinery: its profile is activated, the project narrowed
to its dependencies by WithSelectedJob, and the exit code flows back.
The materialization happens during project loading, BEFORE service
selection and environment resolution: the job's own env_file and
label_file resolve exactly like a selected service's would, unrelated
services' env_file still don't need to exist, and a target that is
neither a service nor a job keeps the precise selection error (the
retry is keyed on "the target is a declared job", never on matching an
error message). A service and a job may share a name: the service wins,
matching what the selector resolved.
A job may depend on other jobs: the whole job closure materializes, so
every depends_on edge resolves to a runnable service satisfying its
declared condition — a dependency job runs to completion through the
exact machinery a service dependency does — instead of dangling as an
unresolvable name. Job extensions (x-*) survive materialization.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
The model may declare jobs this runtime cannot execute yet. Commands
that materialize the application warn about them, and refuse an active
(profile-enabled) scheduled job before any resource is created — up,
create and start alike: silently not scheduling would break
expectations, while manual jobs just wait for an explicit trigger.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
compose-spec/compose-go#866 is merged: jobs top-level element, container
specification layered as ContainerSpec/WorkloadSpec, pre_start hooks
carrying the full container specification resolved at load time. Bump
to the merged head and adapt in the same movement — composite literals
setting moved fields wrap them into the embedded ContainerSpec /
WorkloadSpec (promoted field access was already source-compatible, so
this is literal-only, no behavior change), and pre_start handling is
typed against PreStartHook.
Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
The relay only dials an upstream and forwards bytes: it needs none of
Docker's default Linux capabilities. Run it with CapDrop: ["ALL"],
keeping only NET_BIND_SERVICE back via CapAdd since a route commonly
targets a privileged port (e.g. 80, 443) that the relay -- running
unprivileged as UID 65532 -- must still be able to listen on inside
its own container.
Validated end-to-end against a real provider project: a container on
the relay's network reaches the provider through its published, low
port with the hardened capability set in place.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
glours' review flagged the same bug class this PR already fixes for
start/restart: stopContainer (down.go) runs PreStop hooks
unconditionally, and runHook execs into the container -- which fails
against a shell-less relay. Guarded it the same way, with a regression
test.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
docker-agent review (via glours) flagged that startService's own
pre_start call site had the same gap this PR fixes for start/restart:
lowestNumberedContainer can resolve to the relay container, and
runPreStart shares its VolumesFrom with whatever container it is
given -- reachable via plain start/up whenever a provider service also
declares pre_start. Guarded it the same way, with a regression test.
Also, while touching this: mirror the TOCTOU/StateRemoving tolerance
already needed in removeServiceRelay onto ensureServiceRelay own
removal branch, which had the identical latent race; dedupe
isRelayContainer against the pre-existing isRelay(labels) in
monitor.go instead of reimplementing the same label check; and collapse
runPlugin two separate command == "up" checks into one.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
docker-agent review: removeServiceRelay called ContainerRemove
unconditionally, unlike the existing ensureServiceRelay which already
guards against two concurrency issues — a relay stuck in StateRemoving
(a concurrent ContainerRemove fails with "removal already in
progress") and a TOCTOU window between findRelayContainer and
ContainerRemove where another goroutine removes the relay first. Both
now degrade to the already-achieved outcome (relay gone) instead of
failing the whole up.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
A provider that stops publishing endpoints on a later `up` left its
previously created relay running: deployRelay was only evaluated when
endpoints were non-empty, so a zero-endpoint up fell through with no
cleanup. The generic reconciler is no help either — it returns early
for any service.Provider != nil, before the recreate logic that would
otherwise catch this. runPlugin now removes any existing relay when a
provider publishes no endpoint on an up, whether it never did or
stopped doing so since a previous run.
Separately, startServiceContainer and restartContainer had no
relay-awareness at all, unlike exec/cp which already refuse to target
one via checkRelayTarget. A relay is a shell-less, FROM-scratch binary
standing in for the service on the network: it has no filesystem or
process for secrets/configs injection or PreStart/PostStart/PreStop
hooks to act on. Nothing in the compose-spec schema forbids declaring
secrets, configs or hooks on a provider: service, so this was reachable
in practice, not just in theory. Both now skip those steps for a relay
container while still starting/restarting the container itself
normally.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
ensureServiceRelay runs concurrently per provider service (the shared
project mutex is released before this Docker API work). Two goroutines
reconciling relays that happen to share a consumer network can both
decide, from their own ContainerList snapshot, that the same relay
still needs connecting to it. Whichever runs second got the daemon's
"endpoint already exists" back from NetworkConnect, wrapped and
returned as a fatal error even though the relay was already in the
desired state.
Tolerate errdefs.IsConflict the same way the rest of the relay/network
reconciliation already does, instead of failing the run over a race
that already resolved to the right outcome.
(docker-agent review on #14236)
Signed-off-by: Nicolas De Loof <[email protected]>
relayIdentity only hashes image+routes, never network topology. When
a dependent service is added after the relay is already up, on a
network the relay isn't attached to, the relay's identity is
unaffected -- so ensureServiceRelay's running-container fast path
returned nil without ever looking at whether networkKeys had grown.
The new consumer could never reach the provider through it, silently:
no error, just an unreachable compose-native address.
ensureRelayNetworks now runs on every reuse path (running, stopped,
paused) before the state-specific branch decides what to do with the
container, connecting whichever networks in networkKeys the relay
isn't already on -- reusing NetworkSettings from the same
ContainerList call findRelayContainer already made, no extra inspect.
Networks the relay is already connected to are left untouched.
Signed-off-by: Nicolas De Loof <[email protected]>
relay/ (github.com/docker/compose-relay) is a separate go.mod: root
`go vet`/`golangci-lint`/`go test` never covered it, and the local
pre-commit hook explicitly excludes any directory with its own go.mod
for the same reason. Only the image build (relay-image-cross) was ever
validated in CI -- the Go source itself had no quality gate.
Adds relay-lint and relay-test bake targets, mirroring the root lint/
test Dockerfile stages: a build-base downloading go.mod's (currently
empty) dependency set, a lint stage running the same pinned
golangci-lint version as the root, and a test stage running go vet
then go test. relay-lint joins the "validate" bake group; both are new
matrix entries in the CI validate job.
relay/ gets its own .golangci.yml (the lint stage's build context is
"./relay" only, so it can't see the root config) with the same rule
set as root, minus what doesn't apply to a dependency-free module
(the e2e build tag, testify/gotest.tools-specific depguard entries).
Turning lint on for the first time surfaced 8 real findings in
relay/main.go and main_test.go (unchecked Close() errors, %v instead
of %w on a wrapped error, a redundant embedded-field selector, and
log.Fatalf after a defer that would never run) -- fixed here so the
new job is green from the start, not merged red.
Signed-off-by: Nicolas De Loof <[email protected]>
`compose stop` on a provider service short-circuited straight into
runPlugin(..., "stop"), never touching stopContainers -- so the relay
container this PR gives a provider service (RestartPolicyUnlessStopped,
a real persistent container) was left running and still Up in
`compose ps`, contradicting docs/extension.md's claim that stop
treats the relay like the service.
downService already stops/removes a provider's containers before
invoking the plugin; stop.go gets the same treatment: the service's
containers are stopped unconditionally, then the plugin's own stop
hook (if any) runs for provider services. start/restart needed no
change -- neither special-cases providers, so they already operate on
whatever containers match the service filter, relay included.
Verified locally: reverting just this change makes the new e2e steps
below fail on "stop halts the relay container".
Extends TestProviderPublishEndpoint with stop/start/restart coverage
of the relay container.
(glours review on #14193)
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
The 'Connection to a service' section only covered environment-variable
injection, while the relay mechanism was buried in the message-type
reference list. Move the relay narrative into its own subsection there,
illustrated with a sequence diagram showing the full path — the provider
publishing the endpoint, Compose deploying the relay on the dependents'
networks, and the application connecting at the compose-native address.
The publish-endpoint entry in the message list keeps the wire-format
contract and points to the new section.
Signed-off-by: Nicolas De Loof <[email protected]>
Sebastiaan van Stijn