tests-integration: Serialize the install-config writer against readers - #2520
Merged
Merged
Conversation
cgwalters
requested changes
Sep 30, 2026
| /// Read a configuration fragment found by a directory scan, returning `None` | ||
| /// if it was removed since the scan: `/run` and `/etc` are writable, so a | ||
| /// fragment can vanish while we're loading, and that shouldn't be fatal. | ||
| fn read_fragment(path: &std::path::Path) -> Result<Option<String>> { |
Collaborator
There was a problem hiding this comment.
No, we should only fix the tests
Contributor
Author
There was a problem hiding this comment.
Dropped the config.rs commit; the PR is now test-only (f4264de, your sign-off kept).
| /// Serializes the tests that read the install config against the one that | ||
| /// adds a fragment to /run/bootc/install, since libtest runs them concurrently | ||
| /// and the other readers would otherwise see its temporary config. | ||
| static INSTALL_CONFIG_LOCK: Mutex<()> = Mutex::new(()); |
Collaborator
There was a problem hiding this comment.
I think the test that writes config needs to be mutually exclusive with the others, don't we want a RWLock?
Contributor
Author
There was a problem hiding this comment.
Yes, now a std::sync::RwLock (the container tests are threads of one process): printconfig --all takes it for writing, install config for reading. A probe that polls for the fragment in the reader saw it 0/200 runs with this and 200/200 without exclusion; details in the PR body. @cgwalters, ready for another look.
cgwalters-bot
force-pushed
the
bot/printconfig-race
branch
from
October 1, 2026 00:33
b5e7c08 to
f4264de
Compare
cgwalters-bot
changed the title
libtest runs the container tests concurrently as threads of one process, and "printconfig --all" writes and then removes a fragment in /run/bootc/install that "install config" can pick up while scanning the same directory. That made "install config" flake with a bare "No such file or directory" when the fragment vanished between the scan and the read, and it could also see the fragment half-written or get the wrong config. This is a race between the tests, so fix it there: the test that writes config takes a write lock and the ones that read it take a read lock. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
Contributor
Author
|
Rebased onto main; 1 commit, no content change. Generated-by: https://github.com/cgwalters/#llms |
cgwalters-bot
force-pushed
the
bot/printconfig-race
branch
from
October 1, 2026 03:02
f4264de to
9dec807
Compare
cgwalters
enabled auto-merge (rebase)
October 1, 2026 15:16
cgwalters
approved these changes
Oct 1, 2026
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
2 participants
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.
The
install configcontainer integration test flakes witherror: Loading configuration: No such file or directory (os error 2)(seen on the sealed leg of #2500, and in a fork run on 2026-09-24). libtest runs the container tests as concurrent threads, andprintconfig --allcreates then removes/run/bootc/install/10-test.toml, whichbootc install print-configurationin theinstall configtest can find in its directory scan and then fail to open (or read half-written).Per review this is fixed only in the tests: a shared
RwLockincontainer.rs, taken for writing by the test that writes install config and for reading by the one that reads it. Astd::sync::RwLockis enough since all the container tests are threads of onebootc-integration-testsprocess.Testing, on a 16-core devspace with
BOOTC_base=quay.io/fedora/fedora-bootc:44:just validatepassed;just test-containerpassed (unit tests and all 10 container integration tests), andjust test-container-integrationpassed 5/5 more runs.bootc-integration-tests container --test-threads=2 config) 300 times failed 0 times, but so did the same on unpatched main, so the race window is too narrow to reproduce directly that way.install configpoll for the writer's10-test.tomlfor 200ms before reading: with the lock it never saw it (0/200 runs); with the writer taking the lock for reading instead of writing it saw it in 200/200.The
Signed-off-by: Colin Walters <walters@verbum.org>on this commit (f4264de) was kept from cgwalters's approval of the review draft (cgwalters-forge#24 (review)) through the rework he asked for in #2520 (review) (dropping the product-code commit and switching to aRwLock).Generated-by: https://github.com/cgwalters/#llms