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

Add install --console and ... customize --dest-console to configure GRUB and kernel console #977

Merged
merged 8 commits into from
Sep 12, 2022

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Sep 8, 2022

Once CoreOS stops enabling the serial console by default, users will need a way to enable it themselves. This really only affects bare metal, since other platforms have a consistent console configuration, so coreos-installer is a good place to handle it. Users can already pass console= kargs, but we need a way to specify GRUB commands, ideally without having to actually use GRUB syntax.

Add a --console option that accepts kernel console= syntax, since that's already familiar. Parse the option and convert it to both a console karg and GRUB commands, using the existing mechanism for GRUB command substitution. If --platform is also specified, have --console override that platform's default console settings. If --console is specified more than once, aggregate the console settings, following the multiple-console semantics for GRUB (use both consoles) and the kernel (use all consoles, with the last console primary).

There are serial console features only supported by GRUB (stop bits) or by the kernel (RTS/CTS flow control). Support the common subset and reject the rest. Also, round-trip console kargs to a less-compact but semantically equivalent format, since explicit is good and it saves some code complexity.

Document this as "bootloader console" rather than "GRUB console" to allow for future bootloaders other than GRUB.

Add the corresponding --dest-console option to the customize commands. Use the installer-config-directives features.json object introduced by coreos/coreos-assembler#3083 to check whether the image's coreos-installer is new enough to support --console. In the future we can potentially support a --live-console or combined --console customize option that also affects the live kernel and bootloader (#979), but defer that for now.

It was originally factored out for unit testing, but these days we just
use KargsEditor which has its own tests.  Simplify by using KargsEditor
directly, with a separate initial probe to ensure we can find the
ignition.platform.id.
This has us rewriting the BLS config twice if `--platform` is specified,
but that option is non-performance-critical and also an edge case, so
let's aim for legibility.

Preparatory for next commit.
@bgilbert bgilbert changed the title WIP: Add install --console and ... customize --dest-console to configure GRUB and kernel console Add install --console and ... customize --dest-console to configure GRUB and kernel console Sep 9, 2022
@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 9, 2022

Ready for review!

@bgilbert bgilbert marked this pull request as ready for review September 9, 2022 07:54
@bgilbert bgilbert requested a review from lucab September 9, 2022 07:54
@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 9, 2022

CI is failing because registry.ci.openshift.org/coreos/coreos-assembler:latest doesn't have coreos/coreos-assembler#3083 yet.

@dustymabe
Copy link
Member

Overall strategy seems sane to me without reviewing the code directly.

One question I have.. Should we consider giving the user a warning if they try to set the kernel argument for console directly so that existing users get pointed towards this new (better) feature?

Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

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

Code LGTM.
I am both scared by the regex monster and by your newly acquired knowledge on serial oddities across Linux and GRUB 😄
On a more serious note, I really only have a general question about UX.

man/coreos-installer-install.8 Show resolved Hide resolved
@dustymabe
Copy link
Member

One question I have.. Should we consider giving the user a warning if they try to set the kernel argument for console directly so that existing users get pointed towards this new (better) feature?

And by "set the kernel argument for console directly" I mean either in the provided Ignition config (won't work for remote Ignition references, but that's OK), or through coreos-installer features like --append-karg.

@bgilbert
Copy link
Contributor Author

Updated!

Should we consider giving the user a warning if they try to set the kernel argument for console directly so that existing users get pointed towards this new (better) feature?

And by "set the kernel argument for console directly" I mean either in the provided Ignition config (won't work for remote Ignition references, but that's OK), or through coreos-installer features like --append-karg.

I've added a warning for install --append-karg and iso/pxe customize --dest-karg-append. Checking Ignition kargs doesn't seem worth it, though. We couldn't check merged/replaced configs as you say, couldn't detect kernel arguments specified via OCP MachineConfig, and the user may not have the flexibility to switch from Ignition kargs to --console (for example, because the Ignition config was generated by a tool).

Once CoreOS stops enabling the serial console by default, users will
need a way to enable it themselves.  This really only affects bare
metal, since other platforms have a consistent console configuration,
so coreos-installer is a good place to handle it.  Users can already
pass console= kargs, but we need a way to specify GRUB commands,
ideally without having to actually use GRUB syntax.

Add a --console option that accepts kernel console= syntax, since
that's already familiar.  Parse the option and convert it to both a
console karg and GRUB commands, using the existing mechanism for GRUB
command substitution.  If --platform is also specified, have --console
override that platform's default console settings.  If --console is
specified more than once, aggregate the console settings, following the
multiple-console semantics for GRUB (use both consoles) and the kernel
(use all consoles, with the last console primary).

There are serial console features only supported by GRUB (stop bits) or
by the kernel (RTS/CTS flow control).  Support the common subset and
reject the rest.  Also, round-trip console kargs to a less-compact but
semantically equivalent format, since explicit is good and it saves
some code complexity.

Document this as "bootloader console" rather than "GRUB console" to
allow for future bootloaders other than GRUB.
If a flag was absent from features.json, we were failing to parse rather
than defaulting the flag to false.  This didn't matter in practice because
both flags were introduced into production at the same time as the file
in FCOS 35.20211215.2.0.
Use the installer-config-directives features.json object introduced by
coreos/coreos-assembler#3083 to check whether
the image's coreos-installer is new enough to support --console.
Rather than manually adding new fixtures to every test individually, have
each test declare its minimum supported fixture, and then automatically
run the test on any newer fixtures.

Fixes failure to invoke some tests on 2022-02 fixture.
In particular, it includes the "console" directive.
If one or more console= kargs are specified to the install or iso/pxe
customize subcommands, and Console can successfully parse all of those
kargs, warn the user that they might benefit from specifying them using
the console mechanism instead.

For legibility, wrap the message at 80 characters, but avoid wrapping in
the middle of the example argument lists.  Use the textwrap crate to do
this.  textwrap is already a clap dependency without default features, and
we also avoid non-default features here, so this doesn't increase the
binary weight.
@bgilbert bgilbert merged commit fb97694 into coreos:main Sep 12, 2022
@bgilbert bgilbert deleted the console branch September 12, 2022 18:13
@@ -0,0 +1,347 @@
// Copyright 2022 Red Hat, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

This module is both terrifying and great, which is about what we expected I think. 😆

Really nice work!

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