corndog: use systemd-sysctl for sysctl options #333
Conversation
|
I don't speak Rust natively so if there's anything in here that's not idiomatic, feel free to let me know. |
| // We don't fail because sysctl keys can vary between kernel versions and depend on | ||
| // loaded modules. It wouldn't be possible to deploy settings to a mixed-kernel fleet | ||
| // if newer sysctl values failed on your older kernels, for example, and we believe | ||
| // it's too cumbersome to have to specify in settings which keys are allowed to fail. |
There was a problem hiding this comment.
The current implementation of this PR doesn't maintain the functionality mentioned in this comment, but we could pretty easily achieve this by prepending - to all the sysctls passed in by default (which would then remove the need for the backup-modify-restore shuffle, which might make things simpler overall), or by documenting the ability of users to do so. Open to feedback/direction here.
sources/api/corndog/src/main.rs
Outdated
| config.push_str(&format!("{}={}\n", key.as_ref(), value)); | ||
| } | ||
|
|
||
| fs::write(SYSCTL_CONFIG, config).context(error::WriteSysctlConfigSnafu)?; |
There was a problem hiding this comment.
I need to do some double-checking around:
- Does this actually append to the file as written, or does it clobber it?
- If we append and we write duplicate sysctls to the same config file, how does systemd-sysctl handle that? (Does the most recent sysctl win?)
I haven't yet been able to get a local build working (turns out my hard drive is smaller than I thought!) but I'll investigate these shortly.
There was a problem hiding this comment.
Answers:
- This clobbers the file (must fix)
- systemd-sysctl will use the last value in a file for the same sysctl. For example, a config file with the contents:
dev.raid.speed_limit_max = 199999
dev.raid.speed_limit_max = 199998
garbage
dev.raid.speed_limit_max = 199997
and running systemctl restart systemd-sysctl will set dev.raid.speed_limit_max to 199997 and report an error when attempting to parse the "garbage" line - the return code of the systemd unit will be 1 but all of the correctly specified sysctl k/v pairs will have been applied.
systemd-sysctl also will deduplicate multiple lines and only write when there are changes to be made, so in theory endlessly appending to a file should be a reasonable way to do this.
There was a problem hiding this comment.
Output from running systemd-sysctl with the above file in support of the above conclusions:
bash-5.1# SYSTEMD_LOG_LEVEL=debug /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/systemd-sysctl
Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/50-default.conf
Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/50-pid-max.conf
Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/80-release.conf
Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/90-kubelet.conf
Parsing /etc/sysctl.d/90-primary_interface.conf
Parsing /etc/sysctl.d/99-zzz-override_cilium.conf
Overwriting earlier assignment of net/ipv4/conf/all/rp_filter at '/etc/sysctl.d/99-zzz-override_cilium.conf:7'.
Parsing /etc/sysctl.d/99-zzz-zzz-testing.conf
Overwriting earlier assignment of dev/raid/speed_limit_max at '/etc/sysctl.d/99-zzz-zzz-testing.conf:2'.
/etc/sysctl.d/99-zzz-zzz-testing.conf:3: Line is not an assignment, ignoring: garbage
Overwriting earlier assignment of dev/raid/speed_limit_max at '/etc/sysctl.d/99-zzz-zzz-testing.conf:4'.
...
Setting '/proc/sys/dev/raid/speed_limit_max' to '199997'
No change in value '199997', suppressing write
|
Sorry for the delay, @aetimmes. And thanks so much for opening the PR for this. I will take a look at this PR and get back to you soon. I will also help verify the behavior. |
Issue number:
Closes bottlerocket-os/bottlerocket#4314
Description of changes:
Change corndog's sysctl function to utilize systemd-sysctl instead of writing values ephemerally to tmpfs, so that other system functions (eg Ciliums sysctl-fixup) don't completely clobber the sysctls set by corndog.
Testing done:
Unit tests added inline
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.