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]>
RequireEventuallyServiceState succeeded on the first NDJSON entry for
the service matching the expected state, without checking any
remaining entries. During a recreation window compose ps can list two
containers for the same (scale: 1) service simultaneously (old and
new); the old one could still read "running" while the new one is
"starting", passing the poll prematurely on stale state.
Track whether any entry for the service was seen, and only succeed
once every one of them matches -- fail (poll.Continue) on the first
mismatch instead.
(docker-agent review on #14228)
Signed-off-by: Nicolas De Loof <[email protected]>
`--format=json` prints one JSON object per line, not a single object:
RequireEventuallyServiceState's json.Unmarshal into a single map broke
as soon as more than one container was listed for the service (a
scaled service, or the transient window during recreation where the
old and new containers are both listed) -- every poll iteration
reported "invalid JSON" and the function always timed out instead of
detecting the actual state.
Scan stdout line by line, matching entries by Service, and succeed as
soon as one reaches the expected state. Also check the exit code
before parsing so a real `compose ps` failure is reported as such
instead of a misleading "invalid JSON" message.
(docker-agent review on #14228)
Signed-off-by: Nicolas De Loof <[email protected]>
TestAttachRestart and TestUpDependenciesNotStopped each flaked on an
assertion checked exactly once, right after an unrelated signal on a
different channel to the daemon:
- TestAttachRestart counted "failing-1 | world" immediately after a
WaitForCondition on "failing-1 exited with code 1" reached 3. The
exited status comes from the /events stream; the log line comes
from a separate /logs?follow=1 connection (followStartedContainers
in pkg/compose/up.go). Nothing orders one relative to the other, so
the last restart's log line can still be in flight the instant the
3rd exited is observed.
- TestUpDependenciesNotStopped called RequireServiceState right after
seeing "hello app" in the attached log stream. That line reaching
the test is no guarantee the daemon-reported container state (read
via a fresh `compose ps` in a brand new process) has caught up yet.
Neither is a compose bug: `ps`/ContainerList and the log stream are
independent channels with no cross-channel ordering guarantee from
the engine, and compose does not cache container state. Both tests
now poll for the actual condition instead of using a log line as a
proxy for it: TestAttachRestart gets a second, bounded
WaitForCondition on the log count, and TestUpDependenciesNotStopped
uses a new RequireEventuallyServiceState (pkg/e2e/assert.go) that
polls `compose ps` instead of checking it once.
Signed-off-by: Nicolas De Loof <[email protected]>
Rename-only change, no behavior difference, no exported identifier
renamed (only parameter names of a few exported functions, which are
not part of the Go API).
- names that hide what a value is: parameters holding a *name* or *ID*
while named like the object — project→projectName, service→serviceName,
container→containerID — so string vs struct is readable at every call
site without opening the signature
- misleading receivers: monitor methods used c (reads as
client/container/CLI) → m; jsonWriter used p while its ttyWriter
sibling uses w → w
- cryptic abbreviations with non-trivial scope: nw (meant both
network.Summary and types.NetworkConfig) → network/networkConfig,
svc → service/serviceName/serviceCopy, oc/ocs → observedContainer(s),
cnts → serviceContainers, cnx → attachResponse
- container.Summary loop variables unified on ctr (was a c/ctr mix);
fillBindMounts also dropped its p/s/m single-letters, one of which
was shadowed by a loop variable of a different type
- option-struct identifiers unified on options (opts remains only for
variadic functional options); ambiguous ones get a precise name
(bindOptions, networkCreateOptions, projectOptionsFns)
Signed-off-by: Nicolas De Loof <[email protected]>
Before this, assertion libraries were mixed, sometimes
even in the same file.
git grep -l '"gotest.tools/v3/' | wc -l
75
git grep -l '"github.com/stretchr/testify' | wc -l
24
Signed-off-by: Sebastiaan van Stijn <[email protected]>
If running `up` in foreground mode (i.e. not `-d`),
when exiting via `Ctrl-C`, Compose stops all the
services it launched directly as part of that `up`
command.
In one of the E2E tests (`TestUpDependenciesNotStopped`),
this was occasionally flaking because the stop
behavior was racy: the return might not block on
the stop operation because it gets added to the
error group in a goroutine. As a result, it was
possible for no services to get terminated on exit.
There were a few other related pieces here that
I uncovered and tried to fix while stressing this.
For example, the printer could cause a deadlock if
an event was sent to it after it stopped.
Also, an error group wasn't really appropriate here;
each goroutine is a different operation for printing,
signal-handling, etc. If one part fails, we don't
actually want printing to stop, for example. This has
been switched to a `multierror.Group`, which has the
same API but coalesces errors instead of canceling a
context the moment the first one fails and returning
that single error.
Signed-off-by: Milas Bowman <[email protected]>
This keeps parity with v1, where only the containers explicitly
passed to `up` are torn down when `Ctrl-C` is hit, so any
dependencies that got launched (or orphan containers hanging
around) should not be touched.
Fixes#9696.
Signed-off-by: Milas Bowman <[email protected]>
Nicolas De Loof