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

Multiplexing of VNC and local TTY for the shim console #4593

Merged
merged 5 commits into from
Feb 14, 2025

Conversation

rouming
Copy link
Contributor

@rouming rouming commented Feb 6, 2025

Hi EVE, long time no see :)

This series implements the long-standing feature namely multiplexing of VNC and local TTY for a single shim console. Feature was discussed many times with @europaul (here is his attempt to do that #4163, but seems abandoned). My approach is different to what Paul started: instead of tweaking the chroot2.c starter, better to leverage this to QEMU itself. My patches to QEMU were recently taken upstream (https://patchew.org/QEMU/[email protected]/) and should appear in QEMU 10. Due to this I would like to finish this work in EVE, which I started a few months back.

There are a couple of changes in this series, namely:

  1. Rename 'prime-cons' to 'shim-cons', Hopefully nobody depends on the old naming.
  2. Use PTY chardev for QEMU console chardev devices, instead of socket, which simplifies wrappers around these tools.
  3. Backport of QEMU patches related to aggregator hub chardev (including one fix for the PTY device)
  4. Switch shim console to an aggregator hub device

In short, the new hub character device in QEMU combines input from several backend devices (VNC, tty, socket, etc.) into a single frontend console device (actual virtio-console guest device), so that the user can control a guest console from a remote VNC and a local TTY console.

The QEMU part was tested during the implementation, but with EVE I could test compilation only and straggle with container deployment (all zededa controller keys were expired, and back then it was my only way to start a container, to my shame). I would appreciate if someone from EVE can help me with running the stuff and testing the whole integration.

Also, I would like to join an lf-edge call, so can explain everything related to this series.

@OhmSpectator
Copy link
Member

Wow, it looks like we really are an open-source project with a lively community and great PRs from contributors! =D

@europaul europaul self-requested a review February 6, 2025 14:17
@rene
Copy link
Contributor

rene commented Feb 6, 2025

Thanks for this great contribution @rouming , good to see you here as well...We will test it and please, join our LF-Edge channel on slack so you can syncup on our lf-edge calls: https://lfedge.slack.com/archives/CHMEEC0MP

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Looks great!

Just to confirm, would a comprehensive test for this change be:

  1. Running an application
  2. Attaching to it using our eve attach-console command
  3. Running VNC to the application

I’d like to have a test scenario ready to verify it. Let me know if I’m missing anything.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to reflect the change of the name in the documentation as well:

In order to attach to the console of the hosting Vm of the Container application another console ID should be used which is named `prime-cons`:

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there are some references to prime-cons on this doc. file...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll change the doc accordingly.

@OhmSpectator
Copy link
Member

And yeah, the unit test still should be fixed to reflect the changes =)

@eriknordmark
Copy link
Contributor

There is an EVE call next Thursday. Should we put you on the agenda?

@naiming-zededa
Copy link
Contributor

naiming-zededa commented Feb 6, 2025

hi @rouming great to meet you here :-)
Just a question, w/ this patch, in order to access the docker console, the connect script is still needed, correct?
basically if the container supports shell, can i still use attach-console script for the connection to drop in the container inside?

Copy link
Contributor

@rene rene left a comment

Choose a reason for hiding this comment

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

@rouming , LGTM, pls, fix the tests and the comments... just minor stuff...

@europaul
Copy link
Contributor

europaul commented Feb 7, 2025

I ran this EVE version with a container app with the following DomainConfig

  "EnableVnc": true,
  "VncDisplay": 0,
  "VncPasswd": "",
  "CPUsPinned": false,
  "VMMMaxMem": 0,
  "EnableVncShimVM": false,

VNC to the container itself works well, but attaching to a serial console doesn't:

84639879-4d73-4439-b457-3f4b675ce3c4:~# eve list-app-consoles
PID	APP-UUID				CONS-TYPE	CONS-ID
---	--------				---------	---------
5357	734a45b9-0af4-416f-8ce5-64f5b1a8bc1e	VM		734a45b9-0af4-416f-8ce5-64f5b1a8bc1e.1.1/cons
84639879-4d73-4439-b457-3f4b675ce3c4:~# eve attach-app-console 734a45b9-0af4-416f-8ce5-64f5b1a8bc1e.1.1/cons
Error: console '734a45b9-0af4-416f-8ce5-64f5b1a8bc1e.1.1/cons' does not exist.
Try to use 'eve list-app-consoles'.

