Automated reconciliation of boot settings

[markusboehme]

Issue number:

Closes #2155

Description of changes:

This PR implements what has been proposed in #2155, the addition of a new settings.boot.reboot-to-reconcile setting to automatically reboot Bottlerocket if either the kernel or systemd command line were changed during boot. At a high level, prairiedog now compares any newly written bootconfig initrd against the one already present and flags the need for a reboot. Before entering the configured systemd target, a new service unit then triggers the reboot if needed.

To reviewers, I recommend going commit by commit. The commits are fairly detailed and hopefully tell a story if followed in order.

Testing done:

• [x] No boot settings specified: No automatic reboot; host can be rebooted
• [x] Boot settings specified with reboot-to-reconcile unset: No automatic reboot; host can be rebooted and comes up with specified settings
• [x] Boot settings specified with reboot-to-reconcile set to false: No automatic reboot; host can be rebooted and comes up with specified settings
• [x] Boot settings specified with reboot-to-reconcile set to true: Automatic reboot; host comes up with specified settings
• [x] Boot settings specified with reboot-to-reconcile set to true and bootstrap containers: Automatic reboot after bootstrap containers have exited; host comes up with specified settings
• [x] Tested forward and backward migration from 1.9.2 to a faux 1.10.0 with this PR on builds of the aws-k8s-1.23 variant, both with and without reboot-to-reconcile set

Notes:

• ~While this implements a new setting, I don't think a migration is needed as the setting is persisted and restored by prairiedog in the bootconfig initrd. Older versions of prairiedog will just ignore the setting.~ Update: A migration is needed as pointed out by @etungsten: While prairiedog may ignore the unknown setting, it will bite when trying to deserialize the data store. The current version comes with a migration.
• If you're taking this on a test ride yourself, please see #2359: If you configure a boot setting that doesn't require a value, please specify the empty string for now.

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.

[etungsten]

I don't think a migration is needed as the setting is persisted and restored by prairiedog in the bootconfig initrd. Older versions of prairiedog will just ignore the setting.

We would still need a migration as we're introducing a new setting. If this new setting is not properly removed from the settings data-store on downgrade then the host will fail to boot because the settings model will not anticipate this new setting when apiserver tries to de-serialize the existing settings data-store.

[markusboehme]

I don't think a migration is needed as the setting is persisted and restored by prairiedog in the bootconfig initrd. Older versions of prairiedog will just ignore the setting.

We would still need a migration as we're introducing a new setting. If this new setting is not properly removed from the settings data-store on downgrade then the host will fail to boot because the settings model will not anticipate this new setting when apiserver tries to de-serialize the existing settings data-store.

I see, thanks! I was under the impression all serialization/deserialization for BootSettings would be handled by prairiedog, but clearly the settings are persisted in the data store as well.

While anything under and including settings.boot would be scrubbed by the migration that introduced that setting as a prefix in v1.8.0, this doesn't take care of any rollbacks to v1.8.0, v1.9.0, etc. I will add a new AddSettingsMigration for the settings.boot.reboot-to-reconcile key.

[markusboehme]

I don't quite understand the need to persist the reboot-to-reconcile flag within the actual bootconfig data.

Originally, this is mainly a result of misunderstanding the serialization of BootSettings, i.e. assuming they would only be persisted by prairiedog. Erikson clarified this in his earlier comment. There should be nothing in the way of dropping that setting from the bootconfig data again. When invoked by sundog, prairiedog generate-boot-settings will have to query the API for the current data and merge the kernel and init settings read from the bootconfig data into it. I will try this next--I expect it will get rid of quite a bit of extra code.

Let me back up a step. As a rule I tend to find marker files distasteful, and one of my stronger architectural preferences is to favor API queries over ad-hoc files, since those files tend to create their own mini-API surfaces that aren't consistent. [...]

It's really that nested sequence of state that's not clear to me - why we have API settings and bootconfig and the marker file, and if we can do away with one or both of the latter bits of state.

Thanks for taking the time to share your thoughts on the overall design! I think a part of the confusion stems from trying to see deeper reasons in having reboot-to-reconcile in the bootconfig datat when I thought of it as just a necessity. :blush: Not doing that anymore will likely make things clearer.

I had considered, but not implemented, the idea of a new prairiedog command. Decoupling the decision to reboot and actually rebooting via a marker file was motivated by the idea a bootstrap container might find other reasons to request a reboot before the host enters its normal operational state. There is no such request for now, so perhaps it makes sense to drop this and go with the specific solution that keeps all logic inside prairiedog.

[bcressey]

Decoupling the decision to reboot and actually rebooting via a marker file was motivated by the idea a bootstrap container might find other reasons to request a reboot before the host enters its normal operational state. There is no such request for now, so perhaps it makes sense to drop this and go with the specific solution that keeps all logic inside prairiedog.

I was thinking that an alternative using helper commands might be a oneshot unit with multiple potential restart commands, e.g.:

ExecStart=/usr/bin/prairiedog reboot-if-required
ExecStart=/usr/bin/netdog reboot-if-required
ExecStart=/usr/bin/updog reboot-if-required

Where each program gets to encapsulate its own state checks and decision-making.

But your use case - allowing bootstrap containers to "schedule" a reboot after all other bootstrap containers have run, by writing a marker file in a well-known place - makes sense too, and wouldn't be possible with the sequenced helper commands.

I'm happy to keep the /run/reboot-required approach.

[markusboehme]

@bcressey I see what you mean now. This definitely looks and feels more as if made from one piece, and doesn't preclude bootstrap containers from signalling a need to reboot either: There likewise could be a /usr/bin/bootstrap-containers reboot-if-required, checking e.g. a marker file that is local to a particular bootstrap container. This even would provide more flexibility (e.g. accountability, or honoring the request only for certain containers). I'm thus moving away from the global marker file in the next iteration of the PR.

[bcressey]

There likewise could be a /usr/bin/bootstrap-containers reboot-if-required, checking e.g. a marker file that is local to a particular bootstrap container.

Nice! I really like this idea.

[markusboehme]

The force push changes the approach as discussed above:

• The reboot now happens after reaching the configured.target, sidestepping the need to explicitly consider bootstrap containers and whether they're essential or not.
• The initrd boot config now only contains the kernel and init command line arguments. The new reboot-to-reconcile setting is only persisted in the data store.
• prairiedog grew a new reboot-if-required command to compare the initrd boot config against the boot config the host booted with, issuing a reboot if that is required and settings.boot.reboot-to-reconcile is set to true. Other first-party components can likewise introduce reboot-if-required commands of their own and hook into the existing reboot-if-required.service.
• The reboot-if-required setting now comes with the migration needed for a rollback and has been tested.

There's merge conflicts to sort out in Cargo.toml and Release.toml needs merging as well. I refrained from rebasing for now to allow for easy diffing against the older revision. Will rebase to unblock the test process once folks have signaled they're in agreement with the current approach.

[markusboehme]

The latest force push contains these changes:

• reboot-if-required.service now is Type=oneshot as it should have been, waiting for prairiedog to issue the reboot
• Extracted a function to actually compare the boot settings for reboot-worthy changes, now coming with dedicated unit tests

I'll rebase onto develop and resolve merge conflicts once there's no need to diff against the previous revisions anymore. This will also unblock the automated testing for this PR.

[markusboehme]

Thanks for the reviews! I force-pushed now to resolve the conflicts in Cargo.toml and Release.toml, unblocking the automated tests.