cups vm tests: fix race condition, add more tests #338193
Open
+13
−6
Conversation
This file contains 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
While aimed at the cups-pdf printing package, this vm tests also serves as a test for the cups printing module in general.
There is a nasty race condition in the cups tests.
To understand what is going on, one must first note that
printers are installed in the vms with ensure-printers.service,
which is started as part of multi-user.target.
ensure-printers.service in turn triggers a start of
cups.service as it needs to connect to the local cups daemon.
This is what happens when the test runs:
1 the test waits for cups.socket or cups.service to start up
(subtest "Make sure that cups is up on both sides...")
2 after cups.service started
(it starts even in the "socket" case,
triggered by ensure-printers.service),
ensure-printers.service is started
3 the test tries to connect to the cups daemons via curl
(subtest "HTTP server is available too")
4 the test verifies the required printers are installed
("lpstat -a" called by subtest "LP status checks")
Usually, 3 needs some time, so ensure-printers.service
already installed all printers that are required by 4.
But if 3 is too fast, or if ensure-printers.service is too slow,
4 fails to find the printers it is looking for.
One can provoke the problem by adding
> systemd.services.ensure-printers.serviceConfig.ExecStartPre = "/run/current-system/sw/bin/sleep 10";
to the `nodes.client` configuration.
The commit at hand fixes the problem by changing 1:
Instead of waiting for cups,
it now waits for ensure-printers.service
(which in turn waits for cups.service and cups.socket).
This is also in accordance with the
subtest description in the code that promises to
"Make sure that cups is up [...] and printers are set up".
Add two new vm tests for the printing configuration that test `listenAddresses = []`, i.e., the situation where cups only listens on the unix domain socket `/run/cups/cups.sock`. This helps catching bugs like this: OpenPrinting/cups#985 NixOS#337748
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4534 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4592 |
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.
Description of changes
Motivated by #337748 (comment) , this adds
cups-pdfto cups'passthru.testslistenAddresses = []While developing the new tests, I hit a rare race condition in the existing vm tests, which is also fixed as part of this pull request. Details are explained in the respective commit message.
Notifying maintainers @domenkozar @matthewbauer and inspirator @risicle
Note: Although the inspiring bug occured on NixOS 24.05, I don't think this should be backported, as it's quite unlikely that such a bug happens again during the remaining lifetime of NixOS 24.05.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Result of
nixpkgs-reviewrun on x86_64-linux 1(nothing)
Add a 👍 reaction to pull requests you find important.