-
Notifications
You must be signed in to change notification settings - Fork 524
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
Allow variants to specify extra kernel parameters #1491
Allow variants to specify extra kernel parameters #1491
Conversation
For example, this is useful when variants designed to run on different platforms need different serial console parameters.
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.
❤️
Code-wise this looks good and will be super useful in the future! Thanks for adding the options to the docs also.
I'll need to experiment a little to ensure that this will fix #1489 . I believe that if we pass the correct console and forward systemd/journald's output to the console that we should be able to see all the output.
To actually fix #1489 we'll need to move all the console params into the variant definitions - do you want to make that change here or in a different PR? |
This PR is intended to add the capability, not make any functional changes. Why all the params, though? I think it makes sense to leave sensible defaults, tty0 and ttyS0, so that this isn't required for most variants, and variants using other console devices can add them. |
I quibble a bit with the "sensible defaults" - correct console parameters are inherently platform specific even in the best case, and the ones we have now are strongly influenced by EC2. They aren't right for VMWare, Raspberry Pi, and perhaps most others. There are also architecture limits on the total size of the command line, so ideally we wouldn't be including redundant parameters in normal cases. |
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!
^ and yes - agreeing with @bcressey here on the console parameters. We can do that in a separate PR. |
Issue number:
Fixes #1490
Description of changes:
Note that the parameters are intentionally added to the start of the command line because this isn't intended as an override; our parameters genuinely have to be there! We should design another feature if there are use cases for overhauling the parameters rather than just adding new ones.
Testing done:
Built aws-dev after adding parameters:
Saw the new parameters in /proc/cmdline:
Host was still healthy, ran containers well,
systemd status
running
, API gets/sets OK, etc.Build aws-k8s-1.17 without adding any parameters and saw that there was no change to /proc/cmdline:
Host was similarly healthy, and ran pods OK.
Rendered the new docs OK.
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.