the shim console is missing and connection to the regular console fails.
Also there are no devices present where they should be

84639879-4d73-4439-b457-3f4b675ce3c4:~# l /run/hypervisor/kvm/734a45b9-0af4-416f
-8ce5-64f5b1a8bc1e.1.1/
total 4K     
drwxr-xr-x    2 root     root         100 Feb  7 10:12 .
drwxr-xr-x    3 root     root          60 Feb  7 10:12 ..
srwxr-xr-x    1 root     root           0 Feb  7 10:12 listener.qmp
-rw-------    1 root     root           5 Feb  7 10:12 pid
srwxr-xr-x    1 root     root           0 Feb  7 10:12 qmp

I think there is something wrong with the xen1.cfg, that's probably also why the unit tests fail. I'll take a closer look a little later.

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

There is an EVE call next Thursday. Should we put you on the agenda?

@eriknordmark Yes, please.

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

hi @rouming great to meet you here :-) Just a question, w/ this patch, in order to access the docker console, the connect script is still needed, correct? basically if the container supports shell, can i still use attach-console script for the connection to drop in the container inside?

@naiming-zededa hi Naiming, right, nothing changes on this matter. I replaced socket backend device with the pty, which simplifies the tooling in the eve script around the tio command, so you can directly attach to it. Other than this the change should just bring support of the "same" console to both VNC and local shim console.

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

@rouming , LGTM, pls, fix the tests and the comments... just minor stuff...

hey Rene, will do. I tried to fix unit tests blindly, but missed :)

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

I think there is something wrong with the xen1.cfg, that's probably also why the unit tests fail. I'll take a closer look a little later.

yes, easily can be, did the change blindly, unfortunately, due to missing container deployment. Thanks for running this.

If you folks can remind me how to use eden in order to start a random container, I will appreciate. Anyway this is not urgent. I'll play myself later.

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

Looks great!

Just to confirm, would a comprehensive test for this change be:

  1. Running an application
  2. Attaching to it using our eve attach-console command
  3. Running VNC to the application

I’d like to have a test scenario ready to verify it. Let me know if I’m missing anything.

Right, the 2. and 3. should have similar output, basically leading you to a same virtio console device.
Don't forget to press Ctrl-Alt-2 on the VNC window to switch the consoles in VNC itself.

@rene
Copy link
Contributor

rene commented Feb 7, 2025

Looks great!
Just to confirm, would a comprehensive test for this change be:

  1. Running an application
  2. Attaching to it using our eve attach-console command
  3. Running VNC to the application

I’d like to have a test scenario ready to verify it. Let me know if I’m missing anything.

Right, the 2. and 3. should have similar output, basically leading you to a same virtio console device. Don't forget to press Ctrl-Alt-2 on the VNC window to switch the consoles in VNC itself.

@rouming in order to test the container application, you can also use LF-Edge Sandbox: https://lfedge.org/resources/lf-edge-sandbox/

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

Difference to the previous version:

  • Fixed kvm unit tests
  • Reflect shim-cons name in doc
  • Backported latest PTY (chardev/char-pty.c) fixes, which caused the problem observed by @europaul , now cons and shim-cons should appear

@rene
Copy link
Contributor

rene commented Feb 7, 2025

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

@rouming , it looks like the new patches are broken: https://github.com/lf-edge/eve/actions/runs/13203300686/job/36860327193?pr=4593#step:9:826

Right, thanks, forgot to change the path to reflect qemu-xen, was testing them manually.

@rouming
Copy link
Contributor Author

rouming commented Feb 7, 2025

Difference to the previous version:

  • Changed paths in patch files: added "a/tools/qemu-xen/" prefix.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.90%. Comparing base (ba08639) to head (74e5268).
