Uh oh!
There was an error while loading. Please reload this page.
ci: skip the cri-o tests that do not test conmon - #684
Conversation
d68866a to
86e84ceCompareThe cri-o suite takes about 40 minutes, most of the time this workflow spends. It has been trimmed before -- image, policy, crio-wipe and, in intent, seccomp -- but not in a while, and it has grown since. Measured with `bats --timing` on a full run, the suite spends 8943 seconds of test time across four parallel jobs, and a third of that goes to test files with no conmon in them at all: cri-o's image and registry handling, its metrics endpoints, its configuration reloading and status commands, the security profiles the runtime applies, and its device and plugin plumbing. The largest single item is apparmor.bats, one test at 238 seconds. Drop them, keeping everything that runs containers: ctr, pod, timeout, exec, restore, logs, cgroups, namespaces, network and the rest. The removal itself is rewritten. Until the previous commits moved the cri-o tree out of root's home, this line sudo rm -f "$CRIO_DIR"/test/seccomp*.bats removed nothing: the glob is expanded by the shell running the step, which could not read the directory, so it matched nothing, was passed through literally, and rm -f swallowed it. That is how seccomp_oci_artifacts.bats and seccomp_runtime_handler.bats kept running for years despite the comment above them saying they do not. So the names are spelled out, and rm is not given -f: a file cri-o renames now fails the job, rather than quietly rejoining the run. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
86e84ce to
ec4d800CompareThe cri-o job runs two matrix arms, one of about 30 and one of about 40 minutes, and GitHub's default fail-fast cancels the rest of a matrix as soon as one of them fails. So a failure in the short arm throws away the long one, whatever it was about to tell us -- and as it is usually the setup, not the tests, that fails there (a stalled Ubuntu mirror, say), what gets thrown away is the actual test result. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
jnovy
left a comment
There was a problem hiding this comment.
LGTM. Clean, well-motivated CI improvement. Two commits, one file, no production code touched.
Commit 1 - skip non-conmon cri-o tests:
The old removal line had a subtle bug that the PR description documents well: the seccomp*.bats glob was expanded by the unprivileged step shell, but $CRIO_DIR pointed into /root (unreadable), so the glob matched nothing, rm -f swallowed the error, and ~300s of seccomp tests ran every time despite the stated intent to skip them.
The replacement is correct and well-crafted:
- Bash array with categorized, commented entries is easy to maintain and review against upstream.
"${skip[@]}"and"$CRIO_DIR/test/$t.bats"are properly quoted throughout.- Deliberate omission of
-fis the right call: if cri-o renames or removes a test file, the job fails loudly rather than silently re-admitting tests under a new name (the exact failure mode the old glob had). - Dropping
sudofrom thermis correct given that #678 moved the cri-o tree out of root's home. - 28 files in the skip list, matching the ~3200s / one-third-of-suite figure from the bats timing data.
Commit 2 - fail-fast: false:
Standard good practice for a two-element matrix where each job takes ~30 minutes. No reason to discard one result because the other failed. Comment explains the rationale concisely.
No bugs, no security concerns, good shell scripting practices.
TomSweeneyRedHat
commented
Aug 20, 2026
Very, Very Nice @kolyshkin |
jnovy
left a comment
There was a problem hiding this comment.
LGTM.
Commit 1 (skip non-conmon cri-o tests): The old sudo rm -f with unquoted globs was silently a no-op - the glob was expanded by the unprivileged shell against an unreadable /root directory, matching nothing, and -f swallowed the error. The replacement bash array + loop is correct, properly quoted, and the deliberate omission of -f is a nice touch - renamed or removed upstream test files will now cause a loud failure instead of silent re-admission. Dropping sudo is consistent with #678 moving the tree out of root's home. Categories are well-organized.
Commit 2 (fail-fast: false): Standard good practice for the two-element matrix where each job takes ~30 minutes.
The cri-o suite is the bulk of this workflow: about 40 minutes, against 3 for critest and 5-9 for the setup. It has been trimmed before --
image,policy,crio-wipeand, in intent,seccomp*-- but not in a while, and cri-o has grown a lot of tests since.Where the time goes
Measured by running the suite with
bats --timing(#683), which puts a duration on every TAP line. 481 tests, 8943 seconds of test time spread over four parallel jobs:What is dropped
28 files, about 3200 seconds -- a third of the suite -- none of which involve conmon:
image,image_remove,image_volume,imagefsinfo,inspecti,cert,tls_config,namespaced_auth_dir,oci_artifacts,artifact_additional_stores,seccomp_oci_artifacts,policyapparmor(one test, 238s),seccomp_notifier(conmon's seccomp notify support was removed in Drop seccomp notify support #671),seccomp_runtime_handlerreload_config,reload_system,annotation_migration,workloads,inject_gomaxprocs,status,crio-check,crio-wipe,profilecri-metrics,metricscdi,nriEverything that runs containers stays:
ctr,pod,timeout,ctr_timeout,exec_*,conmon_track,restore,logs,cgroups,namespaces,network,ctr_seccomp,devices,ctr_usernsand the rest.The glob that never worked
sudo rm -f "$CRIO_DIR"/test/seccomp*.bats "$CRIO_DIR"/test/image.bats ...Until #678 moved the cri-o tree out of root's home, this removed nothing: the glob is expanded by the shell running the step, as the user, and
$CRIO_DIRwas under/root, which that user cannot read. So it matched nothing, was passed through literally, andrm -fswallowed it -- which is whyseccomp_oci_artifacts.batsandseccomp_runtime_handler.batsare in the timing table above, 300 seconds of tests we have believed to be off for years. The paths without a glob were removed correctly all along.So the names are now spelled out, and one that does not exist is an error rather than a no-op: a file cri-o renames should be noticed, not quietly let back into the run.
Rebased on top of #678, so the removal needs no
sudoat all -- the tests are ours now.