Skip to content

cmdlib: let qemu write command output on its stdout#1403

Merged
openshift-merge-robot merged 2 commits intocoreos:masterfrom
jlebon:pr/cosa-cmdout
Apr 27, 2020
Merged

cmdlib: let qemu write command output on its stdout#1403
openshift-merge-robot merged 2 commits intocoreos:masterfrom
jlebon:pr/cosa-cmdout

Conversation

@jlebon
Copy link
Copy Markdown
Member

@jlebon jlebon commented Apr 27, 2020

Instead of using tail -qF, just write the output to a virtio-serial
port and tell qemu to output that on its stdout. That way, we don't have
to worry about cleaning up the tail process at all.

This also fixes an issues we've been seeing where tail is killed
before it had a chance to print the entire command output, which
crucially might omit the final error message.

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 27, 2020

The second commit here cleans up the output of runvm even more!

@cgwalters
Copy link
Copy Markdown
Member

Oh that is just a super cool approach.
/approve

@cgwalters
Copy link
Copy Markdown
Member

@jlebon needs to pacify shellcheck

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 27, 2020

Done!

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 27, 2020

Hmm weird, looks like cosa fetch --strict is just hanging in CI. Works locally with FORCE_UNPRIVILEGED=1, though might have to do with Jenkins not allocating a TTY? Investigating.

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 27, 2020

/hold

@jlebon jlebon force-pushed the pr/cosa-cmdout branch 4 times, most recently from 2a9105f to e44ffb7 Compare April 27, 2020 20:16
@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 27, 2020

OK, I think I got this working now!

Note we close stdin to qemu here otherwise it seems like it just waits
forever for input to be written into the virtio port.

/hold cancel

Comment thread src/cmdlib.sh Outdated
jlebon added 2 commits April 27, 2020 17:12
Instead of using `tail -qF`, just write the output to a virtio-serial
port and tell qemu to output that on its stdout. That way, we don't have
to worry about cleaning up the `tail` process at all.

This also fixes an issues we've been seeing where `tail` is killed
before it had a chance to print the entire command output, which
crucially might omit the final error message.

Note we close stdin to qemu here otherwise it seems like it just waits
forever for input to be written into the virtio port.
Otherwise it always prints a bunch of warnings about getting `EPERM` and
the distro using security by obscurity (which while informative, isn't
something we're realistically going to address ourselves in the
short-term -- and IIRC there are already one or two RHBZs out about
that).
@cgwalters
Copy link
Copy Markdown
Member

/lgtm

Copy link
Copy Markdown
Contributor

@darkmuggle darkmuggle left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread src/cmdlib.sh
"${srcvirtfs[@]}" -append "root=/dev/vda console=${DEFAULT_TERMINAL} selinux=1 enforcing=0 autorelabel=1" \
"${qemu_args[@]}"
kill %1
"${qemu_args[@]}" <&-; then # the <&- here closes stdin otherwise qemu waits forever
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Neat, and thank you for the comment :)

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, darkmuggle, 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:
  • OWNERS [cgwalters,darkmuggle,jlebon]

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 bf75ff7 into coreos:master Apr 27, 2020
@jlebon jlebon deleted the pr/cosa-cmdout branch July 6, 2020 20:32
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.

5 participants