cmdlib: Use kola qemuexec, delete qemu-kvm code #1289
Conversation
cgwalters
force-pushed
the
cosa-always-qemuexec
branch
2 times, most recently
from
47f640e to
dd53484
Compare
|
Right, forgot this depends on #1285 too since the 9p bits reference |
|
For example this PR would have been unnecessary because we already had the right thing in |
cgwalters
force-pushed
the
cosa-always-qemuexec
branch
2 times, most recently
from
3eb4939 to
82a4148
Compare
|
Do we need an update to CI or an update to qemuexec? |
I don't yet know why that's failing, but I plan to debug it more once the dependencies (all currently passing CI) have landed. |
|
Still working on this, split out #1342 |
This is a followup to: coreos#1342 which is part of: coreos#1289 Our builds right now run qemu in a way that's highly sensitive to changes in device ordering. SCSI hardware supports a [world wide name](https://en.wikipedia.org/wiki/World_Wide_Name); change our qemu invocation to use `42` (in decimal) for that. Unfortunately because we're not using udev in our supermin VM right now we don't get the nice `/dev/disk/by-id` symlink to it. Instead just walk `/sys/block` manually.
This is a followup to: coreos#1342 which is part of: coreos#1289 Our builds right now run qemu in a way that's highly sensitive to changes in device ordering. SCSI hardware supports a [world wide name](https://en.wikipedia.org/wiki/World_Wide_Name); change our qemu invocation to use `42` (in decimal) for that. Unfortunately because we're not using udev in our supermin VM right now we don't get the nice `/dev/disk/by-id` symlink to it. Instead just walk `/sys/block` manually.
This is a followup to: coreos#1342 which is part of: coreos#1289 Our builds right now run qemu in a way that's highly sensitive to changes in device ordering. SCSI hardware supports a [world wide name](https://en.wikipedia.org/wiki/World_Wide_Name); change our qemu invocation to use `42` (in decimal) for that. Unfortunately because we're not using udev in our supermin VM right now we don't get the nice `/dev/disk/by-id` symlink to it. Instead just walk `/sys/block` manually.
cgwalters
force-pushed
the
cosa-always-qemuexec
branch
from
82a4148 to
0024ac0
Compare
cgwalters
force-pushed
the
cosa-always-qemuexec
branch
from
0024ac0 to
b207d09
Compare
|
OK cool, so this one is working now! Just depends on #1343 merging first. |
|
tested this on s390x , did a kola test run ..looks good..i will test on ppc64le too but i don't foresee any issues |
|
OK since we're pretty confident in this I'd say we just merge this one directly! |
|
Tested on x86_64, ran buildextend-metal, then went through the coreos-installer with the raw image. Looks good! |
|
/hold cancel |
|
Just to be safe, could we hold this a bit more until we build the |
This is part of speeding up `cosa build-fast`; here the `dhclient` run takes about a second out of 11 (after other speedups) so that's a 9% win. After rebasing on coreos#1289 I'll also drop out the virtio-net NIC entirely which will help us ensure tasks that shouldn't pull external content don't.
This is part of speeding up `cosa build-fast`; here the `dhclient` run takes about a second out of 11 (after other speedups) so that's a 9% win. After rebasing on #1289 I'll also drop out the virtio-net NIC entirely which will help us ensure tasks that shouldn't pull external content don't.
That's done now? https://builds.coreos.fedoraproject.org/browser?stream=next |
ashcrow
requested review from
bgilbert and
jlebon
and removed request for
ashcrow
jlebon
left a comment
jlebon
left a comment
There was a problem hiding this comment.
Very nice cleanup! Just minor comments, LGTM overall.
| fi | ||
| mkdir -p "${workdir}"/cache /host/container-work | ||
| cachedev=$(blkid -lt LABEL=cosa-cache -o device) | ||
| cachedev=$(blkid -lt LABEL=cosa-cache -o device || true) |
There was a problem hiding this comment.
Hmm, this is already in the tree (#1345), but ahh... it's the exact same commit, so that's why GitHub and Jenkins aren't complaining. OK, that should work then; I'm a bit interested to see GitHub handle it.
| if [ -z "${disk:-}" ]; then | ||
| # hex 0x2a = 42 in decimal; this is set in cmd-buildextend-metal. | ||
| # We use the WWN as an unambiguous way to identify our target disk, | ||
| # independent of other devices attached to the VM (caches, etc.) |
There was a problem hiding this comment.
Note virtio-blk does support something similar as well with serial=... (that's what coreos/ignition#905 uses). This is fine too though! (I would think virtio-scsi is slower than virtio-blk for our use case, but since it's not a long-running VM or anything, it's probably negligible overall. Not to mention our cache qcow2 has been using SCSI for a while too now.)
There was a problem hiding this comment.
Ahh right...hmm, yes, I think if I just steal these bits from your osmet PR we could switch to serial and /dev/disk/by-id. Will try. What I ran into is that blkid didn't know how to parse the serials or something, and I thought running udev outside of systemd would be harder than that.
# we want /dev/disk symlinks for coreos-installer
/usr/lib/systemd/systemd-udevd --daemon
/usr/sbin/udevadm trigger --settle
There was a problem hiding this comment.
OK I fought this in cgwalters@75aeaba and couldn't get it to work; I think somehow the target drive is ending up as /dev/vda. See also coreos/ignition#905 (comment)
There was a problem hiding this comment.
Replied in coreos/ignition#905 (comment). But yeah, let's just always enable udev to make all this cleaner.
There was a problem hiding this comment.
OK so...I tried to use this for the root device but that's in the supermin code which has its own custom init that only handles /dev/vda or UUIDs (and for some reason it uses random UUIDs still for the rootfs...)
The flip side of this is...using WWNs is also natural for people doing installs to real devices (e.g. with coreos-installer), so it's somewhat useful to also do that for our builds.
There was a problem hiding this comment.
I think my vote hence is to land this, since it's tested and an improvement, we we can do something else as followup.
And this final commit gets rid of almost all the core qemu logic in cmdlib, so e.g. architecture specific changes only need to happen in one place. The only special casing left is `$devtype` and the terminal. First, change how we identify devices. This is a followup to: coreos#1342 which is part of: coreos#1289 Our builds right now run qemu in a way that's highly sensitive to changes in device ordering. SCSI hardware supports a [world wide name](https://en.wikipedia.org/wiki/World_Wide_Name); change our qemu invocation to use `42` (in decimal) for that. Unfortunately because we're not using udev in our supermin VM right now we don't get the nice `/dev/disk/by-id` symlink to it. Instead just walk `/sys/block` manually. Next, use `kola qemuexec`. We inherit the virtio-rand setup, CPU calculations etc. And now we can just use virtio for the root disk and the cache disk, which drops all the eyeball-glazing scsi incantations. (Well, of course arch changes also end up in libvirt which we use indirectly too, but at least *we* only have one place)
This is a followup to: coreos#1342 which is part of: coreos#1289 Our builds right now run qemu in a way that's highly sensitive to changes in device ordering. SCSI hardware supports a [world wide name](https://en.wikipedia.org/wiki/World_Wide_Name); change our qemu invocation to use `42` (in decimal) for that. Unfortunately because we're not using udev in our supermin VM right now we don't get the nice `/dev/disk/by-id` symlink to it. Instead just walk `/sys/block` manually.
And this final commit gets rid of almost all the core qemu logic in cmdlib, so e.g. architecture specific changes only need to happen in one place. The only special casing left is `$devtype` and the terminal. We inherit the virtio-rand setup, CPU calculations etc. And now we can just use virtio for the root disk and the cache disk, which drops all the eyeball-glazing scsi incantations. (Well, of course arch changes also end up in libvirt which we use indirectly too, but at least *we* only have one place)
cgwalters
force-pushed
the
cosa-always-qemuexec
branch
from
b207d09 to
6a78bde
Compare
|
/lgtm |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is a followup to: #1342 which is part of: #1289 Our builds right now run qemu in a way that's highly sensitive to changes in device ordering. SCSI hardware supports a [world wide name](https://en.wikipedia.org/wiki/World_Wide_Name); change our qemu invocation to use `42` (in decimal) for that. Unfortunately because we're not using udev in our supermin VM right now we don't get the nice `/dev/disk/by-id` symlink to it. Instead just walk `/sys/block` manually.
Regression from coreos#1289 that probably explains other mysterious problems I was seeing too. Basically, if a qemu image already existed, we would auto-load it with `kola qemuexec`. In the case of builds, we absolutely do not want to do that. I probably didn't see this *initially* when working on this because my build didn't have a qemu image yet.
Regression from coreos#1289 that probably explains other mysterious problems I was seeing too. Basically, if a qemu image already existed, we would auto-load it with `kola qemuexec`. In the case of builds, we absolutely do not want to do that. I probably didn't see this *initially* when working on this because my build didn't have a qemu image yet.
Regression from #1289 that probably explains other mysterious problems I was seeing too. Basically, if a qemu image already existed, we would auto-load it with `kola qemuexec`. In the case of builds, we absolutely do not want to do that. I probably didn't see this *initially* when working on this because my build didn't have a qemu image yet.
7 participants
And this final commit gets rid of all of the qemu hardcoding in
cmdlib, so e.g. architecture specific changes only need to happen
in one place.
(Well, of course arch changes also end up in libvirt which we
use indirectly too, but at least we only have one place)