Skip to content
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

coreos-boot-edit: add triggering condition on coreos-copy-firstboot-network.stamp #745

Closed
wants to merge 1 commit into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Nov 23, 2020

That way we completely avoid mounting /boot rw if we don't need to.
Also clarify comment about the boot dependency. Additional roles for
this service ideally would follow the same pattern.

Minor follow-up to #743.

…etwork.stamp

That way we completely avoid mounting `/boot` rw if we don't need to.
Also clarify comment about the boot dependency. Additional roles for
this service ideally would follow the same pattern.

Minor follow-up to coreos#743.
@jlebon
Copy link
Member Author

jlebon commented Nov 23, 2020

Not tested yet.

Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kelvinfan001 kelvinfan001 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jlebon
Copy link
Member Author

jlebon commented Nov 24, 2020

Once we merge coreos-inject-rootmap.service into this, then I think the motivation for using the triggering model proposed here drops a lot because right now we always inject at least root=... rw on first boot, so the service will just always kick in. (It's conditional on root= not being present, but that karg being present on firstboot is a bit of a snowflake -- mostly for setting up multipath at install-time).

So closing this one. (And accordingly, we can indeed drop the stamp file as discussed in #743.)

@jlebon jlebon closed this Nov 24, 2020
@kelvinfan001
Copy link
Member

kelvinfan001 commented Nov 24, 2020

Just to be sure, would this mean we get rid of ConditionKernelCommandLine=!root too (since it's rare that it's not satisfied)? Or keep the stamp file and have

ConditionKernelCommandLine=|!root
ConditionPathExists=|/run/coreos-copy-firstboot-network.stamp

@jlebon
Copy link
Member Author

jlebon commented Nov 24, 2020

I was arguing for dropping the triggering conditions entirely and just having the unit always run to be consistent, even if in some rare situations it might not have been necessary.

We'd still make the call to rdcore rootmap itself in the script conditional on root= not being present in /proc/cmdline though.

@kelvinfan001
Copy link
Member

We'd still make the call to rdcore rootmap itself in the script conditional on root= not being present in /proc/cmdline though.

Ahh, got it. 👍

@jlebon jlebon deleted the pr/boot-edit-cleanups branch April 24, 2023 01:19
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.

3 participants