CP-54393: VNC console idle timeout #6692

stephenchengCloud · 2025-09-30T02:35:30Z

This commit adds idle timeout feature for vnc console connections.

Key changes:

Add idle timeout detection by monitoring RFB keyEvent and pointerEvent.
Add callback function to proxy to parse the RFB messages and determine if the connection is idle or not.

This commit adds idle timeout feature for vnc console connections. Key changes: - Add idle timeout detection by monitoring RFB keyEvent and pointerEvent. - Add callback function to `proxy` to parse the RFB messages and determine if the connection is idle or not. Signed-off-by: Stephen Cheng <[email protected]>

stephenchengCloud · 2025-09-30T02:38:31Z

This is a re-open PR of #6667 based on the rfb parser.
Tested:
Set idletimeout = 60s

Sep 26 07:23:22 eu1-dt094 xapi: [debug||621 HTTPS 10.70.0.164->:::80|Connection to VM console R:b967a8c7f944|console] Connected; running proxy (between fds: 55 a        nd 56)
Sep 26 07:23:22 eu1-dt094 xenopsd-xc: [debug||7 ||device] Got QMP event, domain-20: VNC_CONNECTED
…

Sep 26 07:24:28 eu1-dt094 xapi: [debug||621 HTTPS 10.70.0.164->:::80|Connection to VM console R:b967a8c7f944|console] Idle timeout exceeded! (timeout: 60.0s)
Sep 26 07:24:28 eu1-dt094 xapi: [debug||621 HTTPS 10.70.0.164->:::80|Connection to VM console R:b967a8c7f944|console] Proxy exited
Sep 26 07:24:28 eu1-dt094 xenopsd-xc: [debug||7 ||device] Got QMP event, domain-20: VNC_DISCONNECTED

lindig · 2025-09-30T08:47:51Z

ocaml/libs/xapi-stdext/lib/xapi-stdext-unix/unixext.ml

+exception Close_proxy
+
+let proxy ?(should_close : (bytes * int * int -> bool) option = None)
+    ?(poll_timeout = -1) (a : Unix.file_descr) (b : Unix.file_descr) =


The parameter poll_timeout is optional. Should we not need special values to indicate that we have no timeout?

This seems to be a consequence of the interface of polly, which expects a -1 for "no timeout".

However, you can define should_close without option = None (as the ? makes it an option type already). Then you don't need to use Some when you call the function in console.ml.

Yes. This should be a parameter with a default but it should not be an optional value.

proxy is not only used by console. I made poll_timeout as optional because I don't want to modify the code that calls proxy in other places before.

ocaml/xapi/console.ml

lindig · 2025-09-30T08:52:21Z

ocaml/xapi/xapi_globs.ml

+(* The timeout (in seconds) for event polling in the proxy loop.
+   If set to a positive value, the poll will wake up periodically,
+   which is useful for implementing features like idle timeout or periodic inspection of proxy buffers. *)
+let poll_timeout_sec = ref 5


Should this not be a float value?

This timeout value is for polly.wait_fold. It should be an int.
Check:
https://github.com/lindig/polly/blob/master/lib/polly.mli#L127

The expected value at the Polly API is an int in milliseconds - so we could take a float in seconds and convert it to milliseconds when we pass it to Polly.

This should be an Mtime.Span.t, also the current name doesn't make it clear it's for a proxy

Suggested change

let poll_timeout_sec = ref 5

let proxy_poll_period_timeout = ref Mtime.Span.(5 * s)

lindig · 2025-09-30T08:52:51Z

ocaml/xapi/xapi_globs.ml

    )
