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

buildextend-installer: ship live PXE artifacts inside live ISO #1643

Merged
merged 5 commits into from
Aug 8, 2020
Merged

buildextend-installer: ship live PXE artifacts inside live ISO #1643

merged 5 commits into from
Aug 8, 2020

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Aug 6, 2020

@dustymabe discovered that the kernel does allow uncompressed appended initrds; it just needs them to start on a 4-byte boundary. This allows us to refactor the live ISO as a wrapper around bit-for-bit copies of the live PXE artifacts. That's useful because Fedora and RHEL users may expect to be able to copy PXE artifacts out of the ISO image.

Switch to an uncompressed rootfs image; pad the initramfs image appropriately so the two can still be concatenated. Move the ISO Ignition padding area to a separate file so the PXE initramfs artifact doesn't need to include padding. Move the kernel and initramfs to the paths within the ISO that Fedora and RHEL conventionally use, and put the rootfs image next to them. Put the rootfs stream hash in the initrd, and the osmet files in the rootfs, for both sets of artifacts.

There are several changes here that interlock with fedora-coreos-config, and no ratchet. I've temporarily added a commit to run cosa CI against coreos/fedora-coreos-config#548, but I propose that we ultimately merge over red and accept the transient CI break.

We're going to be implementing the ISO image in terms of the rootfs,
so this option will no longer provide any savings.
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Looks pretty good. A few comments/suggestions (optional). Will circle back after looking at the dracut module changes.

src/cmd-buildextend-installer Show resolved Hide resolved
src/cmd-buildextend-installer Show resolved Hide resolved
src/cmd-buildextend-installer Outdated Show resolved Hide resolved
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work @bgilbert

@dustymabe
Copy link
Member

There are several changes here that interlock with fedora-coreos-config, and no ratchet. I've temporarily added a commit to run cosa CI against coreos/fedora-coreos-config#548, but I propose that we ultimately merge over red and accept the transient CI break.

Personal Opinion: I'm fine with that assuming we coordinate the changes and try to keep downtime minimalish.

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.

A few comments but LGTM overall. Nice work!

src/cmd-buildextend-installer Outdated Show resolved Hide resolved
src/cmd-buildextend-installer Show resolved Hide resolved
src/cmd-buildextend-installer Show resolved Hide resolved
src/cmd-buildextend-installer Outdated Show resolved Hide resolved
Its contents are already compressed, and we can use the lack of a
compression wrapper to unify the ISO and PXE images.  It turns out that
the kernel allows uncompressed appended initrds, we just need to pad
the previous image to a 4-byte boundary.
Requires fedora-coreos-config/live bootloader config changes to append
/images/ignition.img as an additional initrd.
It seems to be conventional in Fedora/RHEL to put the kernel in
/images/pxeboot/vmlinuz and the initramfs in /images/pxeboot/initrd.img.

Requires matching fedora-coreos-config/live bootloader config changes.
Embed bit-for-bit copies of the initrd and rootfs in the ISO so that
users can copy them out again.  Place the squashfs at the beginning of
the rootfs image so 20live can find it.  Always keep the osmet files
in the rootfs and the rootfs stream hash in the base initrd.
@bgilbert
Copy link
Contributor Author

bgilbert commented Aug 8, 2020

Okay, CI is green, and I've dropped the last DNM: ci commit. Ready for /lgtm.

/hold

@jlebon
Copy link
Member

jlebon commented Aug 8, 2020

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgilbert, dustymabe, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [bgilbert,dustymabe,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@bgilbert
Copy link
Contributor Author

bgilbert commented Aug 8, 2020

/override continuous-integration/jenkins/pr-merge
/unhold

@openshift-ci-robot
Copy link

@bgilbert: Overrode contexts on behalf of bgilbert: continuous-integration/jenkins/pr-merge

In response to this:

/override continuous-integration/jenkins/pr-merge
/unhold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants