-
Notifications
You must be signed in to change notification settings - Fork 93
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 high-level subcommands for customizing live ISO images and PXE initrds #706
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments, but this looks great overall!
#[derive(Debug, StructOpt)] | ||
pub struct CommonCustomizeConfig { | ||
/// Script to run before installation | ||
#[structopt(long, value_name = "path")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I thought we landed on capitalized strings for value names, but now looking at the current cmdline.rs
, it looks like a mix. We should probably decide on something and be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xref #305 (comment) 😁
Consistency seems good, yes. I still find SCREAMING CASE less readable, and it seems unnecessary since clap wraps value names in <>
anyway. But if you feel strongly about it, I can convert these over.
Naming is hard...
|
I'm not thrilled with it either; better ideas welcome.
It applies to the destination system just as the other |
Rebased, refactored for recent developments, added long help. Not yet tested. |
We're going to have some large subcommand structs, but we allocate one of them per program, so it's fine.
Add a convenience command that combines `iso ignition remove`, `iso network remove`, and `iso kargs reset`.
Serialize and embed an empty Ignition config in the ISO.
In particular, To validate that the input is a CoreOS live initrd, we check for I originally implemented in-place modification of the input, as |
Also of note, some options want to inform the install process but not necessarily initiate it. For example, |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, LGTM! The split commits are helpful.
Mostly just some tiny nits. But I'd be fine getting this in as is too and fixing things up as follow-ups.
@@ -484,6 +484,13 @@ pub struct CommonCustomizeConfig { | |||
/// shell. | |||
#[structopt(long, value_name = "path")] | |||
pub post_install: Vec<String>, | |||
/// Installer config file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Installer YAML config file" ? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user will need to look up (or know) the schema anyway, so "YAML" doesn't add anything here IMO.
"${prog}" pxe customize src-initrd -o initrd "$@" | ||
} | ||
|
||
qemu_common() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would've been nice to leverage kola qemuexec
for this, but yeah... it'd require a bunch of changes. Might be worth thinking about long-term doing the enhancements necessary and switching over. E.g. I'd love to be able to do kola qemuexec -p pxe
.
Serialize and embed an empty Ignition config in a PXE initrd image. Unlike the pxe wrap commands, this subcommand takes a CoreOS initramfs and prepends it to the customizations. This is consistent with the "iso customize" semantics, and will allow us to validate that the OS image supports the customization features we're going to use. We verify that the input is a CoreOS live initrd, and that it hasn't already been customized. The former is done by checking for /etc/coreos-live-initramfs, which was previously an internal API between coreos-assembler and fedora-coreos-config; this makes that file's existence into a stable API. It's possible to implement in-place modification of the input, as "iso customize" does by default, but implementing in-place reversion to a pristine image would be tricky. (We'd need to truncate the image to the start of the concatenated CPIO archive that contains our customizations.) To avoid the footgun, omit in-place customization. However, the subcommand syntax is parallel with "iso customize" ("-o -" is required for stdout) so we could add this later.
Since they're user code connected with the sole reason for the live boot, and they may do something complex that the user might need to debug, direct their stdout/stderr to kmsg+console.
Read supported OS features from /etc/coreos/features.json in the live initramfs image or /coreos/features.json the live ISO.
Call the options --dest-karg-{append,delete} instead of --dest-{append,delete}-karg. This is inconsistent with the install subcommand and its config file, but provides cleaner UX for the customize subcommands.
Some options engage the installer and some don't. The docs try to make this clear, but it doesn't hurt to show a warning.
It helps debug failing tests.
Fully customizing the installation process can involve a lot of moving parts, including installer config files, NetworkManager key files, kernel arguments, and nested Ignition configs. Add user-friendly
iso customize
andpxe customize
wrappers allowing common customizations to be performed with individual command-line options.There are a lot of options here, and I'd like feedback on whether they provide the right set of functionality and whether they're clearly named.
This tool is helpful for visualizing Ignition configs generated by these subcommands.
cc @dustymabe for the networking parts in particular.