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 support for minimal ISO packing/unpacking #559

Merged
merged 4 commits into from
Nov 10, 2021
Merged

Add support for minimal ISO packing/unpacking #559

merged 4 commits into from
Nov 10, 2021

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jun 11, 2021

This command allows packing into the live ISO a minimal version of
itself which does not contain the root squashfs. The use case for
minimal ISOs has come up multiple times, notably in:

coreos/fedora-coreos-tracker#661

This is a sort of intermediate approach where we don't officially ship a
new artifact, but allow users to derive the minimal ISO from the
official live ISO.

The way this works is similar to osmet. It requires a "packing" step at
build time which adds an osmet-like file (the miniso data file) on the
full ISO itself. The data file describes how to construct a minimal ISO
from the full ISO. It weighs in at a few kilobytes, so the impact of the
size of the live ISO is negligible. If we ever officially do ship a
minimal ISO, it also allows for the possibility of matching its checksum
exactly.

The method should be arch-independent and does not rely on e.g.
modifying ISO9660 structures or moving the GPT backup header, etc...

We'll want to also add a --rootfs-url which uses the iso kargs code
to inject coreos.live.rootfs_url since that's the primary use case so
it can all be done in a single step.

One of the primary goals is to ideally satisfy the needs of the Assisted
Installer (openshift/assisted-installer), though
there's more things needed before we get there (notably #545).

@tomassedovic
Copy link

I'd like to voice support for this on behalf of the Bare Metal IPI team. The requirement for this minimal ISO came about because some hardware has an actual size limit for virtual media. This is why the Assisted Installer people implemented this on their side, but that leaves any BM IPI deployments without this ability.

Having this in the coreos-installer will let both workflows share the code and handle the same hardware limitations.

@jlebon
Copy link
Member Author

jlebon commented Jun 16, 2021

Worth summarizing on some of the points in coreos/fedora-coreos-tracker#661:

  • Mounting the full live ISO virtually can be extremely slow in some environments. Some environments have hard size limits.
  • Users should try really hard to use PXE for installs and not the ISO, but that's sometimes not possible if e.g. there's no DHCP or low connectivity.
  • Providing this option makes our install story more complex.
  • A different approach which could resolve some of the issues is shipping coreos-installer in the initramfs (so then a user only needs the PXE kernel and initramfs).
  • It's not hard to DIY a minimal ISO from the published PXE artifacts; we could document it or even provide a tool for this.
  • Complicates network configuration; would need to shove everything in kargs.

Some commentary:

  • Providing this option makes our install story more complex.

This is true, though if there are legitimate and not uncommon environments which cannot have their needs met with our current approaches, then the choice is really between us providing a supported path forward or them figuring it out on their own, possibly doing unsupported things in the process.

  • A different approach which could resolve some of the issues is shipping coreos-installer in the initramfs (so then a user only needs the PXE kernel and initramfs).

This is worth considering because I think it could also help with coreos/fedora-coreos-tracker#399.

  • Complicates network configuration; would need to shove everything in kargs.

Yes, this is true. Network kargs are terrible and may not cover all use cases, so this would really want something like #545 which would allow defining network configs a single time as NM keyfiles and embedded into the ISO.

  • It's not hard to DIY a minimal ISO from the published PXE artifacts; we could document it or even provide a tool for this.

Another way to look at this then I guess is that this is the tool we provide to do this. Also shout-out to @dustymabe who actually suggested exactly this functionality early on. Totally missed that before.

@bgilbert
Copy link
Contributor

  • A different approach which could resolve some of the issues is shipping coreos-installer in the initramfs (so then a user only needs the PXE kernel and initramfs).

This is worth considering because I think it could also help with coreos/fedora-coreos-tracker#399.

RHCOS originally shipped coreos-installer in the initramfs and we moved away from doing that, for a few reasons:

  • If coreos-installer is in the initramfs, the only practical way to control it is via kernel arguments, which we've been trying not to proliferate further.
  • The existing hook mechanisms don't work in the initramfs because Ignition configs can't affect that environment. Any alternative customization mechanism we might build (e.g. injecting systemd units via an appended initrd) would, for the first time, encourage users to execute arbitrary code in the initramfs, adding all sorts of new compatibility constraints.
  • It makes the initramfs larger to support an unusual case.

I'd rather we solve coreos/fedora-coreos-tracker#399 in a different way.

@cgwalters
Copy link
Member

I'm in favor of this generally. Though we also ship a ton of pre-generated artifacts and adding this new small one doesn't seem like a big deal either to me.

@jlebon
Copy link
Member Author

jlebon commented Jun 16, 2021

We discussed this in the meeting today. Overall there wasn't strong objections, but @bgilbert mentioned that if the problems are real, maybe we should simply ship the minimal ISO as a bona fide artifact. This would make the code here less necessary (though it's still handy to be able to extract the minimal ISO and rootfs.img from the live ISO).

@cgwalters
Copy link
Member

To copy/paste IRC:

<dustymabe> walters: considering the rehydrator - i wonder if we could get the same functionality just all using coreos-installer and embedded osmets
<walters> right this overlaps with the rehydrator (https://github.com/cgwalters/coreos-diskimage-rehydrator), it's a more targeted version for a subset of that. That's fine IMO, it's not totally clear we're going to ship the rehydrator, and even if we did I think it's fine if coreos-installer deals with the ISO

@dustymabe
Copy link
Member

dustymabe commented Jun 16, 2021

here's a comment I made in the meeting and was asked to add to the ticket:

I like not shipping another artifact (keeping confusion down) - people who need the minimal ISO will ask or otherwise find the docs for how to create it.

@jlebon
Copy link
Member Author

jlebon commented Jun 16, 2021

here's a comment I made in the meeting and was asked to add to the ticket:

I like not shipping another artifact (keeping confusion down) - people who need the minimal ISO will ask or otherwise find the docs for how to create it.

I think that's a valid point. IOW, the use case is not common enough to warrant a full artifact and everything it entails (including potentially inducing confusion), but common enough for us to provide tools for it.

Two other points I'll bring up:

  1. Related to the above: it's easy to add artifacts, but it's hard to remove them. So doing it this way allows us to provide some support without fully committing to shipping another artifact (which we may never even do in the end).
  2. This was mentioned in the meeting, but capturing here: a minimal ISO on its own isn't really useful because you really need to at least provide coreos.live.rootfs_url (I guess we could default to fetching the rootfs.img from the release metadata but it's very unlikely what the user wants and adds more constraints on the releng side). Forcing the use of a tool like this can guide users towards doing the right thing ("host the artifact spit out by --save-rootfs and add its link in --rootfs-url").

@bgilbert
Copy link
Contributor

This was mentioned in the meeting, but capturing here: a minimal ISO on its own isn't really useful because you really need to at least provide coreos.live.rootfs_url (I guess we could default to fetching the rootfs.img from the release metadata but it's very unlikely what the user wants and adds more constraints on the releng side). Forcing the use of a tool like this can guide users towards doing the right thing ("host the artifact spit out by --save-rootfs and add its link in --rootfs-url").

Yeah, I find this part persuasive.

@dustymabe
Copy link
Member

@jlebon jlebon changed the title WIP/RFC: Add coreos-installer iso uproot WIP: Add coreos-installer iso uproot Jun 24, 2021
@jlebon
Copy link
Member Author

jlebon commented Jun 24, 2021

Dropping RFC since this was approved now.

@dtantsur
Copy link

Hi folks! Will the minimal ISO be customizable via https://coreos.github.io/coreos-installer/iso-embed-ignition/ or will the only customization option be providing an ignition URL? I'm thinking about environments where DHCP is not available.

@jlebon
Copy link
Member Author

jlebon commented Jun 29, 2021

Hi folks! Will the minimal ISO be customizable via coreos.github.io/coreos-installer/iso-embed-ignition or will the only customization option be providing an ignition URL? I'm thinking about environments where DHCP is not available.

Yes, embedding Ignition configs and modifying kargs will still be possible with the minimal ISO.

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Sep 24, 2021
This switch enables the new minimal ISO packing feature (see
coreos/coreos-installer#559).

Once a coreos-installer version with miniso support is present in RHCOS
and FCOS, we can make it unconditional and drop the switch. For now,
this will allow testing in the coreos-installer upstream CI.
@jlebon jlebon changed the title WIP: Add coreos-installer iso uproot Add support for minimal ISO packing/unpacking Sep 24, 2021
@jlebon
Copy link
Member Author

jlebon commented Sep 24, 2021

OK, updated this now and ready for review!

Requires: #635
Requires: coreos/coreos-assembler#2466

This is still missing the last bit, which is supporting a --rootfs-url switch. Will tackle that in a separate commit. (Edit: added this bit now in the final push, and updated the cosa PR accordingly!)

@jlebon jlebon marked this pull request as ready for review September 24, 2021 21:53
@dtantsur
Copy link

\o/

Is there a way to test this PR now or does it require any changes to published images (the miniso data file)?

@jlebon
Copy link
Member Author

jlebon commented Sep 27, 2021

\o/

Is there a way to test this PR now or does it require any changes to published images (the miniso data file)?

Yes, it requires coreos/coreos-assembler#2466.

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Sep 28, 2021
This switch enables the new minimal ISO packing feature (see
coreos/coreos-installer#559).

Once a coreos-installer version with miniso support is present in RHCOS
and FCOS, we can make it unconditional and drop the switch. For now,
this will allow testing in the coreos-installer upstream CI.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Oct 1, 2021
This switch enables the new minimal ISO packing feature (see
coreos/coreos-installer#559).

Once a coreos-installer version with miniso support is present in RHCOS
and FCOS, we can make it unconditional and drop the switch. For now,
this will allow testing in the coreos-installer upstream CI.
jlebon added a commit to coreos/coreos-assembler that referenced this pull request Oct 3, 2021
This switch enables the new minimal ISO packing feature (see
coreos/coreos-installer#559).

Once a coreos-installer version with miniso support is present in RHCOS
and FCOS, we can make it unconditional and drop the switch. For now,
this will allow testing in the coreos-installer upstream CI.
@jlebon
Copy link
Member Author

jlebon commented Oct 4, 2021

[2021-10-03T15:58:32.647Z] + kola testiso -S --scenarios pxe-install,pxe-offline-install,iso-install,iso-offline-install,miniso-install --output-dir tmp/kola-testiso-metal
[2021-10-03T15:58:32.647Z] Testing scenarios: [pxe-offline-install iso-install iso-offline-install miniso-install pxe-install]
...
[2021-10-03T16:07:34.647Z] Successfully tested scenario miniso-install for 34.20211003.dev.0 on bios (metal)

🎉

Copy link
Contributor

@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.

This is great! Minor nits/cleanups, plus two issues: extract will fail on a hash mismatch if the full ISO has an embedded config or kargs, and iso kargs reset on the minimal ISO will reinstate coreos.liveiso.

.cci.jenkinsfile Show resolved Hide resolved
src/cmdline.rs Outdated Show resolved Hide resolved
src/miniso.rs Outdated Show resolved Hide resolved
src/miniso.rs Outdated Show resolved Hide resolved
src/miniso.rs Outdated Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
src/live.rs Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
src/cmdline.rs Outdated Show resolved Hide resolved
Prep for calling it in a future patch.
@jlebon
Copy link
Member Author

jlebon commented Nov 5, 2021

Updated for comments!

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Nov 5, 2021
@jlebon
Copy link
Member Author

jlebon commented Nov 5, 2021

Requires: coreos/coreos-assembler#2543

jlebon added a commit to coreos/coreos-assembler that referenced this pull request Nov 8, 2021
Copy link
Contributor

@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.

Looks great! Just some minor fixes/nits.

src/live.rs Outdated Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
src/miniso.rs Outdated Show resolved Hide resolved
src/live.rs Outdated Show resolved Hide resolved
Prep for using it in a future patch.
Prep for using it in a future patch.
This command allows packing into the live ISO a minimal version of
itself which does not contain the root squashfs. The use case for
minimal ISOs has come up multiple times, notably in:

coreos/fedora-coreos-tracker#661

This is a sort of intermediate approach where we don't officially ship a
new artifact, but allow users to derive the minimal ISO from the
official live ISO.

The way this works is similar to osmet. It requires a "packing" step at
build time which adds an osmet-like file (the miniso data file) on the
full ISO itself. The data file describes how to construct a minimal ISO
from the full ISO. It weighs in at a few kilobytes, so the impact of the
size of the live ISO is negligible. If we ever officially do ship a
minimal ISO, it also allows for the possibility of matching its checksum
exactly.

The method should be arch-independent and does not rely on e.g.
modifying ISO9660 structures or moving the GPT backup header, etc...

We support a `--rootfs-url` option which uses the `iso kargs` code to
inject `coreos.live.rootfs_url` since that's the primary use case. That
way, the minimal ISO can be prepared in a single step.

One of the primary goals is to ideally satisfy the needs of the Assisted
Installer (https://github.com/openshift/assisted-installer), though
there's more things needed before we get there (notably #545).
@jlebon
Copy link
Member Author

jlebon commented Nov 10, 2021

Updated!

Comment on lines +195 to +198
matches,
skipped,
written,
written_compressed,
Copy link
Member Author

Choose a reason for hiding this comment

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

Was really tempted to return a struct like XzPackStats or something here to group these loose return values. But... let's just keep that as a follow-up.

Copy link
Contributor

@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.

🎈 🎉 🎆

@jlebon jlebon enabled auto-merge November 10, 2021 19:34
@jlebon jlebon merged commit c396abf into coreos:main Nov 10, 2021
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.

6 participants