Stop the test suite touching the developer's machine on macOS, and fix three more macOS-only defects - #1623
Merged
Merged
Conversation
The guard that stops a test driving the real service manager asked whether podman.UnitLifecycle was nil, on the reasoning that a test which wants the lifecycle installs a stub there. On macOS the launchd manager is assigned from an init() in internal/services, so the var is never nil and the guard has never once fired: every test that reached StartUnit, StopUnit or RestartUnit drove the launchd of whoever ran the suite. What that cost is not theoretical. A single go test ./... booted out the developer's own com.lerd.lerd-nginx job, enabled it again and re-bootstrapped it against a plist inside the test's temp directory, which the test then removed on cleanup. The job was left registered to a path that no longer exists, so the next restart of nginx had nothing to load, and a com.lerd.lerd-php81-fpm job for a version nobody installed was left behind the same way. Isolating HOME, which the tests already do, moves the plist file but not the launchd domain it is bootstrapped into, and no per-test domain exists to move it to. So the manager a platform installs at init is now recorded apart from whatever is in UnitLifecycle, and the refusal covers both the unset case and the platform one. A stub a test assigns directly is still told apart from the real thing and still drives the lifecycle, which is what keeps the tests that do exercise it working.
Three checks in the installer suite could not pass on a Mac. Two of them reach for setsid to get a process with no controlling terminal, which is util-linux and has no macOS equivalent, so both died at command not found rather than testing anything. The third allocates a pty through script(1), which exists on both, but spelled its arguments the util-linux way, and the BSD one rejects -c outright. Both now go through a helper that picks the form the platform accepts. Perl's POSIX::setsid stands in where setsid is missing, since perl is in the base install on macOS and on every distro lerd targets, and the pty helper puts the file and the command in whichever order the local script(1) wants. The BSD side also needs stdin held open a moment longer, or it reads EOF before the prompt it is meant to answer has been printed. The suite now runs 63 for 63 on macOS instead of 60, which matters because these particular checks cover the path that decides whether lerd install can ask a question at all, and that path is one a Mac contributor is as likely to change as anyone.
…its plist RemoveContainerUnit removed the plist and stopped there, and the comment above RemoveQuadlet already claimed this was what kept an orphan agent out of the LaunchAgents directory. The file went, but launchd was never told, so the job stayed registered against a path that no longer existed and went on answering launchctl list forever. That is where a com.lerd.lerd-kafka job for a service disabled long ago was still coming from. The stop goes through StopUnit rather than a bootout written out here, so it lands on the one funnel that already knows how to refuse a real service manager when a test is driving it, and Linux keeps the behaviour it had. A guard now sits under the launchctl helper as well, for the same reason the write guard sits under writePlist. Isolating HOME moves the plist but not the domain it is bootstrapped into, and launchd offers no per-test domain to move it to, so a mutating verb reached from a test has nowhere safe to land and says so instead of landing on the developer's own. Reads are left alone because several paths depend on them and none of them change anything.
domain add appended the TLD to whatever it was given, so a name that already ended in one came back doubled: shop.acme.test was stored as shop.acme.test.test, which nginx served under a name no resolver would ever be asked for. The help says to pass the name without the TLD, but the domain is the thing people have in mind and the thing they type, and remove had the same gap, so a mistyped add could not be undone by repeating it. The TUI has trimmed a trailing TLD off this input since it gained the inline domain editor. The CLI now does the same, through one helper both subcommands call, which is what makes the two agree about what a domain argument means.
util-linux script runs the command it is given through $SHELL, not through sh, so the tty check ran its payload under whatever the developer's login shell is. install.sh only skips main when BASH_SOURCE says it was sourced, and no other shell sets it, so under zsh or fish or dash the guard read as "not sourced" and sourcing the installer to get at ask ran the installer instead. The check then failed on a GOT_YES that was never printed, and the suite spent that time running install against the throwaway HOME bats had set. The form this replaced wrapped the payload in bash -c, which is what kept it away from $SHELL, and the BSD branch still does. Pinning SHELL for the call is the same guarantee without a second layer of quoting.
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
1 participant
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.
Records the manager a platform installs at init apart from whatever sits in
UnitLifecycle, so the under-test refusal covers the platform case and not only the unset one. A stub a test assigns directly is still told apart from the real thing and still drives the lifecycle, which is what keeps the tests that do exercise it working. A full suite run now leaves the launchd unit list byte-identical, where before it left nginx pointing at a deleted temp plist.Removing a container unit goes through
StopUnitbefore dropping the plist, so launchd is told the job is going rather than left holding a file that is gone. Routing it through that funnel rather than a bootout written out at the removal site means it lands on the one path that already refuses a real service manager under test, and Linux keeps the behaviour it had.A guard now sits under the
launchctlhelper too, for the same reason the write guard sits underwritePlist: isolatingHOMEmoves the plist but not the domain, and launchd offers no per-test domain, so a mutating verb reached from a test has nowhere safe to land and says so. Reads are left alone because several paths depend on them and none of them change anything. It earned its place immediately by catching three further leak paths the first fix did not cover.The installer checks that need a process with no controlling terminal fall back to perl's
POSIX::setsidwheresetsidis missing, and the pty helper orders its arguments the way the localscript(1)wants, with stdin held open long enough for the BSD one to see the prompt. That suite goes from 60 to 63 of 63 on macOS.lerd domain addandlerd domain removenow take the name with or without the TLD through one helper both call, which is what makes them agree with the TUI about what a domain argument means.Closes #1622