Report is 21 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4593   +/-   ##
=======================================
  Coverage   20.90%   20.90%           
=======================================
  Files          13       13           
  Lines        2894     2894           
=======================================
  Hits          605      605           
  Misses       2163     2163           
  Partials      126      126           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OhmSpectator
Copy link
Member

@europaul, ah, sorry, I thought it was a test attempt before the patches were fixed.

@rouming
Copy link
Contributor Author

rouming commented Feb 11, 2025

Folks, I will return to this. Please don´t merge.

@rouming rouming changed the title Multiplexing of VNC and local TTY for the shim console [WIP] Multiplexing of VNC and local TTY for the shim console Feb 11, 2025
@OhmSpectator OhmSpectator marked this pull request as draft February 11, 2025 12:25
@OhmSpectator
Copy link
Member

Folks, I will return to this. Please don´t merge.

Ok. Converted to draft.

@naiming-zededa
Copy link
Contributor

@europaul, have you retested the PR? If it works for you, I suggest merging.

@OhmSpectator last time I tried it I encountered the issues described above. And so did @naiming-zededa. We should have a closer look and test more

I have tried the latest patch last week, I only observed one 'cons' from the container app, I don't see the 'shim-cons' item.

The name 'prime' for a hosting Vm is non-standard name in the
industry. The established and familiar is 'shim'. Let's stick
to something which is common and rename 'prime-cons' to a
'shim-cons'.

Hopefully nobody depends on the old naming.

Signed-off-by: Roman Penyaev <[email protected]>
Just a few replacements.

Signed-off-by: Roman Penyaev <[email protected]>
This is a backport of QEMU upstreamed work, related to aggregator
`hub` chardev implementation.

The work implements a new chardev backend `hub` device, which
aggregates input from multiple backend devices and forwards it to a
single frontend device. Additionally, `hub` device takes the output
from the frontend device and sends it back to all the connected
backend devices. This allows for seamless interaction between
different backend devices and a single frontend interface.

The motivation is the EVE project [2], where it would be very
convenient to have a virtio console frontend device on the guest that
can be controlled from multiple backend devices, namely VNC and local
TTY emulator. The following is an example of the QEMU command line:

    -chardev pty,path=/tmp/pty,id=pty0 \
    -chardev vc,id=vc0 \
    -chardev hub,id=hub0,chardevs.0=pty0,chardevs.1=vc0 \
    -device virtconsole,chardev=hub0 \
    -vnc 0.0.0.0:0

Which creates two backend devices:

* Text virtual console (`vc0`)
* A pseudo TTY (`pty0`) connected to the single virtio hvc console with the
  help of a new backend aggregator (`hub0`)

`vc0` renders text to an image, which can be shared over the VNC
protocol.  `pty0` is a pseudo TTY backend which provides bidirectional
communication to the virtio hvc console.

Once QEMU starts, the VNC client and any TTY emulator can be used to
control a single hvc console. For example, these two different
consoles should have similar input and output due to the buffer
aggregation:

    # Start TTY emulator
    tio /tmp/pty

    # Start VNC client and switch to virtual console Ctrl-Alt-2
    vncviewer :0

Signed-off-by: Roman Penyaev <[email protected]>
…evice

Starting from QEMU 10 (patches already backported to EVE) there is
support of an aggregator hub chardev, which aggregates input from
multiple backend devices and forwards it to a single frontend device.

For a shim console we have a local pseudo TTY (so called shim-cons,
available on `eve list-app-consoles`) and a VNC (should be explicitly
enabled by the `debug.enable.vnc.shim.vm` flag). Input from both
chardevs will be aggregated with the `hub` help and forwarded to a
virtual console `hvc1`. This gives a user possibility to control
shim VM with one virtual console from several (pty and vnc)
backends.

Signed-off-by: Roman Penyaev <[email protected]>
@rouming rouming changed the title [WIP] Multiplexing of VNC and local TTY for the shim console Multiplexing of VNC and local TTY for the shim console Feb 13, 2025
@rouming rouming marked this pull request as ready for review February 13, 2025 08:14
@github-actions github-actions bot requested a review from rene February 13, 2025 08:14
@rouming
Copy link
Contributor Author

