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

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

@7c6f434c 7c6f434c commented Nov 24, 2019

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

"""
# Compute the source, target, and intermediate shared file names
out_dir = os.environ.get("out", os.getcwd())
filename = re.sub(r".*/", "", source)

This comment has been minimized.

@flokli

flokli Nov 24, 2019
Contributor

Please use pathlib here to calculate basenames and assemble paths:

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

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)

This comment has been minimized.

@flokli

flokli Nov 24, 2019
Contributor

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

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)

This comment has been minimized.

@flokli

flokli Nov 24, 2019
Contributor

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

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():

This comment has been minimized.

@flokli

flokli Nov 24, 2019
Contributor

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

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])

This comment has been minimized.

@flokli

flokli Nov 24, 2019
Contributor

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

@@ -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 {};

This comment has been minimized.

@flokli

flokli Nov 24, 2019
Contributor

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 7c6f434c force-pushed the 7c6f434c:add-systemd-analyze-test branch from a1821bc to 277eedf Nov 24, 2019
@7c6f434c
Copy link
Member Author

@7c6f434c 7c6f434c commented Nov 24, 2019

Depends on/includes #74077

@7c6f434c 7c6f434c force-pushed the 7c6f434c:add-systemd-analyze-test branch 2 times, most recently from db34588 to 16ab350 Nov 24, 2019
machine.wait_for_unit("multi-user.target")
with subtest("Prepare output dir"):
machine.succeed("mkdir systemd-analyze")

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

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).

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

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

This comment has been minimized.

@7c6f434c

7c6f434c Dec 3, 2019
Author Member

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

"systemd-analyze {} > {}/{} 2> {}/{}.err".format(
" ".join(args), tgt_dir, name, tgt_dir, name
)
Comment on lines +25 to +30

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

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",
)

This comment has been minimized.

@7c6f434c

7c6f434c Dec 3, 2019
Author Member

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.

This comment has been minimized.

@flokli

flokli Dec 4, 2019
Contributor

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

This comment has been minimized.

@7c6f434c

7c6f434c Dec 8, 2019
Author Member

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

run_systemd_analyze(["dot"], "dependencies.dot")
run_systemd_analyze(["plot"], "systemd-analyze.svg")
with subtest("Install the resulting data"):

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

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

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

This comment has been minimized.

@flokli

flokli Nov 25, 2019
Contributor

Some files seem to be missing here.

This comment has been minimized.

@7c6f434c

7c6f434c Dec 3, 2019
Author Member

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

This comment has been minimized.

@flokli

flokli Dec 4, 2019
Contributor

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

This comment has been minimized.

@7c6f434c

7c6f434c Dec 8, 2019
Author Member

Less asbtractions mean more Black pain. More comments added

@7c6f434c
Copy link
Member Author

@7c6f434c 7c6f434c commented Nov 25, 2019

@7c6f434c
Copy link
Member Author

@7c6f434c 7c6f434c commented Nov 25, 2019

@7c6f434c
Copy link
Member Author

@7c6f434c 7c6f434c commented Nov 25, 2019

@7c6f434c 7c6f434c force-pushed the 7c6f434c:add-systemd-analyze-test branch from 16ab350 to 743cf85 Nov 25, 2019
@flokli
Copy link
Contributor

@flokli flokli commented Dec 1, 2019

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

@7c6f434c 7c6f434c force-pushed the 7c6f434c:add-systemd-analyze-test branch from 743cf85 to c82a440 Dec 1, 2019
@7c6f434c
Copy link
Member Author

@7c6f434c 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 7c6f434c commented Dec 1, 2019

Maybe I should re-mention @flokli

@flokli
Copy link
Contributor

@flokli 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 7c6f434c commented Dec 3, 2019

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

@7c6f434c 7c6f434c force-pushed the 7c6f434c:add-systemd-analyze-test branch from c82a440 to e365454 Dec 8, 2019
@flokli
Copy link
Contributor

@flokli flokli commented Dec 11, 2019

@GrahamcOfBorg test systemd-analyze

@flokli flokli merged commit b5c5ed2 into NixOS:master Dec 11, 2019
17 checks passed
17 checks passed
systemd-analyze on aarch64-linux No attempt
Details
systemd-analyze on x86_64-darwin No attempt
Details
systemd-analyze on x86_64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
tests.systemd-analyze on aarch64-linux Success
Details
tests.systemd-analyze on x86_64-linux Success
Details
@flokli
Copy link
Contributor

@flokli flokli commented Dec 11, 2019

Thanks!

@7c6f434c
Copy link
Member Author

@7c6f434c 7c6f434c commented Dec 12, 2019

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

@flokli
Copy link
Contributor

@flokli flokli commented Dec 12, 2019

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

@7c6f434c
Copy link
Member Author

@7c6f434c 7c6f434c commented Dec 12, 2019

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.