-
Notifications
You must be signed in to change notification settings - Fork 168
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 a qemu-iso platform #1571
Add a qemu-iso platform #1571
Conversation
I think what we may really want to do is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff. Definitely like the idea of making it easier to run the ISO without pulling out libvirt.
Though I think it'd be cleaner if instead of overloading the --qemu-image
arg for this, it's a separate option, e.g. --qemu-cdrom
. That should clean up the big if-statement switches based on the filename extension, and at the CLI level itself ISTM cleaner to be more explicit. Not a huge deal though if others want to get this in with the current semantics.
/cc @arithx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we may really want to do is:
kola run -p qemu-iso
andkola run -p qemu-metal
(the latter of which would actually go through the full install path).
But we can build that on top of this.
Agreed that ultimately ending up with them treated as independent platforms (that might re-use a lot / all of the platform builder code) is the desired end state.
Though I think it'd be cleaner if instead of overloading the --qemu-image arg for this, it's a separate option, e.g. --qemu-cdrom. That should clean up the big if-statement switches based on the filename extension, and at the CLI level itself ISTM cleaner to be more explicit. Not a huge deal though if others want to get this in with the current semantics.
In the far gone past we've done some auto-detection things on the images so I wouldn't block on it but I would agree that I prefer it being explicit (although as mentioned in the preivous reply I do think the correct level of explicitness is the platform rather than an image flag).
mantle/platform/qemu.go
Outdated
// primary disk is selected. This allows us to have "boot once" functionality on | ||
// both UEFI and BIOS (`-boot once=d` OTOH doesn't work with OVMF). | ||
|
||
// TBD: aarch64 does not support ide-cd but cdrom won't work either.Including it here as it doesn't error out at least. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to explicitly error out with a message denoting that this setup path won't work on the arch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is just moved, but I can do that too.
OK I am looking at making this a separate platform, copying code from qemu - but it will be a notably bigger patch. |
OK reworked to add |
16d76b4
to
f3a0d1d
Compare
/test sanity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh sorry, I think this needs a rebase on top of #1573 now.
Lower the bits to do "inject Ignition via coreos-installer embed iso" into the qemu layer. Copy the "qemu-unpriv" platform code into a "qemu-iso" platform (both are mostly thin wrappers around `qemu.go`). This makes it completely trivial to get a `ssh` shell in an ISO: `cosa run -p qemu-iso --memory 8192` And even better means we can start running a lot of the kola tests directly against the ISO (though not everything will work, e.g. tests that reboot can't persist state). I did verify this works: `kola run -p qemu-iso --qemu-memory 8192 basic`
Rebased 🏄 |
Nice! Tested this locally, and it works great. (Apart from having to stop the boot to type in /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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Not sure if this is related to this PR, but just saw this in the FCOS pipeline:
|
cosa commits since last successful build:
|
Ugh yeah it might be the qemu-iso platform changes. Sorry 😢 Who tests the tests etc |
This might not actually be related to this PR. Filed #1597. |
Lower the bits to do "inject Ignition via coreos-installer embed iso"
into the qemu layer. Copy the "qemu-unpriv" platform code into a
"qemu-iso" platform (both are mostly thin wrappers around
qemu.go
).This makes it completely trivial to get a
ssh
shell in an ISO:cosa run -p qemu-iso --memory 8192
And even better means we can start running a lot of the kola tests
directly against the ISO (though not everything will work, e.g.
tests that reboot can't persist state). I did verify this works:
kola run -p qemu-iso --qemu-memory 8192 basic