Clean up daemon handling in the tests by Ericson2314 · Pull Request #5753 · NixOS/nix

Ericson2314 · 2021-12-09T15:42:27Z

Motivation

init.sh is tested on its own. We used to do that. I deleted it in 4720853 but I am not sure why. Better to just restore it; at one point working on this every other test passed, so seems good to check whether init.sh can be run twice.

We don't need to run init.sh twice, but I want to try to make our tests as robust as possible so that manual debugging (where tests for better or worse might be run ways that we didn't expect) is less fragile.

Split common.sh into the vars and functions definitions vs starting
the daemon (and possibly other initialization logic). This way,
init.sh can just source the former. Trying to start the daemon
before nix.conf is written will fail because nix daemon requires
--experimental-features 'nix-command'.

killDaemon is idempotent, so it's safe to call when no daemon is
running.

startDaemon and killDaemon use the PID (which is now exported to
subshells) to decide whether there is work to be done, rather than
NIX_REMOTE, which might conceivably be set differently even if a
daemon is running.

startDaemon and killDaemon can save/restore the old NIX_REMOTE as
NIX_REMOTE_OLD.

init.sh kills daemon before deleting everything (including the daemon
socket).

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

edolstra · 2021-12-09T19:13:55Z

What was the issue here? I re-run init.sh all the time without any problems.

Ericson2314 · 2021-12-09T19:26:35Z

@edolstra At one point everything was passing but init.sh. This has now grown a lot into fixing various issues. (I am about to push again.)

Ericson2314 · 2021-12-09T19:31:49Z

OK updated with lots more stuff. Curious if CI will agree with me, but seems good locally.

Ericson2314 · 2021-12-09T21:31:37Z

macOS build timed it out?

abathur · 2021-12-11T16:01:47Z

Probably the flaky build issue in #3605

Ericson2314 · 2021-12-12T00:17:59Z

Thanks

Ericson2314 · 2021-12-14T22:09:01Z

Rebased, hoping won't get that spurious error this time.

Ericson2314 · 2021-12-14T22:44:10Z

Yay it did!

Ping @edolstra and @regnat

Ericson2314 · 2022-01-23T22:36:22Z

@edolstra Does this look good to you? I would like to start pushing on this and other testing improvements again prior to getting back to fun feature work like RFC 92, so we have a firmer foundation before any more (potentially destabilizing) new feature.

tomberek

Test failures (repair.sh.test) are unrelated. Re-based onto master and functions as expected.

thufschmitt

I very much like the idea behind this.

As an alternative (maybe simpler, but also maybe stupid because I didn’t give it a lot of thoughs) scheme, how about

Making common.sh pure again (remove the startDaemon call from it)
Start the daemon in init.sh (and source it rather than execute it in mk/run_tests.sh so that the needed variables are properly propagated)
?

Ericson2314 · 2022-02-28T16:27:36Z

@thufschmitt I had thought about that, but I decided you (probably) had an intent of init.sh just preparing files and not running processes, and I decided that was a good thing :). I forget the exact reasons why.

Ericson2314 · 2022-02-28T17:08:34Z

@thufschmitt rebased!

dpulls · 2022-02-28T18:23:52Z

🎉 All dependencies have been resolved !

Ericson2314 · 2022-02-28T20:33:15Z

tests/init.sh

+set -eu -o pipefail
+
+# Don't start the daemon
+source common/vars-and-functions.sh


@thufschmitt to answer the question you asked on IRC a bit better, I didn't put the starting the daemon in init for these reasons:

Running tests without recalling init should still work. if one wants to examine state: the test itself starts and stops the daemon, rather than something else starting and it stopping.

init.sh merely does cleanup and setup of files. Besides removing stuff, it just creates new blank filesystem state. Stopping the daemon before removing the files is just for any rogue daemon, before it becomes harder to stop because the socket file was deleted.

The file is split because init.sh still needs the env vars.

Oh yes of course, it’s nice to be able to manually run init.sh once and then the test (I actually do that all the time)

Ericson2314 · 2022-03-09T03:43:51Z

@edolstra @thufschmitt approved. Is this OK with you?

Ericson2314 · 2022-03-25T02:24:26Z

@edolstra pinging again. It really would be good to merge this. It doesn't commit us to doing any more testing, it just makes the existing testing more robust.

Ericson2314 · 2023-01-16T19:31:55Z

I was inspired to return to this because #7600 has a devilishly hard to debug issue because it only cropped up in the daemon tests, and I was reminded how flaky that stuff is today.

In the process I found that the history had become quite messy, and so I am cleaning it up. ~~This is not just the first commit in what was a few cleanups. So the previous reply history may talk about changes that are no longer in here.~~ Ah I see that the original change probably didn't work on its own.

tests/init.sh

Ericson2314 · 2023-02-08T13:48:46Z

@thufschmitt Small reminder on reviewing this :). @roberth and I last night discussed some good ideas to clean stuff up further, and avoid the profusion of support files (to which this PR sadly adds one more). But I think it is good to land this in the meantime; having the daemon start and stop logic be more robust will make further cleanups easier.

thufschmitt

This has been stalled for too long, indeed. Let's merge!

thufschmitt · 2023-02-23T06:00:31Z

Well, now there are some conflicts 🙃

`init.sh` is tested on its own. We used to do that. I deleted it in 4720853 but I am not sure why. Better to just restore it; at one point working on this every other test passed, so seems good to check whether `init.sh` can be run twice. We don't *need* to run `init.sh` twice, but I want to try to make our tests as robust as possible so that manual debugging (where tests for better or worse might be run ways that we didn't expect) is less fragile.

Split `common.sh` into the vars and functions definitions vs starting the daemon (and possibly other initialization logic). This way, `init.sh` can just `source` the former. Trying to start the daemon before `nix.conf` is written will fail because `nix daemon` requires `--experimental-features 'nix-command'`. `killDaemon` is idempotent, so it's safe to call when no daemon is running. `startDaemon` and `killDaemon` use the PID (which is now exported to subshells) to decide whether there is work to be done, rather than `NIX_REMOTE`, which might conceivably be set differently even if a daemon is running. `startDaemon` and `killDaemon` can save/restore the old `NIX_REMOTE` as `NIX_REMOTE_OLD`. `init.sh` kills daemon before deleting everything (including the daemon socket).

Ericson2314 · 2023-02-23T16:35:51Z

Thanks @thufschmitt. For some reason when I rebased --- no conflicts!

This was referenced Dec 9, 2021

Harden tests' bash #5754

Merged

[backport 2.3] Allow testing with different daemons #5650

Merged

[backport 2.4] Recent merged testsuite improvements #5755

Merged

Ericson2314 force-pushed the init-twice-works branch from 57227e1 to 26bd675 Compare December 9, 2021 17:08

Ericson2314 marked this pull request as draft December 9, 2021 17:39

Ericson2314 force-pushed the init-twice-works branch from 26bd675 to 9131e1a Compare December 9, 2021 19:29

Ericson2314 changed the title ~~Make init.sh safe to run twice~~ Clean up daemon handling Dec 9, 2021

Ericson2314 marked this pull request as ready for review December 9, 2021 19:31

Ericson2314 force-pushed the init-twice-works branch from 9131e1a to 551675d Compare December 14, 2021 22:08

Ericson2314 mentioned this pull request Feb 17, 2022

Move build loop into the client #5056

Open

tomberek approved these changes Feb 20, 2022

View reviewed changes

Ericson2314 mentioned this pull request Feb 28, 2022

tests: Fix the start of the daemon #6179

Merged

thufschmitt reviewed Feb 28, 2022

View reviewed changes

Ericson2314 force-pushed the init-twice-works branch 2 times, most recently from bec7803 to 7a6a87e Compare February 28, 2022 17:08

Ericson2314 changed the base branch from master to 0.5-release February 28, 2022 18:32

Ericson2314 changed the base branch from 0.5-release to master February 28, 2022 18:32

Ericson2314 commented Feb 28, 2022

View reviewed changes

Ericson2314 force-pushed the init-twice-works branch from 7a6a87e to 273984c Compare March 6, 2022 22:23

stale bot added the stale label Sep 21, 2022

Ericson2314 force-pushed the init-twice-works branch from 273984c to f20e6b7 Compare January 16, 2023 19:26

Ericson2314 requested a review from edolstra as a code owner January 16, 2023 19:26

stale bot removed stale labels Jan 16, 2023

Ericson2314 changed the title ~~Clean up daemon handling~~ Make init.sh safe to run twice Jan 16, 2023

Ericson2314 commented Jan 16, 2023

View reviewed changes

tests/init.sh Outdated Show resolved Hide resolved

Ericson2314 added the with-tests Issues related to testing. PRs with tests have some priority label Jan 16, 2023

Ericson2314 force-pushed the init-twice-works branch from f20e6b7 to 3f6b136 Compare January 16, 2023 20:40

Ericson2314 changed the title ~~Make init.sh safe to run twice~~ Clean up daemon handling Jan 17, 2023

Ericson2314 changed the title ~~Clean up daemon handling~~ Clean up daemon handling in the tests Jan 20, 2023

thufschmitt self-assigned this Jan 23, 2023

Ericson2314 referenced this pull request in hercules-ci/nix Feb 7, 2023

WIP tests

04b896e

Ericson2314 requested a review from roberth February 7, 2023 22:56

Ericson2314 force-pushed the init-twice-works branch from 3f6b136 to f52667b Compare February 7, 2023 23:08

roberth mentioned this pull request Feb 10, 2023

Run test suite on NixOS #7778

Closed

14 tasks

thufschmitt approved these changes Feb 23, 2023

View reviewed changes

Ericson2314 added 2 commits February 23, 2023 11:31

Ericson2314 force-pushed the init-twice-works branch from f52667b to 87da941 Compare February 23, 2023 16:35

roberth approved these changes Feb 23, 2023

View reviewed changes

roberth merged commit b5bbf14 into NixOS:master Feb 23, 2023

Ericson2314 deleted the init-twice-works branch February 23, 2023 17:40

Uh oh!

Conversation

Ericson2314 commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Checklist for maintainers

Uh oh!

edolstra commented Dec 9, 2021

Uh oh!

Ericson2314 commented Dec 9, 2021

Uh oh!

Ericson2314 commented Dec 9, 2021

Uh oh!

Ericson2314 commented Dec 9, 2021

Uh oh!

abathur commented Dec 11, 2021

Uh oh!

Ericson2314 commented Dec 12, 2021

Uh oh!

Ericson2314 commented Dec 14, 2021

Uh oh!

Ericson2314 commented Dec 14, 2021

Uh oh!

Ericson2314 commented Jan 23, 2022

Uh oh!

tomberek left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Feb 28, 2022

Uh oh!

Ericson2314 commented Feb 28, 2022

Uh oh!

dpulls bot commented Feb 28, 2022

Uh oh!

Ericson2314 Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thufschmitt Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Mar 9, 2022

Uh oh!

Ericson2314 commented Mar 25, 2022

Uh oh!

Ericson2314 commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Feb 8, 2023

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt commented Feb 23, 2023

Uh oh!

Ericson2314 commented Feb 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Ericson2314 commented Dec 9, 2021 •

edited

Loading

Ericson2314 Feb 28, 2022 •

edited

Loading

Ericson2314 commented Jan 16, 2023 •

edited

Loading