Skip to content

Conversation

stephenchengCloud
Copy link
Contributor

  • First commit:
    Introduces a new pool-level parameter that restricts VNC console access
    to a single active session per VM/host.
    This prevents multiple users from simultaneously connecting to the same VM console,
    preventing one user 'watching' another user operating a session.

  • Second commit:
    When the limit_console_sessions is true.

    • Enforced a single active VNC console connection per VM/host
    • Disable connection to websocket

Tested:

  • [root@eu1-dt094 bin]# xe pool-param-get uuid=de76d8c9-2e1b-f868-39fe-5768bf8c3c0d param-name=limit-console-sessions
    true
    Only one console can be connected:
image
  • [root@eu1-dt094 bin]# xe pool-param-get uuid=de76d8c9-2e1b-f868-39fe-5768bf8c3c0d param-name=limit-console-sessions
    false
    Both consoles can be connected:
image

@stephenchengCloud stephenchengCloud changed the base branch from master to feature/limit-vnc-console September 2, 2025 11:09
@stephenchengCloud stephenchengCloud force-pushed the private/stephenche/CP-54217 branch from a6d9ab5 to df2787b Compare September 3, 2025 03:11
@stephenchengCloud stephenchengCloud force-pushed the private/stephenche/CP-54217 branch from df2787b to aa00d9e Compare September 3, 2025 03:36
Copy link
Contributor

@gangj gangj left a comment

Choose a reason for hiding this comment

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

Please rebase to update the commits before merge, thank you

@stephenchengCloud stephenchengCloud force-pushed the private/stephenche/CP-54217 branch 4 times, most recently from ca81f54 to 881a2f7 Compare September 3, 2025 06:56
@stephenchengCloud
Copy link
Contributor Author

Squashed the commits.

@gangj
Copy link
Contributor

gangj commented Sep 3, 2025

Remove duplicated sign-off in commit message.
I think you can add the 2 CP ticket number in the commit message title, or keep them splitted in 2 commits.

This change introduces a new pool-level parameter that restricts VNC console access
to a single active session per VM/host.
This prevents multiple users from simultaneously connecting to the same VM console,
preventing one user 'watching' another user operating a session.
When the `limit_console_sessions` is true.
- Enforced a single active VNC console connection per VM/host
- Disable connection to websocket

Signed-off-by: Stephen Cheng <[email protected]>
@stephenchengCloud stephenchengCloud force-pushed the private/stephenche/CP-54217 branch from 881a2f7 to ee86ee9 Compare September 3, 2025 08:01
Signed-off-by: Stephen Cheng <[email protected]>
@stephenchengCloud
Copy link
Contributor Author

Fixed comments and tested.

active_connections := VMSet.remove vm_id !active_connections
)

let try_add vm_id =
Copy link
Member

Choose a reason for hiding this comment

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

try_add and remove are too specific for the internal implementation.
How about try_acquire and release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we use map instead of set to record the connection count, it seems try_add is better.
Changed remove to drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a Hashtbl which is stateful that does not require the update-and-assign? But I'm fine using Map because it is most similar to Set and code changes will be smaller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Hashtbl and Map are both fine. I considered Hashtbl. The reason I chose Map is that Map is immutable, which is more Ocaml style.

@minglumlu
Copy link
Member

Hi @robhoes
Could you please look at this?

Signed-off-by: Stephen Cheng <[email protected]>
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.

5 participants