test: fix some flakes / wait for conmon to exit - #673
Open
kolyshkin wants to merge 10 commits into
Open
Conversation
kolyshkin
force-pushed
the
test-wait-for-conmon
branch
4 times, most recently
from
August 19, 2026 02:10
44fe1b5 to
9924730
Compare
$CONMON_PID_FILE is a single path per test, but a test may start more than one conmon (one for the container and one for an --exec), in which case the second one overwrites the first one's pidfile, and there is no way to tell what the first conmon's pid was. Have start_conmon_with_default_args use a pidfile of its own for every conmon it starts, and expose the pid of the one just started as $CONMON_PID, so that tests do not have to deal with the path at all. $CONMON_PID_FILE stays for the tests that run conmon directly. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The test waits for the container to reach the "stopped" state and then
reads the container log, expecting the container output to be there.
This is racy: the container being stopped does not mean conmon is done
writing the log, and the test can (and in CI does) read an empty log:
not ok 92 attach: container finishes immediately without --stdin
# (from function `bail-now' in file conmon/test/test_helper.bash, line 519,
# from function `assert' in file conmon/test/test_helper.bash, line 635,
# in test file conmon/test/07-attach.bats, line 37)
# `assert "${output}" =~ "Container stopped!" "'Container stopped!' found in the log"' failed
#| FAIL: 'Container stopped!' found in the log
#| expected: =~ Container stopped\!
#| actual: ''
conmon having exited does mean the log is complete, so add a helper
waiting for that, and use it in the affected test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The same pattern as fixed by the previous commit -- wait for the
container to be stopped, then read a conmon log -- is used in a few more
places. These have not been seen to fail, but the race is the same, so
convert them, too.
Where a test starts more than one conmon, $CONMON_PID is saved right
after the start, and only the conmon(s) owning the log(s) actually read
are waited for.
The most common instance of the pattern is run_conmon_with_default_args
itself, so the barrier goes right into it, covering all of its callers.
This one has been seen to fail:
not ok 45 ctr logs: k8s partial message
# `assert "${output}" =~ "stdout P"' failed
#| expected: =~ stdout P
#| actual: ''
in the coverage job, where conmon is built with --coverage and is thus
slow enough to lose the race more often.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The intent was to wait for conmon to exit, but the argument is the name of the pidfile rather than the pid it contains, so bash rightfully complains wait: `/tmp/.../conmon-pidfile': not a pid or valid job spec which the test then discards along with the rest of the errors. In other words, this waits for nothing at all. Use wait_for_conmon_exit instead. The sleep above it is left as is: it is what gives the sync pipe reader time to write out what conmon sent it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
test-wait-for-conmon
branch
from
August 19, 2026 02:14
9924730 to
d881273
Compare
kolyshkin
requested review from
jankaluza and
jnovy
and removed request for
jankaluza
August 19, 2026 02:49
Collaborator
Author
The GitHub Actions default job timeout is 6 hours, so a job that hangs squats a runner for the rest of the day instead of failing. This is not hypothetical: today both the setup step (apt, five git clones, wget) and the test step (which starts with a podman pull) have been seen hanging on what looks like stalled network I/O, and one such job was still sitting there 2.5 hours later. Give the jobs timeouts a few times their normal runtime, so a hang fails in minutes rather than hours: the conmon job normally takes about 4 minutes and the cri-o one about 40. Note static.yml already sets one (360, the default), left alone here. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This script clones five repositories, downloads a tarball and a repo key, and runs apt, and not one of those has a usable timeout by default: - git has none at all; - curl has no total timeout and no retries, only a 300 second connect timeout, so a connection that stalls mid-transfer hangs forever; - wget does have a 900 second read timeout, but also 20 tries by default, which adds up to hours; - apt retries nothing by default. This is not theoretical: this step has been seen hanging for 49 minutes, and was still going when the job was cancelled. Give each of them a bound. The git ones get GIT_HTTP_LOW_SPEED_LIMIT and GIT_HTTP_LOW_SPEED_TIME, which abort a connection that transfers less than 1 KiB/s for a minute, and cover all five clones at once. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setup_suite pulls the image the test rootfs is made of. It is the only network access in the whole suite, podman has no timeout of its own, and a hang there stops the suite before a single test runs -- with no output at all, since bats shows setup_suite's output only once it completes. The test step has been seen hanging for 35 minutes with exactly that signature. Bound the pull, and say in the failure message that a timeout is a possibility. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five seconds for a container to change state is not much when the CI runner is loaded, and it has been observed to be too little: not ok 51 runtime: container execution with multiple log drivers not ok 101 ctrl: resize the terminal, negative width and height #| FAIL: timed out waiting for 'stopped' from conmon-test-... The helper polls, so a longer limit costs nothing when things are well -- it returns as soon as the state is reached -- and only comes into play where the alternative is a spurious failure. Make it 30 seconds. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"integration: exec exit codes work correctly" runs six podman commands, of which exactly one -- the initial probe -- has a timeout. The rest can hang forever, and one of them did: ok 67 exec requires proper arguments <17 minutes of nothing> The job has exceeded the maximum execution time of 20m0s Test 68 is this one. Nothing is printed while it hangs, because bats shows a test's output only once the test ends, so the suite just goes quiet -- and before the job timeout added earlier in this series, quiet for up to six hours. The container creation is the likeliest culprit: container_id=$(podman --conmon $conmon_path run -dt ... sleep 30) a command substitution waits for stdout to be closed, and conmon is started by podman with that very stdout -- this test runs the freshly built conmon on purpose, so it is exactly the thing that may hold it open. Wrap every podman invocation here in a timeout. A hang now fails the test in a minute, with the rest of the suite still running. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
OK this one is ready but I'll wait for review from @jnovy first |
The Acquire::*::Timeout options added in the previous commit turned out not to be enough. They apply to individual requests of the acquisition method, while the frontend will happily wait for that method forever, which is exactly what happened: 04:56:18 + sudo apt -o Acquire::Retries=3 ... update 04:57:27 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease [126 kB] <19 minutes of nothing> 05:16:30 The job has exceeded the maximum execution time of 20m0s Five of ten parallel jobs died that way in one experiment, all of them after apt gave up on the azure.archive.ubuntu.com mirror (a screenful of "Ign:") and fell back to archive.ubuntu.com. Bound the whole command instead, and retry it a couple of times, since a stalled mirror is a transient thing. Also stop waiting on the dpkg lock, which a previous timed out run may well have left behind, and switch to apt-get, whose CLI is the one meant for scripts. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
attach: container finishes immediately without --stdinis flaky: it waits forthe container to reach the
stoppedstate and then reads the log, but thecontainer being stopped does not mean conmon is done writing the log, so the
test can read an empty one.
Seen in runc CI (the runc
validate / conmonjob runs this suite against arunc build):
A rerun of the exact same commit passed.
conmon having exited does mean the log is complete, so this waits for that.
Waiting for a specific conmon needs its pid, and
$CONMON_PID_FILEis a singlepath per test that every conmon a test starts overwrites, so the first commit
gives each conmon started by
start_conmon_with_default_argsits own pidfileand exposes the pid as
$CONMON_PID. The rest builds on that: fix the flakytest, then convert the remaining "wait for stopped, then read a log" places,
which have not been seen to fail but share the race.
Testing: the full suite passes locally with
-j $(nproc). Note that I couldnot reproduce the race itself locally -- instrumenting the new helper showed
conmon had always already exited by the time it looked -- so this is verified
as "correct barrier, no regressions" rather than "observed to fix the flake".
The last commit fixes an unrelated but similar thing spotted on the way:
04-runtime.batsdidwait $CONMON_PID_FILE 2>/dev/null || true, i.e. itwaited on a filename and swallowed the error, so it waited for nothing.
Update: rebased onto main, and the barrier is now in
run_conmon_with_default_argsitself (amended into "test: wait for conmon in the remaining log checks" rather than added as a separate commit).That helper is the most common instance of the pattern -- 13 call sites across
02-ctr-logs.bats,04-runtime.batsand10-ctrl.bats, every one of which reads a log right after -- so one barrier there covers all of them.It also turns out this one is not merely theoretical: the same race fails the
coveragejob, where conmon is built with--coverageand is thus slow enough to lose it more often. Seen on an unrelated PR's run:Same signature as the
07-attach.batsone: the runtime reportsstopped, the log is read, and it is still empty. So this PR fixes two observed flakes, not one.Re-tested after the rebase:
02-ctr-logs,04-runtime,07-attach,08-execand10-ctrlall pass locally.Update 2: four more commits, all about CI not hanging for hours.
Today's runs kept getting stuck -- one job sat for 2.5 hours before being cancelled -- and it was never the same place twice:
hack/github-actions-setuphung for 49 minutes on one run, the test step for 35 minutes on another. Both consist of network access with no timeout on it, and the GitHub Actions default job timeout is 6 hours, so nothing ever gave up.ci: set job timeouts--timeout-minuteson the jobs (conmon 20, cri-o 90, coverage 20, lint 10), a few times their normal runtime. A hang now fails in minutes instead of squatting a runner for the rest of the day.ci: time out and retry network access in the setup script-- the script clones five repositories, downloads a tarball and a repo key, and runs apt. git has no timeout at all; curl has no total timeout and no retries; wget has a 900 second read timeout and 20 tries by default; apt retries nothing. All of them get a bound now, the clones viaGIT_HTTP_LOW_SPEED_LIMIT/GIT_HTTP_LOW_SPEED_TIME.test: time out the image pull--setup_suitepulls the image the test rootfs comes from. It is the only network access in the suite, and a hang there stops everything before a single test runs, silently: bats showssetup_suiteoutput only once it completes.test: bump the wait_for_runtime_status timeout-- 5 seconds turned out to be too little on a loaded runner:The helper polls, so a longer limit costs nothing when things are healthy; 30 seconds now.
Commits 1-4 fix races (read a log before conmon wrote it); 5-8 stop CI hanging on stalled I/O. Happy to split the latter into their own PR if that reads better.