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 osmet packing #1244

Merged
merged 2 commits into from
Apr 29, 2020
Merged

Conversation

jlebon
Copy link
Member

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

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 Mar 26, 2020

I have a follow-up to this which is to tweak kola testiso to test the offline path in the live ISO track (but keep using the networking one in the live PXE for coverage). But we don't have to block on that I think since it won't actually pass until coreos/coreos-installer#187 is in and tested.

Another big followup to this I started on is running osmet pack twice: once for metal and once more for metal4k if built.

@ashcrow
Copy link
Member

ashcrow commented Mar 26, 2020

Is it safe to say we're waiting for this coreos-install PR to be updated before test pass?

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2020

Yup, exactly!

(BTW, I actually hadn't realized you could link to a specific file in the logs, that's neat!)

src/osmet-pack Outdated
set -- "$@" "${TMPDIR}/coreos-installer"
fi

runvm -drive "if=virtio,id=coreos,format=raw,readonly=on,file=${img_src}" -- \
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this runvm stuff recently and I realized that we could actually switch to a two-phased system where we first build qemu/metal image, and then all runvm tasks after that simply boot that image with an Ignition config - making us much more self-hosting.

(The downside of this is that for RHCOS we can't use 9p but that problem will really, hopefully soon be solved with virtiofs).

Copy link
Member

Choose a reason for hiding this comment

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

To elaborate on this, we wouldn't have a runvm API internally distinct from buildextend-qemu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be interesting. One thing we lose though is that supermin automatically imports all the packages from the build environment. Obviously we could do something similar with Ignition/a systemd service fetching the files we want, but it's nice that it works transparently with supermin.

It would be a good fit for osmet though since we normally want to use the coreos-installer from the FCOS node in that case. So in the developer case, we only have to upload the custom coreos-installer really.

src/osmet-pack Outdated Show resolved Hide resolved
cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Apr 22, 2020
A lot of our code wants things like `/dev/disk/by-X`.  Extracted
from coreos#1244
openshift-merge-robot pushed a commit that referenced this pull request Apr 22, 2020
A lot of our code wants things like `/dev/disk/by-X`.  Extracted
from #1244
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Apr 24, 2020
For coreos#1244, I want to use `serial=osmet` on the disk to mark which one is
the source disk containing the metal image instead of relying on
`index=N`. However, to use `serial=osmet`, I have to use a separate
`-device` arg instead of the implied one from `if=virtio`, and it seems
like doing that causes qemu to give those disks higher priority such
that e.g. it gets `/dev/vda`, which throws off supermin.

Let's switch over all our disks to use `-drive ... -device ...`. While
we're here, only rely on `index=N` for the supermin root itself, since
we can't use udev symlinks in the kernel cmdline. The cache disk too
doesn't need an index, we use the LABEL instead already.
@jlebon
Copy link
Member Author

jlebon commented Apr 24, 2020

OK, this now requires #1398!

@jlebon jlebon force-pushed the pr/osmet branch 2 times, most recently from 4d5caf5 to 35a62a3 Compare April 24, 2020 20:34
@jlebon
Copy link
Member Author

jlebon commented Apr 24, 2020

OK, as mentioned in coreos/coreos-installer#187 (comment), we now have a cosa buildextend-live --osmet switch which optionally activates osmet packing. This means we should be able to land this PR now!

The second commit adds a new testiso scenario:

kola/testiso: add support for offline testing

Add a new `iso-offline-install` scenario which tests the osmet path. In
this path, we completely turn off networking to the host (`-nic none`).

We pass `--offline` to force coreos-installer to use the osmet path. In
the future, we should be able to omit this entirely, which will test
that coreos-installer correctly defaults to osmet when not provided
another image source.

As mentioned in the comment, this might become the default behaviour of
`iso-install` in the future once osmet is fully baked in.

So once this is in, I'll update coreos/coreos-installer#187 to use cosa buildextend-live --osmet and kola testiso -S --scenarios iso-offline-install,... so we can test this in CI!

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Apr 28, 2020
For coreos#1244, I want to use `serial=osmet` on the disk to mark which one is
the source disk containing the metal image instead of relying on
`index=N`. However, to use `serial=osmet`, I have to use a separate
`-device` arg instead of the implied one from `if=virtio`, and it seems
like doing that causes qemu to give those disks higher priority such
that e.g. it gets `/dev/vda`, which throws off supermin.

Let's switch over all our disks to use `-drive ... -device ...`. While
we're here, only rely on `index=N` for the supermin root itself, since
we can't use udev symlinks in the kernel cmdline. The cache disk too
doesn't need an index, we use the LABEL instead already.
openshift-merge-robot pushed a commit that referenced this pull request Apr 28, 2020
For #1244, I want to use `serial=osmet` on the disk to mark which one is
the source disk containing the metal image instead of relying on
`index=N`. However, to use `serial=osmet`, I have to use a separate
`-device` arg instead of the implied one from `if=virtio`, and it seems
like doing that causes qemu to give those disks higher priority such
that e.g. it gets `/dev/vda`, which throws off supermin.

Let's switch over all our disks to use `-drive ... -device ...`. While
we're here, only rely on `index=N` for the supermin root itself, since
we can't use udev symlinks in the kernel cmdline. The cache disk too
doesn't need an index, we use the LABEL instead already.
jlebon added 2 commits April 28, 2020 11:19
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.
Add a new `iso-offline-install` scenario which tests the osmet path. In
this path, we completely turn off networking to the host (`-nic none`).

We pass `--offline` to force coreos-installer to use the osmet path. In
the future, we should be able to omit this entirely, which will test
that coreos-installer correctly defaults to osmet when not provided
another image source.

As mentioned in the comment, this might become the default behaviour of
`iso-install` in the future once osmet is fully baked in.
@jlebon
Copy link
Member Author

jlebon commented Apr 28, 2020

OK, this is ready to go now! Tested succesfully with coreos/coreos-installer#187:

$ kola testiso -S --scenarios iso-offline-install
Testing scenarios: [iso-offline-install]
Successfully tested scenario iso-offline-install for 31.20200424.dev.1 on bios (metal)

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, 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:

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

@openshift-merge-robot openshift-merge-robot merged commit 0af9ac7 into coreos:master Apr 29, 2020
@jlebon jlebon deleted the pr/osmet branch July 6, 2020 20:32
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