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

install: make platform-specific bootloader changes when -p specified #605

Merged
merged 4 commits into from
Jun 7, 2022
Merged

install: make platform-specific bootloader changes when -p specified #605

merged 4 commits into from
Jun 7, 2022

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Aug 24, 2021

If -p is specified and /boot/coreos/platforms.json exists, look up the specified platform and the metal platform in that file. Remove any metal-specific kernel arguments or GRUB commands specified there, and inject any kargs/commands for the new platform. This allows us to apply a platform-specific console configuration at install time.

Set the platform before performing kargs modification to allow the latter to change a platform-specific default console.

When installing via coreos.inst kargs, persist any console= kargs to the destination system, as suggested in coreos/fedora-coreos-tracker#567 (comment).

@bgilbert
Copy link
Contributor Author

Notably, while this PR allows the user to override console kargs, it doesn't provide a way to override the GRUB console commands.

@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 2, 2021

Tested the contents of grub.cfg and kargs after install -p with:

  • No platforms.json
  • Target platform omitted from platforms.json
  • Target platform with kargs + GRUB commands in platforms.json
  • Target platform with kargs only in platforms.json
  • Target platform with GRUB commands only in platforms.yaml

Ready for review.

@bgilbert bgilbert marked this pull request as ready for review September 2, 2021 03:50
@bgilbert bgilbert marked this pull request as draft September 3, 2021 17:36
@bgilbert bgilbert marked this pull request as ready for review May 28, 2022 07:35
@bgilbert
Copy link
Contributor Author

Rebased and reworked. Tested the contents of grub.cfg and kargs after install with:

  • no -p,
  • -p aws (platform-specific kargs and GRUB commands),
  • -p aliyun (no platform-specific kargs or commands),
  • -p metal (redundant),

with each of:

Also tested:

  • -p aws --delete-karg=console=ttyS0,115200n8 with platforms.json
  • Forwarding console kargs when installing with coreos.inst.install_dev

Ready for review.

@cgwalters
Copy link
Member

I hadn't really looked at this before. I totally understand the goals, but this is also taking us even farther away from the "fancy dd" model. The feature requires coreos-assembler as is and is another thing that won't work for RHEL for Edge. (Of course, it could be made to, and I do hope eventually they support Ignition at least optionally which would greatly simplify the story, but just noting)

How much do we need this versus just documenting how to use the existing kernel argument switches?

src/install.rs Outdated Show resolved Hide resolved
src/install.rs Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 1, 2022

This code is only invoked for -p. I agree that the config file hackery isn't ideal, but -p is already doing config file hackery so I feel like that bridge has already been crossed. If we're going to have platform-specific kargs, I don't think -p will be user-friendly without this; the user would need to go look up what kargs to add (and potentially remove) to match the preconfigured image for that platform.

RHEL for Edge doesn't use Ignition, so -p already isn't meaningful there, and in fact will fail because it won't find a preconfigured ignition.platform.id karg.

@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 1, 2022

Updated!

src/install.rs Outdated Show resolved Hide resolved
src/install.rs Show resolved Hide resolved
src/install.rs Outdated Show resolved Hide resolved
src/install.rs Outdated Show resolved Hide resolved
src/install.rs Outdated Show resolved Hide resolved
scripts/coreos-installer-service Outdated Show resolved Hide resolved
scripts/coreos-installer-service Outdated Show resolved Hide resolved
Copy link
Contributor Author

@bgilbert bgilbert 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 the review! The last commit needs some additional thought, so I've moved it to a separate PR #877.

scripts/coreos-installer-service Outdated Show resolved Hide resolved
scripts/coreos-installer-service Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 7, 2022

Updated!

Avoid a growing list of loose function imports.
Use the existing tool for consistency.  It's slightly smarter than simple
string replacement.
If -p is specified and /boot/coreos/platforms.json exists, look up the
specified platform and the "metal" platform in that file.  Remove any
metal-specific kernel arguments or GRUB commands specified there, and
inject any kargs/commands for the new platform.  This allows us to apply
a platform-specific console configuration at install time.
Allow kargs modification to change a platform-specific default console.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM!

@bgilbert bgilbert merged commit 6e5efa0 into coreos:main Jun 7, 2022
@bgilbert bgilbert deleted the platforms branch June 7, 2022 17:57
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.

None yet

3 participants