+  ; ( "poll_timeout_sec"
+    , Arg.Set_int poll_timeout_sec
+    , (fun () -> string_of_int !poll_timeout_sec)


I would use a float.

robhoes · 2025-10-07T09:55:03Z

ocaml/libs/xapi-stdext/lib/xapi-stdext-unix/unixext.ml

+exception Close_proxy
+
+let proxy ?(should_close : (bytes * int * int -> bool) option = None)
+    ?(poll_timeout = -1) (a : Unix.file_descr) (b : Unix.file_descr) =


This seems to be a consequence of the interface of polly, which expects a -1 for "no timeout".

robhoes · 2025-10-07T13:06:30Z

ocaml/libs/xapi-stdext/lib/xapi-stdext-unix/unixext.ml

+              match should_close with
+              | Some cb ->
+                  let buf, read_len, offset = CBuf.read b' b in
+                  if cb (buf, read_len, offset) then
+                    close_proxy ()
+              | None ->
+                  ignore (CBuf.read b' b)


This shorter version should work too:

let data = CBuf.read b' b in Option.iter (fun cb -> if cb data then close_proxy () ) should_close

robhoes · 2025-10-07T13:11:08Z

ocaml/libs/xapi-stdext/lib/xapi-stdext-unix/unixext.ml

+      ( match should_close with
+      | Some cb ->
+          if cb (Bytes.empty, 0, 0) then
+            close_proxy ()
+      | None ->
+          ()


Option.iter (fun cb -> if cb (Bytes.empty, 0, 0) then close_proxy () ) should_close

robhoes · 2025-10-07T13:15:13Z

ocaml/libs/xapi-stdext/lib/xapi-stdext-unix/unixext.ml

+              | None ->
+                  ignore (CBuf.read b' b)
+      ) ;
+      ( match should_close with


I guess this is to handle the case where polly returned after a timeout? Could it then go in an else branch?

Modified the code to call the callback only once by providing a init value(Bytes.empty, 0, 0) to Polly.wait_fold

robhoes · 2025-10-07T13:33:49Z

ocaml/xapi/console.ml

+  if !Xapi_globs.poll_timeout_sec < 0 then
+    -1
+  else
+    !Xapi_globs.poll_timeout_sec * 1000 (* convert to milliseconds *)


Or:

max (-1) (!Xapi_globs.poll_timeout_sec * 1000) (* convert to milliseconds *)

Now I changed poll_timeout_sec to a float type. The max (-1) ... can not work, say poll_timeout_sec is set to -0.00001 (though it's unlikely to set like this).
So I changed the logic back to the original one.

ocaml/xapi/console.ml

robhoes · 2025-10-07T13:56:15Z

ocaml/xapi/console.ml

+  let get_console_idle_timeout_value ~__context ~vm =
+    try
+      let idle_timeout =
+        if Db.VM.get_is_control_domain ~__context ~self:vm then


Looks like this triggers several DB calls every 5 seconds for every console. If the VM is running on a pool member, then that means several calls to the coordinator. I understand that we want to be responsive to timeout changes, with we should reduce this to at most one DB call.
Several of these values are static for given VM and can be captured in the closure before the line with fun (buf, read_len, offset) -> below.

Yes. Good point.

robhoes · 2025-10-07T13:56:40Z

ocaml/xapi/console.ml

+          Db.Pool.get_vm_console_idle_timeout ~__context ~self:pool
+      in
+      idle_timeout
+    with _ -> 0L (* Default: no timeout *)


Make it an option please.

robhoes · 2025-10-07T14:09:43Z

ocaml/xapi/console.ml

+          false
+      | Some parser -> (
+        try
+          let data = Bytes.sub_string buf offset read_len in


Note that this still creates a copy. I had hoped that we could avoid that and pass buf+offset+len straight to the parser. Perhaps not worth doing, but I can't remember how the parser processes this.

The parser calls Angstrom.Buffered.feed which needs a string type.
So there must be a copy.

Unfortunately, not even with the unsafe functions the copy can be avoided, as Bytes.subfamily of functions always allocates

robhoes · 2025-10-07T14:12:30Z

ocaml/xapi/console.ml

+                Int64.to_float (get_console_idle_timeout_value ~__context ~vm)
+              in
+              if is_idle_timeout ~idle_timeout_seconds ~last_activity then (
+                debug "Idle timeout exceeded! (timeout: %.1fs)"


Would be handy to say that this is about a console connection and for which VM.

lindig · 2025-10-09T08:53:39Z

ocaml/libs/xapi-stdext/lib/xapi-stdext-unix/unixext.ml

+exception Close_proxy
+
+let proxy ?(should_close : (bytes * int * int -> bool) option = None)
+    ?(poll_timeout = -1) (a : Unix.file_descr) (b : Unix.file_descr) =


Yes. This should be a parameter with a default but it should not be an optional value.

lindig · 2025-10-09T08:57:46Z

ocaml/xapi/xapi_globs.ml

+(* The timeout (in seconds) for event polling in the proxy loop.
+   If set to a positive value, the poll will wake up periodically,
+   which is useful for implementing features like idle timeout or periodic inspection of proxy buffers. *)
+let poll_timeout_sec = ref 5


The expected value at the Polly API is an int in milliseconds - so we could take a float in seconds and convert it to milliseconds when we pass it to Polly.

psafont · 2025-10-09T09:22:32Z

ocaml/xapi/xapi_globs.ml

+  ; ( "poll_timeout_sec"
+    , Arg.Set_int poll_timeout_sec
+    , (fun () -> string_of_int !poll_timeout_sec)


This should be defined as part of the xapi_globs_spec_with_descriptions list, in line 1259:

; ( "proxy_poll_period_timeout" , ShortDurationFromSeconds proxy_poll_period_timeout , "Timeout (in seconds) for event polling in network proxy loops. When \ positive, the proxy will wake up periodically to check tasks like vnc \ idle timeouts or perform other maintenance tasks. Set to -1 to wait \ indefinitely for network events without periodic wake-ups." )

use ShortDurationFromSeconds to have the conf value as float, or LongDurationFromSeconds for int. Because polly accepts milliseconds and the polling loop is bound to timeout in a matter of seconds at most, I recommend the float.

Thanks. I didn't know this before. But I checked the code, it seems ShortDurationFromSeconds cannot be negative, while epoll can take -1 as input. So I'll use float as Lindig suggested, and it'll be simpler to covert it to milliseconds.

val s_to_span : float -> Mtime.Span.t option (** [s_to_span seconds] converts a float representing seconds to a timespan. Returns None when [seconds] is negative, is not a number or larger than ~104 days. Avoid whenever possible, some RPC function already use this so it needs to be available. *)

Signed-off-by: Stephen Cheng <[email protected]>

minglumlu · 2025-10-10T03:32:34Z

ocaml/xapi/console.ml

+                last_activity := Mtime_clock.counter () ;
+
+              (* The idle_timeout config may change during running the callback,
+                 so need to get the latest *)


I think the callback function is performance critical. We may have to give up on this. Specifically, I think we need to avoid any DB operations within this callback.

OK. It's a design choice. In that way, the timeout config change only applys to the new connections.
I can add this to the design doc.

If we need it to apply to current connections, then another option is to have a separate thread watch the DB field using event.from and set a local ref that all console proxies can use. Then DB calls are outside of the critical path. But even simpler to not do it at all...

Yes, better to not do it.
If users want the idle timeout config applys to old connections, it's easy to reconnect the connections.

minglumlu · 2025-10-10T03:33:14Z

ocaml/xapi/console.ml

+          match parser data with
+          | Ok messages ->
+              if is_active messages then
+                last_activity := Mtime_clock.counter () ;


It's unlikely that it would get timed out after this.

You are right. We should not call timed_out function if is_active is true.

ocaml/xapi/console.ml

minglumlu · 2025-10-10T03:36:00Z

ocaml/xapi/console.ml

+              debug "RFB parse error: %s" error_msg ;
+              rfb_parser := None ;
+              false
+        with e ->


Will the parsing raise any exception? Will the result return have covered all the error cases? It is the point of result.

Checking the parser code, it'll return Error when parsing FAIL.
I'll remove the exception.

ocaml/xapi/console.ml

Signed-off-by: Stephen Cheng <[email protected]>

minglumlu · 2025-10-10T10:42:07Z

ocaml/xapi/console.ml

+          let pool = Helpers.get_pool ~__context in
+          Db.Pool.get_vm_console_idle_timeout ~__context ~self:pool
+      in
+      Some idle_timeout


It could be

if idle_timeout > 0. then Some (Int64.to_float idle_timeout) else None

Updated and tested

minglumlu · 2025-10-10T11:19:56Z

ocaml/xapi/console.ml

+      messages
+
+  let timed_out ~idle_timeout_seconds ~last_activity =
+    idle_timeout_seconds > 0.


This checking is not needed anymore.

Signed-off-by: Stephen Cheng <[email protected]>

minglumlu · 2025-10-10T11:24:30Z

Hello, @stephenchengCloud
Could you please have a test for the performance? For example, for the case that several consoles keep refreshing the display, like scrolling the screen.
It would be good to compare the performance between with and without the change.

stephenchengCloud · 2025-10-10T11:25:54Z

Hello, @stephenchengCloud Could you please have a test for the performance? For example, for the case that several consoles keep refreshing the display, like scrolling the screen. It would be good to compare the performance between with and without the change.

Will do.

lindig reviewed Sep 30, 2025

View reviewed changes

robhoes reviewed Oct 7, 2025

View reviewed changes

lindig approved these changes Oct 9, 2025

View reviewed changes

psafont reviewed Oct 9, 2025

View reviewed changes

Stephen Cheng added 2 commits October 9, 2025 19:28

fix

ba36573

Signed-off-by: Stephen Cheng <[email protected]>

Changed poll_timeout to float

d71b6a3

Signed-off-by: Stephen Cheng <[email protected]>

minglumlu reviewed Oct 10, 2025

View reviewed changes

ocaml/xapi/console.ml Outdated Show resolved Hide resolved

minglumlu reviewed Oct 10, 2025

View reviewed changes

ocaml/xapi/console.ml Show resolved Hide resolved

Refine create_idle_timeout_callback

9860a69

Signed-off-by: Stephen Cheng <[email protected]>

minglumlu reviewed Oct 10, 2025

View reviewed changes

minglumlu approved these changes Oct 10, 2025

View reviewed changes

fix2

b238556

Signed-off-by: Stephen Cheng <[email protected]>

stephenchengCloud force-pushed the private/stephenche/CP-54393 branch from 0e57657 to b238556 Compare October 10, 2025 11:23

	let poll_timeout_sec = ref 5
	let proxy_poll_period_timeout = ref Mtime.Span.(5 * s)

CP-54393: VNC console idle timeout #6692

Are you sure you want to change the base?

CP-54393: VNC console idle timeout #6692

Uh oh!

Conversation

stephenchengCloud commented Sep 30, 2025

Uh oh!

stephenchengCloud commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psafont Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenchengCloud Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psafont Oct 9, 2025 •

edited

Loading

stephenchengCloud Oct 10, 2025 •

edited

Loading

minglumlu Oct 10, 2025 •

edited

Loading

minglumlu Oct 10, 2025 •

edited

Loading