Skip to content

mantle/qemu: explicitly add -serial mon:stdio #1737

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 3 commits into from
Sep 23, 2020
Merged

mantle/qemu: explicitly add -serial mon:stdio #1737

openshift-merge-robot merged 3 commits into from
Sep 23, 2020

Conversation

@jlebon
Copy link
Member

@jlebon jlebon commented Sep 22, 2020

This restores access to the monitor via Ctrl-A C and allows passing
through Ctrl-C to the guest as before.

This used to be what QEMU did by default, but when we added an explicit
QMP socket in #1705 it disabled that sugar.

Closes: #1725

@jlebon
Copy link
Member Author

jlebon commented Sep 22, 2020 

Running: rpm-ostree compose tree --repo=/srv/tmp/repo --cachedir=/srv/cache --touch-if-changed /srv/tmp/treecompose.changed --unified-core /srv/src/config/manifest.yaml --download-only --ex-lockfile=/srv/src/config/manifest-lock.x86_64.json --ex-lockfile=/srv/src/config/manifest-lock.overrides.x86_64.yaml --ex-lockfile-strict
Formatting 'cache2.qcow2.tmp', fmt=qcow2 size=10737418240 cluster_size=65536 lazy_refcounts=off refcount_bits=16
qemu-system-x86_64: -serial mon:stdio: cannot use stdio by multiple character devices
qemu-system-x86_64: -serial mon:stdio: could not connect serial device to character backend 'mon:stdio'

Ahh right OK, this is conflicting with runvm also doing -serial file. Hmm, kola already knows how to do this. Seems like we should add a --redirect-serial-to-file type switch instead that runvm can use.

cgwalters
cgwalters approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I was confused by the commit title and description, maybe:

mantle/qemu: Use mon:stdio if not in devshell mode

or something.

@jlebon
kola/qemu: transform ConsoleToFile() to ConsoleFile field
9197f56 
That way, the builder will be able to just inspect that field to know if
the serial console is being redirected.

Prep for next patch.
@jlebon
mantle/qemu: default to -serial mon:stdio if not redirecting
87ad35f 
This restores access to the monitor via `Ctrl-A C` and allows passing
through `Ctrl-C` to the guest as before.

This used to be what QEMU did by default, but when we added an explicit
QMP socket in coreos#1705 it disabled that sugar.

Closes: coreos#1725
@jlebon
kola/qemuexec: add --console-to-file
1d128b8 
We want kola to be in control of what happens to the serial console. But
`runvm` wants to redirect the console to a file.

Add a flag to `qemuexec` to make that possible.
@jlebon
Copy link
Member Author

jlebon commented Sep 23, 2020

OK, reworked this now!

cgwalters
cgwalters approved these changes Sep 23, 2020
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

/lgtm

mantle/cmd/kola/qemuexec.go
cmdQemuExec.Flags().StringArrayVar(&bindrw, "bind-rw", nil, "Same as above, but writable")
cmdQemuExec.Flags().BoolVarP(&forceConfigInjection, "inject-ignition", "", false, "Force injecting Ignition config using guestfs")
cmdQemuExec.Flags().BoolVar(&propagateInitramfsFailure, "propagate-initramfs-failure", false, "Error out if the system fails in the initramfs")
cmdQemuExec.Flags().StringVarP(&consoleFile, "console-to-file", "", "", "Filepath in which to save serial console logs")
Copy link
Member

@cgwalters cgwalters Sep 23, 2020

Choose a reason for hiding this comment

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

"Save serial console to this file" maybe? "in which" is better than ending in "in" (which ends in "in" (which...))

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 23, 2020

[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 2851c65 into coreos:master Sep 23, 2020
@jlebon jlebon deleted the pr/qemu-qmp branch April 22, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgwalters cgwalters cgwalters approved these changes

@ashcrow ashcrow Awaiting requested review from ashcrow

@mike-nguyen mike-nguyen Awaiting requested review from mike-nguyen

Assignees

@cgwalters cgwalters

Labels

approved lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ctrl-C not passing through to qemuexec

4 participants

@jlebon @openshift-ci-robot @cgwalters @openshift-merge-robot