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

runtime-rs: Log qemu's stderr in shim log #8938

Merged
merged 3 commits into from Jan 31, 2024

Conversation

pmores
Copy link
Contributor

@pmores pmores commented Jan 27, 2024

Having a ready access to qemu's stderr is immensely helpful in debugging qemu's launch failures. Qemu's stderr output is now included in regular shim's log (look for lines prepended with "qemu stderr: ").

An unrelated small fix promised in review of PR #8185 is also included in this PR's initial commit.

This fixes a flaw pointed out in review of PR kata-containers#8185.  Creation of the
directory semantically fits better into VM preparation than VM launch.

Signed-off-by: Pavel Mores <pmores@redhat.com>
@katacontainersbot katacontainersbot added the size/medium Average sized task label Jan 27, 2024
@pmores pmores requested review from Apokleos and gkurz January 27, 2024 13:44
@zvonkok
Copy link
Contributor

zvonkok commented Jan 29, 2024

Just a nit, can we prefix PRs that target runtime-rs with runtime-rs: ... whatever I was like, huh, didn't (at)gkurz already added that to go-runtime QEMU .. 👍🏼

@pmores
Copy link
Contributor Author

pmores commented Jan 29, 2024

I'm sorry, I thought the prefix only applies to commit messages and didn't realise that it applies to PR titles as well. Next time around (I don't think I can fix it for this one)!

@gkurz gkurz changed the title Log qemu's stderr in shim log runtime-rs: Log qemu's stderr in shim log Jan 29, 2024
@gkurz
Copy link
Member

gkurz commented Jan 29, 2024

I'm sorry, I thought the prefix only applies to commit messages and didn't realise that it applies to PR titles as well. Next time around (I don't think I can fix it for this one)!

Yes you can. There's an Edit button at the far right of the PR title. I've just done it.

We'll want to capture qemu's stderr in parallel with normal runtime-rs
execution.  Tokio's primitives make this much easier than std's.  This
also makes child process management more consistent across runtime-rs
(i.e. virtiofsd child process is already launched and managed using tokio).

Some changes were necessary due to tokio functions being slightly different
from their std counterparts.  Child::kill() is now async and Child::id()
now returns an Option.

Signed-off-by: Pavel Mores <pmores@redhat.com>
Qemu stderr monitoring runs in its own asynchronous green thread.
For that, `stderr` is taken out of the Child representing the qemu child
process to avoid partial move and make it possible for the main thread
still to call functions on QemuInner::qemu_process (e.g. kill(), id()).

Fixes kata-containers#8937

Signed-off-by: Pavel Mores <pmores@redhat.com>
@pmores pmores force-pushed the log-qemus-stderr-in-shim-log branch from 40be3ad to d53edbd Compare January 30, 2024 08:12
Copy link
Member

@gkurz gkurz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @pmores !

@gkurz
Copy link
Member

gkurz commented Jan 31, 2024

/test

@gkurz gkurz merged commit 8b1dc06 into kata-containers:main Jan 31, 2024
283 of 297 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test size/medium Average sized task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants