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

Support the CoreOS GRUB /boot/coreos/first_boot flag file #13

Merged
merged 1 commit into from May 22, 2020

Conversation

pothos
Copy link
Member

@pothos pothos commented May 22, 2020

When a machine is migrated from CoreOS, GRUB code is not migrated and
specifies the coreos.first_boot=detected kernel parameter if the flag file
/boot/coreos/first_boot exists.
Run Ignition if the kernel parameter is present.

How to use

See flatcar/scripts#68

Testing done

See flatcar/scripts#68

When a machine is migrated from CoreOS, GRUB code is not migrated and
specifies the coreos.first_boot=detected kernel parameter if the flag file
/boot/coreos/first_boot exists.
Run Ignition if the kernel parameter is present.
pothos added a commit to flatcar/coreos-overlay that referenced this pull request May 22, 2020
@pothos pothos requested a review from a team May 22, 2020
@pothos pothos marked this pull request as ready for review May 22, 2020
@@ -39,7 +39,7 @@ if $(cmdline_bool flatcar.first_boot 0) || $(cmdline_bool coreos.first_boot 0);
add_requires ignition-disks.service
add_requires ignition-files.service
# Only try to mount the ESP if GRUB detected a first_boot file
if [[ $(cmdline_arg flatcar.first_boot) = "detected" ]]; then
if [[ $(cmdline_arg flatcar.first_boot) = "detected" ]] || [[ $(cmdline_arg coreos.first_boot) = "detected" ]]; then
Copy link
Contributor

@margamanterola margamanterola May 22, 2020

Choose a reason for hiding this comment

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

Could something else be setting this or is it only the grub script from the other change? If it's just the grub script, I think it would be clearer if it set only one value (flatcar.first_boot) instead of two values depending on the directory.

If there's something else that could potentially set this value, then I guess this change would be fine, but I'd like a comment explaining where the double naming comes from.

Copy link
Member Author

@pothos pothos May 22, 2020

Choose a reason for hiding this comment

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

The issue is that migrated machines will only set coreos.first_boot always because they have the CoreOS GRUB code.

Copy link
Contributor

@margamanterola margamanterola May 22, 2020

Choose a reason for hiding this comment

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

I wonder if we should update that GRUB code then. But anyway, in that case, please add a comment explaining that the coreos flag gets set on machines that were upgraded from coreos (and let's please NOT set it ourself).

Copy link
Member Author

@pothos pothos May 22, 2020

Choose a reason for hiding this comment

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

Ah, sorry for early merging. Yes, we could add a comment, but I didn't think about it because three lines above the two cases are also covered.

Copy link
Member Author

@pothos pothos May 22, 2020

Choose a reason for hiding this comment

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

Even if we updated GRUB there can still be iPXE scripts that set the coreos kernel parameters.

Copy link
Contributor

@margamanterola margamanterola May 22, 2020

Choose a reason for hiding this comment

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

Ok, I was mostly talking about where the split came from, which is not covered by the comment above (which is talking about the conditional, not about the fact that we take into account both variables.

I think it's important to document why we are keeping both variables and more importantly when we intend to stop supporting both (or what would need to happen for us to stop supporting both).

@pothos pothos merged commit 8043da3 into flatcar-master May 22, 2020
@pothos pothos deleted the kai/coreos-first-boot branch May 22, 2020
pothos added a commit to flatcar/coreos-overlay that referenced this pull request May 22, 2020
pothos added a commit to flatcar/coreos-overlay that referenced this pull request May 22, 2020
pothos added a commit to flatcar/coreos-overlay that referenced this pull request May 22, 2020
pothos added a commit to flatcar/coreos-overlay that referenced this pull request May 22, 2020
pothos added a commit to flatcar/coreos-overlay that referenced this pull request May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants