-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Conversation
There was a problem hiding this 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.
@garbas said in 2018:
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. |
There was a problem hiding this 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!
@GrahamcOfBorg test systemd |
@flokli looks like you have added this test section as well?
|
Hm - should be part of another PR (which is already merged).
It probably sneaked in here as well - will have a look later today.
|
5d14f94
to
4f6f4c7
Compare
@mmahut I rebased on latest master. The diff in the tests should look clean now. There's still another failure in the systemd test:
But that's unrelated to this PR - and the cgroup accounting specific tests pass. |
@flokli thank you, have a conflict in the |
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
4f6f4c7
to
6b075dd
Compare
@mmahut done - fixed the indentation of the previous changelog entry too. |
@GrahamcOfBorg test systemd |
Motivation for this change
#29095 (comment)
fixes #29095.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @davidak @arianvp