Introduce kola qemuexec --devshell, make cosa run use it
#1338
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cgwalters
force-pushed
the
qdevsh
branch
3 times, most recently
from
8f8e9a6 to
d3e0778
Compare
Member
Author
|
OK rebased 🏄 - I think you will all like using this as much as I did writing it! |
mike-nguyen
reviewed
Apr 16, 2020
Member
|
I took this PR for a test drive. It's a much better UX than prior! No more annoyances with overwritten terminal lines and no more resetting my terminal after |
jlebon
reviewed
Apr 16, 2020
Member
jlebon
left a comment
jlebon
left a comment
There was a problem hiding this comment.
This looks good overall! Just a few nits.
This finally unifies the advantages of `cosa run` and `kola spawn`. I kept getting annoyed by how serial console sizing is broken (e.g. trying to use `less` etc.). Using `ssh` via `kola spawn` addresses that, but it means you can't debug the initramfs. Now things work in an IMO pretty cool way; if you do e.g. `cosa run --kargs ignition.config.url=blah://` (or inject a bad Ignition config) to cause a failure in the initramfs, you'll see a nice error (building on coreos/ignition-dracut#146 ) telling you to rerun with `cosa run --devshell-console`. Things are also wired up cleanly so that we support rebooting with the equivalent of `kola spawn --reconnect` (which we should probably remove now). You can exit via *either* quitting SSH cleanly or using `poweroff`, and the lifecycle of ssh and qemu is wired together. And finally, if we detect a cosa workdir we also bind it in by default. More to come here, such as auto-injecting debugging tools and containers.
Member
|
/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 |
Member
|
Man, Prow is having a bad day today. /override ci/prow/sanity |
|
@jlebon: Overrode contexts on behalf of jlebon: ci/prow/sanity DetailsIn response to this:
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. |
cgwalters
added a commit
to cgwalters/coreos-assembler
that referenced
this pull request
Apr 23, 2020
In coreos#1338 we fixed the problem of having one's terminal be corrupted by the serial console. This fixes the same problem for `cosa build` which ends up running a VM too. First, there's no real reason to dump the kernel bootup output to the terminal (or build logs) - it's totally uninteresting stuff that we only want to see if something fails. Gather the serial console to one log file, and the output of the *command* we run in the VM to another. Fork off a `tail` process to follow just the command output. (When we rewrite this in a better language we will do this properly async)
cgwalters
added a commit
to cgwalters/coreos-assembler
that referenced
this pull request
Apr 23, 2020
I ran out of space in `/tmp` today and it turned out I had a whole lot of `/tmp/mantle-qemu` files, including some really big disk images. This is a combination of changes from coreos#1338 and the multipath PR coreos#1296 Basically if we get interrupted by Ctrl-C which is a lot more likely now when qemu isn't owning the serial console from the start, we will leak our tmpfiles. We probably need to install a `SIGINT` handler but that's a whole mess that's avoided if we just let the kernel clean up our resources for us, which is what was happening before the multipath PR (well, at least for files and not the swtpm tmpdir, but that's small). And actually coreos#1380 does install a `SIGINT` handler so someone please review that and we can maybe fix this more after that lands.
cgwalters
added a commit
to cgwalters/coreos-assembler
that referenced
this pull request
Apr 24, 2020
I ran out of space in `/tmp` today and it turned out I had a whole lot of `/tmp/mantle-qemu` files, including some really big disk images. This is a combination of changes from coreos#1338 and the multipath PR coreos#1296 Basically if we get interrupted by Ctrl-C which is a lot more likely now when qemu isn't owning the serial console from the start, we will leak our tmpfiles. We probably need to install a `SIGINT` handler but that's a whole mess that's avoided if we just let the kernel clean up our resources for us, which is what was happening before the multipath PR (well, at least for files and not the swtpm tmpdir, but that's small). And actually coreos#1380 does install a `SIGINT` handler so someone please review that and we can maybe fix this more after that lands.
cgwalters
added a commit
to cgwalters/coreos-assembler
that referenced
this pull request
Apr 24, 2020
I ran out of space in `/tmp` today and it turned out I had a whole lot of `/tmp/mantle-qemu` files, including some really big disk images. This is a combination of changes from coreos#1338 and the multipath PR coreos#1296 Basically if we get interrupted by Ctrl-C which is a lot more likely now when qemu isn't owning the serial console from the start, we will leak our tmpfiles. We probably need to install a `SIGINT` handler but that's a whole mess that's avoided if we just let the kernel clean up our resources for us, which is what was happening before the multipath PR (well, at least for files and not the swtpm tmpdir, but that's small). And actually coreos#1380 does install a `SIGINT` handler so someone please review that and we can maybe fix this more after that lands.
openshift-merge-robot
pushed a commit
that referenced
this pull request
Apr 24, 2020
I ran out of space in `/tmp` today and it turned out I had a whole lot of `/tmp/mantle-qemu` files, including some really big disk images. This is a combination of changes from #1338 and the multipath PR #1296 Basically if we get interrupted by Ctrl-C which is a lot more likely now when qemu isn't owning the serial console from the start, we will leak our tmpfiles. We probably need to install a `SIGINT` handler but that's a whole mess that's avoided if we just let the kernel clean up our resources for us, which is what was happening before the multipath PR (well, at least for files and not the swtpm tmpdir, but that's small). And actually #1380 does install a `SIGINT` handler so someone please review that and we can maybe fix this more after that lands.
cgwalters
added a commit
to cgwalters/coreos-assembler
that referenced
this pull request
Apr 25, 2020
In coreos#1338 we fixed the problem of having one's terminal be corrupted by the serial console. This fixes the same problem for `cosa build` which ends up running a VM too. First, there's no real reason to dump the kernel bootup output to the terminal (or build logs) - it's totally uninteresting stuff that we only want to see if something fails. Gather the serial console to one log file, and the output of the *command* we run in the VM to another. Fork off a `tail` process to follow just the command output. (When we rewrite this in a better language we will do this properly async)
openshift-merge-robot
pushed a commit
that referenced
this pull request
Apr 27, 2020
In #1338 we fixed the problem of having one's terminal be corrupted by the serial console. This fixes the same problem for `cosa build` which ends up running a VM too. First, there's no real reason to dump the kernel bootup output to the terminal (or build logs) - it's totally uninteresting stuff that we only want to see if something fails. Gather the serial console to one log file, and the output of the *command* we run in the VM to another. Fork off a `tail` process to follow just the command output. (When we rewrite this in a better language we will do this properly async)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This finally unifies the advantages of
cosa runandkola spawn.I kept getting annoyed by how serial console sizing is broken
(e.g. trying to use
lessetc.). Usingsshviakola spawnaddresses that, but it means you can't debug the initramfs.
Now things work in an IMO pretty cool way; if you do e.g.
cosa run --kargs ignition.config.url=blah://(or inject a badIgnition config) to cause a failure in the initramfs,
you'll see a nice error (building on
coreos/ignition-dracut#146 ) telling you
to rerun with
cosa run --devshell-console.Things are also wired up cleanly so that we support rebooting
with the equivalent of
kola spawn --reconnect(which we shouldprobably remove now). You can exit via either quitting SSH
cleanly or using
poweroff, and the lifecycle of ssh and qemuis wired together.
And finally, if we detect a cosa workdir we also bind it in by
default.
More to come here, such as auto-injecting debugging
tools and containers.