Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add systemd analyze test #74066

Merged
merged 1 commit into from Dec 11, 2019
Merged

Add systemd analyze test #74066

merged 1 commit into from Dec 11, 2019

Conversation

7c6f434c
Copy link
Member

Motivation for this change

Trying to improve the NixOS test infrastructure; fixing the handling of shared dir in Python test driver, adding a copy_from_vm function, and adding a test exporting evereything systemd-analyze has to say.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @flokli

flokli
flokli reviewed Nov 24, 2019
View reviewed changes
nixos/lib/test-driver/test-driver.py Outdated
"""
# Compute the source, target, and intermediate shared file names
out_dir = os.environ.get("out", os.getcwd())
filename = re.sub(r".*/", "", source)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use pathlib here to calculate basenames and assemble paths:

https://docs.python.org/3/library/pathlib.html#methods-and-properties

nixos/lib/test-driver/test-driver.py Outdated
out_dir = os.environ.get("out", os.getcwd())
filename = re.sub(r".*/", "", source)
int_shared_dir = "/tmp/xchg"
shared_temp = tempfile.mkdtemp(dir=self.shared_dir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use tempfile.TemporaryDirectory as a context manager, it also takes care of cleaning up afterwards.

nixos/lib/test-driver/test-driver.py Outdated
shared_temp = tempfile.mkdtemp(dir=self.shared_dir)
shared_temp_basename = re.sub(r".*/", "", shared_temp)
shared_temp_in = "{}/{}".format(int_shared_dir, shared_temp_basename)
src_out = "{}/{}".format(shared_temp, filename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can join pathlib paths by just / in between them.

nixos/lib/test-driver/test-driver.py Outdated
src_in = "{}/{}".format(shared_temp_in, filename)
tgt = "{}/{}/".format(out_dir, target_dir)
# Copy the file to the shared directory inside VM
if not self.is_up():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current behaviour in the Machine class is to start machines if you request things from them, so we should do here, too.

nixos/lib/test-driver/test-driver.py Outdated
self.succeed("cp -r {} {}".format(shlex.quote(source), shlex.quote(src_in)))
self.succeed("sync")
# Copy the file from the shared directory outside VM
ret = subprocess.run(["mkdir", "-p", tgt])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably can also be done with python stdlib instead of shelling out to mkdir.

nixos/tests/all-tests.nix
@@ -260,6 +260,7 @@ in
syncthing-init = handleTest ./syncthing-init.nix {};
syncthing-relay = handleTest ./syncthing-relay.nix {};
systemd = handleTest ./systemd.nix {};
systemd-analyze = handleTest ./systemd-analyze.nix {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if we could split this into a separate PR, but fine to keep it around for now until the test driver additions are done.

@7c6f434c
Copy link
Member Author

Depends on/includes #74077

@7c6f434c 7c6f434c force-pushed the add-systemd-analyze-test branch 2 times, most recently from db34588 to 16ab350 Compare November 25, 2019 12:32
flokli
flokli reviewed Nov 25, 2019
View reviewed changes
nixos/tests/systemd-analyze.nix
machine.wait_for_unit("multi-user.target")

with subtest("Prepare output dir"):
machine.succeed("mkdir systemd-analyze")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can put the equivalent of doing a mkdir -p $(dirname $dst) into thecopy_from_vm function, so we don't have to repeat ourselves.

Also see nixos/tests/blivet.nix, which would benefit from that too (if it wouldn't have been broken, see #33496).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a change in #74077 of course, I can just better explain it with the example here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in-VM directory for systemd-analyze output that will later become the original source location for copy_from_vm

flokli
flokli reviewed Nov 25, 2019
View reviewed changes
nixos/tests/systemd-analyze.nix
Comment on lines +25 to +30
"systemd-analyze {} > {}/{} 2> {}/{}.err".format(
" ".join(args), tgt_dir, name, tgt_dir, name
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be overly generic. We only call it with one argument.

What about just executing machine.succeed(…) with a list of the "raw" commands:

machine.succeed(
    "systemd-analyze blame > blame.txt",
    "systemd-analyze critical-chain > critical-chain.txt",
    "systemd-analyze dot > dependencies.dot",
    "systemd-analyze plot > systemd-analyze.svg",
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overly generic

True, but cheap

list of the "raw" commands

The replacement list loses stderr of the dot subcommand which does contain some data that could be in principle useful.

Also, I do want to have a subdirectory that I can copy as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care that much. I think it might be more readable if written more explicitly, but your call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried. It becomes pretty Black-fragile. Agree on the point of more comments needed.

flokli
flokli reviewed Nov 25, 2019
View reviewed changes
nixos/tests/systemd-analyze.nix Outdated
run_systemd_analyze(["dot"], "dependencies.dot")
run_systemd_analyze(["plot"], "systemd-analyze.svg")

with subtest("Install the resulting data"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with subtest("Copy the resulting data into $out"):

flokli
flokli reviewed Nov 25, 2019
View reviewed changes
nixos/tests/systemd-analyze.nix

with subtest("Install the resulting data"):
machine.copy_from_vm("systemd-analyze", "")
machine.copy_from_vm("systemd-analyze/systemd-analyze.svg", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files seem to be missing here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No: I copy The Main Picture to the toplevel, and then the entire set of data as a subdirectory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, more comments and less abstractions would help understanding it ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less asbtractions mean more Black pain. More comments added

@7c6f434c
Copy link
Member Author

7c6f434c commented Nov 25, 2019 via email

@7c6f434c
Copy link
Member Author

7c6f434c commented Nov 25, 2019 via email

@7c6f434c
Copy link
Member Author

7c6f434c commented Nov 25, 2019 via email

@flokli
Copy link
Contributor

flokli commented Dec 1, 2019

With #74077 merged, can this be rebased on top of master?

@7c6f434c
Copy link
Member Author

7c6f434c commented Dec 1, 2019

OK, the hard part of rebase (building the kernel from master which is ahead of Hydra) is done, the test still seems to work (of course it was always rebased on #74077 so no surprise here)

@7c6f434c
Copy link
Member Author

7c6f434c commented Dec 1, 2019

Maybe I should re-mention @flokli

@flokli
Copy link
Contributor

flokli commented Dec 2, 2019

Can you move the replies sent via E-Mail into the thread? GitHub doesn't properly attach these, and keeping the discussion in threads seems like a good thing to do.

@7c6f434c
Copy link
Member Author

7c6f434c commented Dec 3, 2019 •
edited

Both options provided by GitHub (threaded and single-stream) are annoying in different ways, of course (which is why mailing lists, which are actually suitable for discussion, allow switching the navigation mode), but oh well… got around to starting a Firefox instance.

@flokli stuffed stuff into the threads

@flokli
Copy link
Contributor

flokli commented Dec 11, 2019

@GrahamcOfBorg test systemd-analyze

@flokli flokli merged commit b5c5ed2 into NixOS:master Dec 11, 2019
@flokli
Copy link
Contributor

flokli commented Dec 11, 2019

Thanks!

@7c6f434c
Copy link
Member Author

Thanks for the carefuk rereading of all that and for the explanations!

@flokli
Copy link
Contributor

flokli commented Dec 12, 2019

Thanks for staying through till the end, and enduring my pickyness 😆

@7c6f434c
Copy link
Member Author

I have to recognise that all of the pickyness was clearly actionable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli left review comments

Assignees
No one assigned
Labels
6.topic: nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@7c6f434c @flokli