Skip to content

recovery-chooser-trigger.service: Survive the switch root#64

Merged
valentindavid merged 1 commit intocanonical:mainfrom
valentindavid:valentindavid/recovery-chooser-trigger-stayin-alive
Feb 4, 2022
Merged

recovery-chooser-trigger.service: Survive the switch root#64
valentindavid merged 1 commit intocanonical:mainfrom
valentindavid:valentindavid/recovery-chooser-trigger-stayin-alive

Conversation

@valentindavid
Copy link
Copy Markdown
Member

snap-bootstrap recovery-chooser-trigger is only expected to use
/run so it can safely survive the switch root.

When using stateful re-execution of systemd, this will keep the same
service started from initramfs and continue into the main boot.

Because we need to pull it into initrd-switch-root.target, we also
need to remove default dependencies so that we do not indirectly pull
sysinit.target or local-fs.target into
initrd-switch-root.target.

Note, we should probably add a signal handler in snap-bootstrap recovery-chooser-trigger to be notified when initrd-root-fs.target
is reached to chdir into /sysroot so that we do not drag the old
initramfs in memory until it is stopped. This is because systemd will
lazily umount the old root.

Copy link
Copy Markdown
Contributor

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this change, can you clarify what the order of release should be? if this change was included in the initramfs today without the signal handling you mention are there any side-effects? do we need to do that signal handling before this change can be released/included in kernel snaps?

Copy link
Copy Markdown
Contributor

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as clarified on mattermost since this does not need the signal handling added to the recovery chooser trigger (it would merely be an improvement), this looks good to me

@valentindavid
Copy link
Copy Markdown
Member Author

thanks for this change, can you clarify what the order of release should be? if this change was included in the initramfs today without the signal handling you mention are there any side-effects? do we need to do that signal handling before this change can be released/included in kernel snaps?

The side effect is only memory not released right away. We do not need change in snap-bootstrap. It is just nice.

Also note that this PR will only do something after #58. So maybe we can wait for #58 to be merged. Potentially without #58, we could have 2 runs of recovery-chooser-trigger in the same time. Though it should not be a problem, but I have not tested that.

@anonymouse64
Copy link
Copy Markdown
Contributor

Also note that this PR will only do something after #58. So maybe we can wait for #58 to be merged. Potentially without #58, we could have 2 runs of recovery-chooser-trigger in the same time. Though it should not be a problem, but I have not tested that.

In general I think it's actually better to do the inverse - land as much as possible changes that do nothing, then turn them on with a simple PR at the end. So I think we should land this before the stateful re-exec in #58 (and also I think we should do the stateful re-exec change as part of #59 too unless there's a compelling reason not to)

@valentindavid
Copy link
Copy Markdown
Member Author

In general I think it's actually better to do the inverse - land as much as possible changes that do nothing, then turn them on with a simple PR at the end. So I think we should land this before the stateful re-exec in #58 (and also I think we should do the stateful re-exec change as part of #59 too unless there's a compelling reason not to)

OK, I have tested manually the change and it does not seem to break. Do I need to trigger snapd spread tests for just that change?

Copy link
Copy Markdown
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alfonsosanchezbeato
Copy link
Copy Markdown
Member

FTR, this is how plymouth changes its working folder after the switch root:
https://gitlab.freedesktop.org/plymouth/plymouth/-/blob/main/src/main.c#L649
(maybe not needed for recovery-chooser-trigger, but leaving here as reference, just in case)

@valentindavid valentindavid force-pushed the valentindavid/recovery-chooser-trigger-stayin-alive branch 3 times, most recently from b6fae14 to 9cd5471 Compare February 3, 2022 16:44
`snap-bootstrap recovery-chooser-trigger` is only expected to use
`/run` so it can safely survive the switch root.

When using stateful re-execution of systemd, this will keep the same
service started from initramfs and continue into the main boot.

Because we need to pull it into `initrd-switch-root.target`, we also
need to remove default dependencies so that we do not indirectly pull
`sysinit.target` or `local-fs.target` into
`initrd-switch-root.target`.

Note, we should probably add a signal handler in `snap-bootstrap
recovery-chooser-trigger` to be notified when `initrd-root-fs.target`
is reached to `chdir` into `/sysroot` so that we do not drag the old
initramfs in memory until it is stopped. This is because systemd will
lazily umount the old root.
@valentindavid valentindavid force-pushed the valentindavid/recovery-chooser-trigger-stayin-alive branch from 9cd5471 to 2fb3b70 Compare February 4, 2022 12:39
@valentindavid valentindavid merged commit 45d7e6e into canonical:main Feb 4, 2022
valentindavid added a commit to valentindavid/snapd that referenced this pull request Jul 31, 2024
core-initrd for UC20 is missing the following fixes:
 * canonical/core-initrd#64
 * canonical/core-initrd#70
 * canonical/core-initrd#97

Because of that, it still kills recovery-chooser-trigger from initrd,
and expects it to re-run from snapd snap. Until those fixes
are backported, we need to keep on building snap-bootstrap.

This is not needed by UC22+
ernestl pushed a commit to canonical/snapd that referenced this pull request Aug 2, 2024
core-initrd for UC20 is missing the following fixes:
 * canonical/core-initrd#64
 * canonical/core-initrd#70
 * canonical/core-initrd#97

Because of that, it still kills recovery-chooser-trigger from initrd,
and expects it to re-run from snapd snap. Until those fixes
are backported, we need to keep on building snap-bootstrap.

This is not needed by UC22+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants