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

osmet: efficient packing/unpacking of CoreOS metal images #187

Merged
merged 2 commits into from
Apr 30, 2020
Merged

osmet: efficient packing/unpacking of CoreOS metal images #187

merged 2 commits into from
Apr 30, 2020

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Mar 13, 2020

Introduce a new osmet command with two primary subcommands: pack and
unpack. Together, these allow offline bare metal installation of
CoreOS images while only slightly increasing the size of the initramfs.

The pack subcommand takes as input a block device of a CoreOS metal
image and the expected checksum to match. It mounts the root partition
from that device and generates a smaller version of the metal image
itself with the OSTree objects "punched out" (this is called the
"punched image" in the code). The command outputs this smaller version
as well as a lookup table of where the OSTree objects belonged into an
"osmet" binary file.

The unpack subcommand takes as input an osmet binary file and a path
to an OSTree repo and reconstrust the metal image, bit for bit. This
command is more for testing in practice. The following patch will teach
the install command to use the osmet path by default, which is how
users will interact with this.

@jlebon
Copy link
Member Author

jlebon commented Mar 13, 2020

This is also missing scanning the /boot partition, which should take the size of the osmet binary further down from 80M to 10M. And of course, lots of prep patches and XXX markers to get back to. But the basic concept works otherwise!

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Mar 13, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.
@jlebon
Copy link
Member Author

jlebon commented Mar 13, 2020

See the cosa side of this at coreos/coreos-assembler#1244. That's one way of running this. Though you can also run the osmet-pack script there directly, e.g.:

$ cosa buildextend-metal
$ /usr/lib/coreos-assembler/osmet-pack builds/latest/x86_64/*-metal*.x86_64.raw tmp/osmet.bin <checksum>
...
Total OSTree objects scanned from root: 14614 (14609 mapped, 5 empty)
Total extents: 14609
Duplicate extents dropped: 0
Overlapping extents clamped: 0
Total bytes skipped: 1621889024
Total bytes written: 1237577728
Total bytes written (compressed): 82599292
$ ls -lh tmp/osmet.bin
-rw-------. 1 jlebon jlebon 80M Mar 13 17:01 tmp/osmet.bin

Or you can just bring up a VM with the metal image attached:

cosa run -- -drive if=none,format=raw,file=path/to/metal.raw,id=foo -device virtio-blk,drive=foo

And play with the coreos-installer osmet in there.

src/blockdev.rs Outdated Show resolved Hide resolved
src/blockdev.rs Outdated Show resolved Hide resolved
src/blockdev.rs Outdated Show resolved Hide resolved
src/blockdev.rs Outdated Show resolved Hide resolved
src/blockdev.rs Outdated Show resolved Hide resolved
src/fiemap.rs Outdated Show resolved Hide resolved
src/fiemap.rs Outdated Show resolved Hide resolved
src/cmdline.rs Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
@jlebon
Copy link
Member Author

jlebon commented Mar 16, 2020

I haven't addressed @lucab's review comments yet, but I fixed up all the XXX markers. We now also scan /boot for OSTree objects; this brings the total size of the osmet binary to 5.4M for FCOS!

@jlebon
Copy link
Member Author

jlebon commented Mar 16, 2020

Just noting here to remember for myself: in a conversation with @lucab, he mentioned it'd be useful to have a doc/ entry for this, similarly to the ISO Ignition config embedding functionality. I agree.

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Mar 17, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

OK so I only gave this the start of a review then ran out of bandwidth.

Overall...I trust you, and the code looks really great. I would be fine landing this and trying it out at least.

src/blockdev.rs Outdated Show resolved Hide resolved
src/cmdline.rs Show resolved Hide resolved
src/fiemap.rs Outdated Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
src/osmet.rs Outdated Show resolved Hide resolved
@jlebon jlebon changed the title WIP: osmet: efficient packing/unpacking of CoreOS metal images osmet: efficient packing/unpacking of CoreOS metal images Mar 26, 2020
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Mar 26, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.
@jlebon jlebon marked this pull request as ready for review March 26, 2020 16:53
@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2020

OK, marking this as ready for review! I've had to rework unpacking to make it happen in a thread so that we can just reuse the same ImageSource on the install.rs side as other sources. Another notable change is that I've made the osmet code its own Rust submodule and split it across three files to make it easier to find one's way around.

The second commit here is a key change: we teach coreos-installer install about using osmet as an image source, and make it the default installation method if it's available.

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2020

Some follow-ups to this:

  • make sure this works with metal4k (the groundwork is there to do this, but I haven't tested it yet)
  • possibly catch the optimization for when installing by stream is requested and it already matches our version (and so we can use the embedded osmet file)
  • make sure we're testing this in this upstream CI (though this repo is already doing kola testiso, so we should get it for free once we adapt that)

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2020

Hmm actually, I think we'll need to switch to it as the default as a separate step to help ratchet this in. So the order would be:

  • get osmet pack in (first commit here) and respin coreos-installer in FCOS
  • get the cosa patch in that runs osmet pack and bakes in the osmet files
  • get CI to test this using some env var
  • at some later time, once we're confident it's baked enough, we can drop the env var and make it the default

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2020

OK, dropped that commit!

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2020

CI fix in #198.
And fixed the unused warnings due to dropping the second commit!

@jamescassell
Copy link

How is it different than jigdo?

@cgwalters
Copy link
Member

How is it different than jigdo?

It's a more targeted and efficient solution for what we need to start.

See also rojig BTW which is inspired by jigdo, but (a lot like osmet) is optimized around OSTree and not debs/rpms.

@cgwalters
Copy link
Member

Can you confirm that we're not truly committing to any kind of ABI around osmet? In the end we will ship a coreos-installer binary and a .osmet file as part of the live rootfs that were versioned/built together? (Which I guess gets into coreos-assembler using the target coreos-installer and not the one in the container image)

And another question - we're still verifying the checksum of the resulting image, right? So at worst, osmet could fail to work, but would not write a corrupted image.

@jlebon
Copy link
Member Author

jlebon commented Apr 1, 2020

Can you confirm that we're not truly committing to any kind of ABI around osmet?

Yes, the osmet file isn't and shouldn't for the time being be a distinct public artifact. Importantly, we can assume a strong binding between the packing and unpacking paths (which as you mentioned is why coreos/coreos-assembler#1244 usies the coreos-installer binary from the tree itself). The versioning in the file format is more as a good practice than anything.

Concretely then, this means that we are free to change the implementation details of how offline installs work. Essentially the only API that we're making public with this is that there is an offline path that uses just the live ISO as the artifact.

And another question - we're still verifying the checksum of the resulting image, right? So at worst, osmet could fail to work, but would not write a corrupted image.

Correct. More precisely, the following checks are in place:

  1. cosa buildextend-live passes the expected checksum of the block device, which matches the checksum of the raw file itself from meta.json
  2. after packing, osmet pack does a final check to verify that re-unpacking the data yields the same checksum
  3. the checksum is written to the osmet header
  4. osmet unpack verifies during unpacking that the checksum from the header matches the final checksum of all the bytes written to the provided writer. This keys into the regular coreos-installer install behaviour of keeping the first 1M unwritten in case the checksums don't match.

src/cmdline.rs Show resolved Hide resolved
@cgwalters
Copy link
Member

One thing I can probably help with is adding kola testiso --scenario osmet once the "use osmet by default" PR lands too - it'd just be omitting the coreos.inst.image_url arg and we could also entirely omit networking from the VM too right?

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Apr 24, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.
@jlebon
Copy link
Member Author

jlebon commented Apr 24, 2020

Anything else to do to land this?

I was hoping for @bgilbert to have another look at this before merging since he did such an awesome review. But I think he's been away this week. I do think it's in a pretty good shape though and that we can fixup things as follow-ups. Maybe let's give it until Monday?

One thing I can probably help with is adding kola testiso --scenario osmet once the "use osmet by default" PR lands too - it'd just be omitting the coreos.inst.image_url arg and we could also entirely omit networking from the VM too right?

Thanks, that'd be awesome!

Actually, with this PR, we could have a test already that uses --offline for now with networking turned off in the ISO embed scenario.

And thinking more on this... since it's way more work to respin coreos-installer than cosa, maybe what we should do is land coreos/coreos-assembler#1244 first, but with the osmet packing step behind a switch to buildextend-live. And then we can test this PR in CI right here! Hmm, yeah let me do that.

@jlebon
Copy link
Member Author

jlebon commented Apr 24, 2020

One thing I wanted to note here is that I tested osmet in earnest on RHCOS yesterday (previously, I had only sanity-checked that coreos-installer osmet fiemap /usr/bin/foo didn't error out on FIEMAP_EXTENT_ENCODED).

And we can make it work, but the LUKS container complicates things. So the code here will need to be adapted to handle that (essentially adding to the mapping offsets the offset of the LUKS data).

IOW, this will not work yet as is on RHCOS.

@jlebon jlebon mentioned this pull request Apr 24, 2020
4 tasks
@cgwalters
Copy link
Member

And we can make it work, but the LUKS container complicates things.

Ug. It probably wouldn't be seriously hard to rework the rhcos luks bits to just leave a bit of space for the luks header before the root partition, and when LUKS is enabled, just move the partition back and generate the header only when it's required, that would allow us to drop all the LUKS bits from almost everywhere. Including the dm-linear bit.

@jlebon
Copy link
Member Author

jlebon commented Apr 24, 2020

Ug. It probably wouldn't be seriously hard to rework the rhcos luks bits to just leave a bit of space for the luks header before the root partition, and when LUKS is enabled, just move the partition back and generate the header only when it's required, that would allow us to drop all the LUKS bits from almost everywhere. Including the dm-linear bit.

That'd definitely make things simpler. Not that it's hard to handle it, but it feels awkward to do. From my experimentation yesterday, we have to cryptsetup luksOpen in osmet-pack in cosa for osmet to access the OSTree repo, and here, we have to special-case cipher_null LUKS containers and call out to cryptsetup luksDump to read the data offset.

Is the amount of work required to do the header fiddling work (and all the simplifications that ensue) justified given that (IIUC) the end goal is to get rid of it entirely in favor of coreos/fedora-coreos-tracker#94?

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Apr 24, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Apr 24, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.

One thing to note is that the metal image is now always required before
building the live ISO. This is because the whole point of osmet is to
efficiently pack the metal image.

Since osmet obsoletes the `fulliso` artifact, this PR also removes
support for it.
@jlebon
Copy link
Member Author

jlebon commented Apr 28, 2020

Let's hold this until tomorrow morning. @bgilbert told me he should be able to take another look then. (And actually, I still want to get the cosa bits in so we can test it in CI here.)

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Apr 28, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.

One thing to note is that the metal image is now always required before
building the live ISO. This is because the whole point of osmet is to
efficiently pack the metal image.

Since osmet obsoletes the `fulliso` artifact, this PR also removes
support for it.
openshift-merge-robot pushed a commit to coreos/coreos-assembler that referenced this pull request Apr 29, 2020
This is the server side required for osmet packing:
coreos/coreos-installer#187

We run `osmet pack` when creating the live ISO and include the osmet
binary as part of the CPIO containing the root squashfs.

One thing to note is that the metal image is now always required before
building the live ISO. This is because the whole point of osmet is to
efficiently pack the metal image.

Since osmet obsoletes the `fulliso` artifact, this PR also removes
support for it.
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.

LGTM generally. Some nits, but I'm okay deferring those to a followup.

src/osmet/file.rs Show resolved Hide resolved
src/osmet/mod.rs Outdated Show resolved Hide resolved
src/blockdev.rs Outdated Show resolved Hide resolved
src/cmdline.rs Outdated Show resolved Hide resolved
src/osmet/fiemap.rs Outdated Show resolved Hide resolved
src/osmet/file.rs Outdated Show resolved Hide resolved
src/osmet/io_helpers.rs Outdated Show resolved Hide resolved
src/osmet/unpacker.rs Outdated Show resolved Hide resolved
src/source.rs Outdated Show resolved Hide resolved
@jlebon
Copy link
Member Author

jlebon commented Apr 29, 2020

OK, addressed comments and added a second commit here which turns on CI testing via osmet! Feel free to do another final review on the latest minor changes, though otherwise will merge on green!

src/osmet/mod.rs Outdated Show resolved Hide resolved
src/source.rs Outdated Show resolved Hide resolved
jlebon added 2 commits April 29, 2020 20:42
Introduce a new `osmet` command with two primary subcommands: `pack` and
`unpack`. Together, these allow offline bare metal installation of
CoreOS images while only slightly increasing the size of the initramfs.

The `pack` subcommand takes as input a block device of a CoreOS metal
image and the expected checksum to match. It mounts the root partition
from that device and generates a smaller version of the metal image
itself with the OSTree objects "punched out" (this is called the
"punched image" in the code). The command outputs this smaller version
as well as a lookup table of where the OSTree objects belonged into an
"osmet" binary file.

The `unpack` subcommand takes as input an osmet binary file and a path
to an OSTree repo and reconstrust the metal image, bit for bit. This
command is more for testing in practice. The following patch will teach
the `install` command to use the osmet path by default, which is how
users will interact with this.
This turns on CI testing for the offline path (via osmet). In the
future, we will drop `--osmet` entirely since it'll always be on. (And
similarly, the `iso-offline-install` might become the same thing as
`iso-install` so we might be able to drop it entirely.)

See coreos/coreos-assembler#1244.
@jlebon
Copy link
Member Author

jlebon commented Apr 30, 2020

Successfully tested scenario iso-offline-install for 31.20200430.dev.0 on bios (metal)

🎉

@jlebon jlebon merged commit c238ffa into coreos:master Apr 30, 2020
@jlebon jlebon deleted the pr/osmet branch April 30, 2020 12:59
@dustymabe
Copy link
Member

Yay. this works! My cosa run made a metal image that looks like:

$ wc --bytes builds/latest/x86_64/fedora-coreos-31.20200430.dev.0-metal.x86_64.raw
2959081472 builds/latest/x86_64/fedora-coreos-31.20200430.dev.0-metal.x86_64.raw

$ sha256sum builds/latest/x86_64/fedora-coreos-31.20200430.dev.0-metal.x86_64.raw
9dd23ee016f4c9e8562661dc6cce92f90a43e727f9254f2f671383aecc87a413  builds/latest/x86_64/fedora-coreos-31.20200430.dev.0-metal.x86_64.raw

I booted to the Live ISO and run the installer in offline mode!

$ sudo coreos-installer install /dev/sda --offline
Installing Fedora CoreOS 31.20200430.dev.0 x86_64 (512-byte sectors)
> Read disk 2.8 GiB/2.8 GiB (100%)
Install complete.

And then I read back the written out contents and it matches the same
checksum of the metal image!

$ sudo dd if=/dev/sda bs=512 count=5779456 | sha256sum
5779456+0 records in
5779456+0 records out
2959081472 bytes (3.0 GB, 2.8 GiB) copied, 24.9252 s, 119 MB/s
9dd23ee016f4c9e8562661dc6cce92f90a43e727f9254f2f671383aecc87a413  -

Nice work @jlebon!

@dustymabe
Copy link
Member

One thing that might be concerning from an "optics" point of view from the end user perspective is that the installer now reports a much larger number than it did in the past because it's dealing with uncompressed data rather than real data.. Using --offline:

> Read disk 2.8 GiB/2.8 GiB (100%)

using --stream=next:

Downloading next image (raw.xz) and signature
> Read disk 74.3 MiB/483.3 MiB (15%)

I know the resulting written image to disk is the same size but it does make us look like we're bigger than we were even though we've actually been this size the whole time.

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