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

nixos/systemd: enable systemd cgroup accounting by default #66984

Merged
merged 4 commits into from Aug 27, 2019

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Aug 19, 2019

Motivation for this change

#29095 (comment)

fixes #29095.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 @davidak @arianvp

@flokli flokli requested a review from abbradar August 19, 2019 21:22
Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

Looks good! I'll test it in the coming week and merge.

@davidak
Copy link
Member

davidak commented Aug 20, 2019

@garbas said in 2018:

I think having it false by default is ok, we can have a different discussion about it if we want to enable this or not and when.

Source: #36772 (comment)

So here we are.

For me, it's OK to have it just enabled when needed (e.g. for netdata). I havn't used it outside that.

But since Ubuntu, Fedora and Arch Linux have it enabled by default, we could enable it too by default for everyone. Or just desktop users.

Source: https://docs.netdata.cloud/collectors/cgroups.plugin/#monitoring-systemd-services

And since systemd now enables some options by default, we should have at least enabled them. And why not all to provide the full feature to our users.

Source: #29095 (comment)

Like said earlier, i had no performance issues, but i didn't measure the difference.

@davidak davidak mentioned this pull request Aug 20, 2019
10 tasks
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

I tested it using the provided tests and can verify that the option and tests work!

@mmahut
Copy link
Member

mmahut commented Aug 22, 2019

@GrahamcOfBorg test systemd

@mmahut
Copy link
Member

mmahut commented Aug 22, 2019

@flokli looks like you have added this test section as well?

machine# sysctl: cannot stat /proc/sys/net/core/default_qdisc: No such file or directory
machine: exit status 1```

@flokli
Copy link
Contributor Author

flokli commented Aug 22, 2019 via email

@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2019

@mmahut I rebased on latest master. The diff in the tests should look clean now.

There's still another failure in the systemd test:

error: command `dumpe2fs /dev/vdb | grep -q "^Last mount time: *n/a"' unexpectedly succeeded at /nix/store/39fb7m572abixj1qi7ddzx5lp3k64lw2-nixos-test-driver/lib/perl5/site_perl/Mach
ine.pm line 376, <__ANONIO__> line 194.                                                                                                                                               
(12.12 seconds)  

But that's unrelated to this PR - and the cgroup accounting specific tests pass.

@mmahut
Copy link
Member

mmahut commented Aug 25, 2019

@flokli thank you, have a conflict in the release-notes, do you mind to rebase, please?

systemd defaults DefaultMemoryAccounting and DefaultTasksAccounting to
yes, so no need to enable explicitly
If this is the default for OpenShift already, we probably can enable it
as well.

see openshift/machine-config-operator#581
@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2019

@mmahut done - fixed the indentation of the previous changelog entry too.

@mmahut
Copy link
Member

mmahut commented Aug 26, 2019

@GrahamcOfBorg test systemd

@flokli
Copy link
Contributor Author

flokli commented Aug 27, 2019

@mmahut the systemd test is still broken in master (as written in the comment above), but the newly added subtest does work.

If we're fine with this PR, we should merge it.

I opened #67555 to track the systemd vm test breakage on master.

@flokli flokli merged commit 9a02d9c into NixOS:master Aug 27, 2019
@flokli flokli deleted the systemd-cgroup-accounting branch August 27, 2019 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable cgroup accounting
5 participants