-
Notifications
You must be signed in to change notification settings - Fork 519
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
models: add new setting to auto-load kernel modules #3460
models: add new setting to auto-load kernel modules #3460
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.
Awesome, this looks like a big usability improvement for anyone needing to load additional modules.
With the new setting, you will need to add a migration to add/remove that setting on upgrade/downgrade. There is an example of doing something similar here: ac1e6e9
Other than the missing migration, this looks great!
I will have to have a look at the migrations part. I have not done that before. |
5a3c2e8
to
c0f0cc2
Compare
⬆️ tended the renaming to I will add the migrations part next week as this seems to be a little bit more specialized with a field in a variably named table. Will have to do some reading. |
⬆️ Added the migrations and bumped the Release version before, now that 1.15.0 is released and these migrations will be relevant when moving to the next version. I ended up needing 4 separate migrations. I was contemplating if it would make more sense to have one big custom migration or not, but maybe those with more experience writing migrations can chime in here. I was also wondering if it would be possible to do multiple I did test the migrations, the following is the gist of what happened: Upgrade:
Downgrade (setup one module for autoload previously to see that these keys get cleaned up correctly as well):
|
7db3829
to
df037f7
Compare
⬆️ force push fixed some formatting issues. |
08222a9
to
f8c308a
Compare
Last force pushed fixed additional clippy warnings. |
Do we have other examples? I noted in #3391 that the kernel seems to auto-load the IPVS modules. |
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.
Quite the crash course in migrations, but they look OK to me.
sources/api/migration/migrations/v1.16.0/kernel-modules-autoload-restart/src/main.rs
Outdated
Show resolved
Hide resolved
I missed the part that IPVS modules are now actually working to be autoloaded. I remember back when I was looking at the original ticket that the overhaul of the kube-proxy code indicated that starting in k8s 1.27 the modules could be loaded automatically. But I did not revisit this until now. Another instance of people loading modules manually would be Even if the autoloading of ipvs modules works with k8s-1.27 plus, this is probably a good change for people still using older versions and those use-cases we do not know about for comparably low cost. |
Loading specific kernel modules can be necessary to use specific features on your node. One example would be loading the correct module for your choice of scheduling algorithm for ipvs. Loading kernel modules is currently only possible during boot and requires the use and maintenance of an init-container image to load the desired kernel modules on boot. We can simplify that by adding the additional setting `autoload` for kernel modules and utilizing the already available systemd-modules-load service. Module auto-loading is in conflict with blocking modules from loading through the sibling setting `allowed`. Hence, do not auto-load a module if the module is not allowed at the same time. Blocking takes precedence, as it is the more prohibitive operation. Signed-off-by: Leonard Foerster <[email protected]>
This updates Release.toml so all new builds done off of the develop branch reflect that they are part of the 1.16.0 cycle. Signed-off-by: Leonard Foerster <[email protected]>
Clean up the kernel modules autoload setting on downgrade, as older releases do not support this functionality. Signed-off-by: Leonard Foerster <[email protected]>
f8c308a
to
18ed9ff
Compare
⬆️ force-push changed the |
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.
Nice work, especially with the migrations--learned a thing or two! 🧙
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.
🎉
Issue number: #3050, #2409
Description of changes:
Loading specific kernel modules can be necessary to use specific features on your node. One example would be loading the correct module for your choice of scheduling algorithm for ipvs.
Loading kernel modules on boot currently requires the use and maintenance of an init-container image to load the desired kernel modules on boot. We can simplify that by adding the additional setting
load-on-boot
for kernel modules and utilizing the already available systemd-modules-load service.Module auto-loading is in conflict with blocking modules from loading through the sibling setting
allowed
. Hence, do not auto-load a module on boot if the module is not allowed at the same time. Blocking takes precedence, as it is the more prohibitive operation.Testing done:
I have run tests with some variations of module specific options on a local vm in my dev-system and results were as expected:
autoloading and allowing the module to be loaded:
Results in autoloading the required module and dependency modules when re-triggering systemd-modules-load during boot:
conflict of blocking and auto-loading does not add the module to the config file and does not load the module:
Results in not autoloading the and not adding it to the config file:
Accidental overriding of a blocking setting is ruled out by toml not allowing to define the same table twice. The userdata configuration system will be derailed with a parser error for providing invalid user-data.
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.