Skip to content

Conversation

@cgwalters
Copy link
Member

This is a followup to the previous progress status for
#1367

Rather than trying to parse the serial console, just print it as
is, except strip control characters/newlines so that it becomes
effectively a progress bar. This way, we don't corrupt the terminal,
and we don't fill up its history either.

And, go a step farther and dump the whole serial console in the
case where we get Ctrl-C before a successful login. This aids
in debugging.

@jlebon
Copy link
Member

jlebon commented Apr 22, 2020

Tried this out, though the output is a bit messy because it seems like some control characters still get through or something and we get a mix of some newline characters and some not?

I'm pretty happy with cosa run -c for now, so I'm good with just giving the status quo a try for some time more to see how it feels. (The "dump on Ctrl-C" thing is really neat though; could we split that out into a separate patch?)

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 23, 2020
Previously if one had just `cosa build ostree` then
`cosa run`, qemu just stalls out confused.

(But this would be a bit better with
 coreos#1380
 so someone review that please!)
openshift-merge-robot pushed a commit that referenced this pull request Apr 24, 2020
Previously if one had just `cosa build ostree` then
`cosa run`, qemu just stalls out confused.

(But this would be a bit better with
 #1380
 so someone review that please!)
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.
This is a followup to the previous progress status for
coreos#1367

Rather than trying to parse the serial console, just print it as
is, except strip control characters/newlines so that it becomes
effectively a progress bar.  This way, we don't corrupt the terminal,
and we don't fill up its history either.

And, go a step farther and dump the whole serial console in the
case where we get Ctrl-C before a successful login.  This aids
in debugging.
@cgwalters cgwalters force-pushed the qemu-progress-console branch from 3b6ee7e to c2145ab Compare April 25, 2020 13:26
@cgwalters
Copy link
Member Author

Tried this out, though the output is a bit messy because it seems like some control characters still get through or something and we get a mix of some newline characters and some not?

OK, fixed that.

(The "dump on Ctrl-C" thing is really neat though; could we split that out into a separate patch?)

They're a bit entangled in this patch; seemed simpler to fix the bug.

@jlebon
Copy link
Member

jlebon commented Apr 27, 2020

/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

Details 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 dd883aa into coreos:master Apr 27, 2020
@cgwalters
Copy link
Member Author

Ugh, I swear I fixed the "grub prompt spewed to console" but apparently not...

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.

4 participants