rouming commented Feb 13, 2025

Difference to the previous version:

  • Rebased on latest master
  • Drop pty-instead-of-socket simplification. Apparently char-pty.c does not work on the qemu v8, even with backported fixes. I did not investigate why, but the behavior is odd and could not be reproduced on vanilla qemu v10 (latest master). This is minor, not required for the whole PR, I just wanted to kill stupid invocation of socat, which pumps data from socket to pty device and back. Anyway, this is dropped.
  • This PR was tested on eden with this change: Add --vnc-for-shim-vm eden pod parameter eden#1051, which lets you deploy a container with enabled vnc-for-shim-vm, so this command:
./eden pod deploy -n eclient1 --memory=512MB docker://lfedge/eden-eclient:b1c1de6 -p 2223:22 --vnc-display 0 --vnc-for-shim-vm

Thanks @europaul for this list of eden commands, completely forgot how to run this beast. If you, guys, would like to test on your side, don't forget to switch the VNC main console to shim console, with Alt-Ctrl-2.

I noticed, that VNC shim console has nasty escape symbols at the end of each prompt. This is not due to this patch, but presumably because of the incorrect rendering of escape symbols inside the ui/console-vc.c (like vc_chr_write()).

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run tests

@OhmSpectator
Copy link
Member

OhmSpectator commented Feb 13, 2025

@eriknordmark, we still need confirmation from proper manual testing.

@naiming-zededa
Copy link
Contributor

@OhmSpectator i have tried the latest patch, and i can use the VNC to switch between the two consoles, with my 'realVNC' client

@rouming
Copy link
Contributor Author

rouming commented Feb 13, 2025

@OhmSpectator i have tried the latest patch, and i can use the VNC to switch between the two consoles, with my 'realVNC' client

@naiming-zededa That's great. While using VNC on the second console, please do eve attach-app-console shim-cons, you should see exactly the same data. Both consoles (vnc and a local one) accept input and share same output.

@naiming-zededa
Copy link
Contributor

@OhmSpectator i have tried the latest patch, and i can use the VNC to switch between the two consoles, with my 'realVNC' client

@naiming-zededa That's great. While using VNC on the second console, please do eve attach-app-console shim-cons, you should see exactly the same data. Both consoles (vnc and a local one) accept input and share same output.

yes, @rouming i saw that, both VNC's 2nd console and the attach-app-console to the shim-cons, see the same input and output of each other.

@rouming
Copy link
Contributor Author

rouming commented Feb 14, 2025

yes, @rouming i saw that, both VNC's 2nd console and the attach-app-console to the shim-cons, see the same input and output of each other.

Thanks, @naiming-zededa for verifying that.

@OhmSpectator OhmSpectator merged commit fcd1bff into lf-edge:master Feb 14, 2025
58 of 61 checks passed
@europaul
Copy link
Contributor

europaul commented Feb 17, 2025

@rouming I tested the patch using the aforementioned eden setup:

  • VNC works well and shim-cons as well
  • but cons doesn't work for me - I can send commands but get no replies. This problem goes away, when VNC is disabled. I remember seeing this problem before, but thought I would be fixed with a patch like this. Is it out of scope or is it a bug?

@rouming
Copy link
Contributor Author

rouming commented Feb 17, 2025

@rouming I tested the patch using the aforementioned eden setup:

  • VNC works well and shim-cons as well

@europaul Cool, thanks for testing.

  • but cons doesn't work for me - I can send commands but get no replies. This problem goes away, when VNC is disabled. I remember seeing this problem before, but thought I would be fixed with a patch like this. Is it out of scope or is it a bug?

VNC and "cons" occupy different console devices. If no VNC is set, then "cons" is attached to the container stdin/stdout. If you start VNC, then VNC is attached to the container stdin/stdout, and "cons" occupy another console device, which is dangling and not used. I see this behavior on the latest master. Did you mean something different?

btw, did you notice nasty terminal sequence ^[[nn;mmR] at the end of the prompt in the VNC for the second console (Alt-Ctrl-2)? There is a bug in qemu in the vc terminal emulation: instead of sending cursor position response (CPR) to the application, the CPR is rendered in the VNC window.

@europaul
Copy link
Contributor

VNC and "cons" occupy different console devices. If no VNC is set, then "cons" is attached to the container stdin/stdout. If you start VNC, then VNC is attached to the container stdin/stdout, and "cons" occupy another console device, which is dangling and not used. I see this behavior on the latest master. Did you mean something different?

So if we just switch to that other console device, then cons should also work with VNC enabled? That would be cool!

btw, did you notice nasty terminal sequence ^[[nn;mmR] at the end of the prompt in the VNC for the second console (Alt-Ctrl-2)? There is a bug in qemu in the vc terminal emulation: instead of sending cursor position response (CPR) to the application, the CPR is rendered in the VNC window.

Yes, I noticed this. Good to know the reason!

@rouming
Copy link
Contributor Author

rouming commented Feb 17, 2025

So if we just switch to that other console device, then cons should also work with VNC enabled? That would be cool!

It turns out to be that logic is quite hairy. When you start a container with the VNC enabled, there is the following branch:

./pkg/pillar/cmd/zedmanager/handledomainmgr.go:
   if dc.IsOCIContainer() {
		if dc.EnableVnc {
			dc.ExtraArgs += " console=tty0"
        }

which adds the console=tty0 to the command line of a shim VM.

So there are two cases: VNC enabled and VNC is disabled. For these cases the cmdline is different:

  • shim VM with VNC: console=hvc0 console=tty0
  • shim VM without VNC: console=hvc0

Then there is the following code:

./pkg/xen-tools/initrd/init-initrd:

if grep -q "console=tty0" /proc/cmdline; then
  #shellcheck disable=SC2086
  #we have tty0 console primary, so will add output to hvc0 for logging
  eval /chroot2 /mnt/rootfs "${WORKDIR:-/}" $ug $pid_file $cmd 2>&1 | tee -i /dev/hvc0
else
  #shellcheck disable=SC2086
  eval /chroot2 /mnt/rootfs "${WORKDIR:-/}" $ug $pid_file $cmd <> /dev/console 2>&1
fi

For the VNC case tty0 is a control terminal (which is a VNC session), but output also forwarded to the /dev/hvc0, which is a "cons" socket. Output is only forwarded and "cons" does not control anything, it is "read-only".

For non VNC case there is full interaction with the /dev/console because of this <> /dev/console, and according to the cmdline the only console: console=hvc0

To summarize, once VNC is enabled, it becomes the control terminal and "cons" becomes just an stdout for a started container. Wanna fix this? :)

Update
One observation. "cons" chardev is created as a mux device :

from /run/domainmgr/xen/xenN.cfg:

[chardev "charserial0"]
  backend = "socket"
  mux = "on"
  path = "/run/hypervisor/kvm/86caf1ef-3bf6-4321-a1a9-4ba52897b589.1.1/cons"
  server = "on"
  wait = "off"
  logfile = "/dev/fd/1"
  logappend = "on"

so it can accept input/output from several consoles. There is /dev/hvc0 declaration:

[device]
  driver = "virtconsole"
  chardev = "charserial0"
  name = "org.lfedge.eve.console.0

and also qemu cmdline itself contains a serial device:

ps x | grep qemu
/usr/lib/xen/bin/qemu-system-x86_64  ... -serial chardev:charserial0 ...

so /dev/hvc0 and /dev/ttyS0 both attached to "cons" chardev.

Easy to check. From the shim-cons:

~ # echo a > /dev/hvc0
~ # echo b > /dev/ttyS0

On the cons end you should see 'a' and 'b'. Seems hvc0 is not needed, this can be combed too, looks quite hairy.

@naiming-zededa
Copy link
Contributor

@rouming @OhmSpectator @europaul @rene throw more into this topic :-)
I have the code w/ edgeview to remotely using the cons/shim-cons of 'tio' on laptop

@rouming rouming deleted the backend-hub branch February 17, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants