-
Notifications
You must be signed in to change notification settings - Fork 155
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
Kernel crash dumps autoinstall #2074
Kernel crash dumps autoinstall #2074
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.
Schema possibly aside this all looks good. Probably doesn't make sense to land it until curtin changes are done though?
d259f65
to
332282a
Compare
Either way really. I suppose if we wait it'd save us a slightly annoying revert if it ends up not landing in curtin. |
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.
A few quibbles, but looks good overall.
332282a
to
0c64aba
Compare
.. code-block:: yaml | ||
|
||
autoinstall: | ||
# Allow kernel crash dumps to be enabled dynamically. |
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.
only 24.10 sees this default behavior, pre-24.10 is default off, let's spell that out.
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.
Good point. Done!
452315e
to
6e572d6
Compare
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.
As we're documenting defaults, we should mention the architecture bit as well for default enabled / disabled state, otherwise LGTM.
Introduce the KernelCrashDumpsController and KernelCrashDumpsModel, a non-interactive controller-model pair for setting the kernel-crash-dumps curthooks config. Practically, we introduce a new autoinstall section with the same format as the relevant curthooks config to selectively enable or disable kernel crash dumps on the target system. The default configuration is `{ "enabled": null }` which will let curtin perform dynamic enablement.
The needed wiring to make subiquity use the new kernel crash dumps related controller and model.
6e572d6
to
646c0d9
Compare
Allow check for `None` type in check-yaml-fields.py script. It wasn't clear from the original commit - 2ddde49 - if we purposefully didn't want to compare `null` in the script, or it was just a typo. This will now let us assert a field should be `null` instead of some other value, and fail properly when it is not.
- Add an example kernel-crash-dumps section to the most-options.yaml example file and check the curthook is updated correctly. - Add a check in the autoinstall-simple case that asserts the default value for the kernel-crash-dumps curthook config.
646c0d9
to
be8d1b1
Compare
Done! If I'm missing anything else let me know please. Here is the MP for the relevant curtin changes: https://code.launchpad.net/~cpete/curtin/+git/curtin/+merge/473169 |
Introduce the
KernelCrashDumpsController
andKernelCrashDumpsModel
, a non-interactive controller-model pair for setting the kernel-crash-dumps curthooks config. Practically, we introduce a new autoinstall section with the same format as the relevant curthooks config to selectively enable or disable kernel crash dumps on the target system. The defaultconfiguration is
{ "enabled": null }
which will let curtin perform dynamic enablement.Currently this is a no-op since the
kernel-crash-dumps
curthooks section hasn't been implemented in curtin yet.