CP-53477 Update API/CLI to Support Dom0 SSH Control #6385

LunfanZhang · 2025-03-24T07:55:01Z

This PR introduces support for Dom0 SSH control, providing the following capabilities:

Query the SSH status.
Configure a temporary SSH enable timeout for a specific host or all hosts in the pool.
Configure the console idle timeout for a specific host or all hosts in the pool.

Changes
New Host Object Fields:

ssh_enabled: Indicates whether SSH is enabled.
ssh_enabled_timeout: Specifies the timeout for temporary SSH enablement.
ssh_expiry: Tracks the expiration time for temporary SSH enablement.
console_idle_timeout: Configures the idle timeout for the console.

New Host/Pool APIs:

set_ssh_enable_timeout: Allows setting a temporary timeout for enabling the SSH service.
set_console_timeout: Allows configuring the console idle timeout.

…onfigure Add new host object fields: - ssh_enabled - ssh_enabled_timeout - ssh_expiry - console_idle_timeout Add new host/pool API to enable to set a temporary enabled SSH service timeout - set_ssh_enable_timeout Add new host/pool API to enable to set console timeout - set_console_timeout Signed-off-by: Lunfan Zhang <Lunfan.Zhang@cloud.com>

BengangY · 2025-03-24T10:28:42Z

All codes are in only one commit. I would suggest split them into at least 3 commits:

Datamodel define.
Testing code.
API.

robhoes · 2025-03-24T10:35:59Z

note: the PR has involved datamodel change which results the SDK build failed

That needs to be fixed then...

BengangY · 2025-03-24T10:44:23Z

ocaml/idl/datamodel_host.ml

+            ~default_value:(Some (VDateTime Date.epoch)) "ssh_expiry"
+            "The time when SSH access will expire"
+        ; field ~qualifier:RW ~lifecycle:[] ~ty:Int
+            ~default_value:(Some (VInt 0L)) "console_idle_timeout"


Why do ssh_enabled_timeout and console_idle_timeout have different unit?

From a user perspective, the SSH service timeout should be longer than the idle session timeout. Generally, users might set ssh_enabled_timeout to 30 minutes and console_idle_timeout to 600 seconds, as defined in the REQ. I assume this is reasonable.

BengangY · 2025-03-24T10:47:53Z

ocaml/idl/datamodel_host.ml

@@ -2368,6 +2368,29 @@ let disable_ssh =
    ~params:[(Ref _host, "self", "The host")]
    ~allowed_roles:_R_POOL_ADMIN ()

+let set_ssh_enable_timeout =
+  call ~name:"set_ssh_enable_timeout " ~lifecycle:[]


I think the SDK failure may be caused by the blank space after set_ssh_enable_timeout ...

lindig · 2025-03-24T10:38:29Z

ocaml/idl/datamodel_host.ml

@@ -2368,6 +2368,29 @@ let disable_ssh =
    ~params:[(Ref _host, "self", "The host")]
    ~allowed_roles:_R_POOL_ADMIN ()

+let set_ssh_enable_timeout =
+  call ~name:"set_ssh_enable_timeout " ~lifecycle:[]
+    ~doc:"Set the SSH service enabled timeout for the host"


Is this the enable or enabled timeout? There is a contradiciton between the documentation and the name of the function.

set_ssh_enabled_timeout

The original definition is set_ssh_enable_timeout, but it is conflict with the db setter for new fields ssh_enabled_timeout.
How about set_enabled_ssh_timeout or set_ssh_timeout?

lindig · 2025-03-24T10:39:13Z

ocaml/idl/datamodel_host.ml

+        (Ref _host, "self", "The host")
+      ; ( Int
+        , "timeout"
+        , "The SSH enabled timeout in minutes (0 means no timeout, max 2880)"


Most time values are seconds in the API. So this is likely to cause errors and there is no hint in the names that this is minutes.

Make sense, I will update to sec

lindig · 2025-03-24T10:40:06Z

ocaml/idl/datamodel_host.ml

+    ~params:
+      [
+        (Ref _host, "self", "The host")
+      ; (Int, "console_timeout", "The idle console timeout in seconds")


Case in point: here we use seconds.

lindig · 2025-03-24T10:41:33Z

ocaml/xapi-cli-server/records.ml

@@ -204,6 +204,20 @@ let get_pbds_host rpc session_id pbds =
 let get_sr_host rpc session_id record =
  get_pbds_host rpc session_id record.API.sR_PBDs

+let get_unified_field ~rpc ~session_id ~getter ~transform ~default =


What is the purpose of the function? Its semantics seem unusual. Please add documentation.

This function iterates over specific fields retrieved from all the hosts in a pool. If all the fields have the same value, it prints that value; otherwise, it prints the default value.

If this is about reporting, I believe the maximum should be reported. If this is about using a value, the minimum should be used. This is a security feature and I can't see the purpose of a default if values don't agree.

lindig · 2025-03-24T10:44:11Z

ocaml/xapi/xapi_host.ml

@@ -3112,22 +3114,167 @@ let emergency_clear_mandatory_guidance ~__context =
     ) ;
  Db.Host.set_pending_guidances ~__context ~self ~value:[]

+let validate_timeout timeout =
+  if timeout < 0L || timeout > 2880L then


The maximum is 48h. It would be better expressed as 48*60. But I question that such a high timeout is even sensible. As I said before - this is the API and would prefer to keep times to seconds. The XE interface could still accept minutes.

I discussed this with the architecture team and the PM, and we agreed to consider this limitation as acceptable for a weekend day.
and I agree to keep the time in seconds for better accuracy control, aligning with the general practices of XAPI.

lindig · 2025-03-24T10:44:54Z

ocaml/xapi/xapi_host.ml

+  let current_time = Date.now () in
+
+  let expiry_time =
+    let current_secs = Date.to_unix_time current_time in


I believe this should be expressed using time spans. @psafont

Sorry, I do not get the point, could you explain more?

We avoid time calculations as the one above and prefer to use Ptime - but @psafont is the expert.

lindig · 2025-03-24T10:52:41Z

ocaml/xapi/xapi_host.ml

+        Helpers.call_script "/bin/bash" ["-c"; script] |> Result.ok
+    | timeout ->
+        let script =
+          Printf.sprintf


I am not sure I like modifying a file by creating a shell script that does this - rather than doing it directly. What does the source /root/.bashrc when executed here achieve?

BengangY · 2025-03-24T10:58:41Z

ocaml/xapi/message_forwarding.ml

+
+      let set_ssh_enable_timeout ~__context ~self ~timeout =
+        let uuid = host_uuid ~__context self in
+        info "Host.set_ssh_enable_timeout : host = '%s'" uuid ;


Can use __FUNCTION__ to replace Host.set_ssh_enable_timeout.

Actually, for these ones in message_forwarding, please leave it as it, as it is meant to show the API call that was made (not the OCaml function). But please add the timeout value.

BengangY · 2025-03-24T11:16:11Z

ocaml/xapi/xapi_pool.ml

+    )
+    hosts
+
+let set_console_timeout ~__context ~self:_ ~console_timeout =


What happens if one of the hosts fails?

BengangY · 2025-03-24T11:41:28Z

ocaml/xapi/xapi_host.mli

+val set_console_timeout :
+  __context:Context.t -> self:API.ref_host -> console_timeout:int64 -> unit
+
+val schedule_disable_job :


schedule_disable_job is only used inside xapi_host, so it doesn't need to be added to mli.

It should be exposed, because we will use it in another PR.

BengangY · 2025-03-24T11:43:20Z

ocaml/xapi/xapi_host.ml

+        Db.Host.set_ssh_enabled ~__context ~self ~value:false ;
+        debug "Successfully disabled SSH for host %s" host_uuid
+      with e ->
+        error "Failed to disable SSH for host %s: %s" host_uuid


Should the exception be thrown out?

BengangY · 2025-03-24T11:46:29Z

ocaml/xapi/xapi_host.ml

+    | 0L ->
+        remove_disable_job ~__context ~self
+    | t ->
+        schedule_disable_job ~__context ~self ~timeout:(Int64.mul t 60L)


If enable and start are successful, but schedule_disable_job is failed, then the ssh service will always be on. This is not expected. How to handle this scenario?

There is no way to handle this because the schedule_disable_job is an asynchronous job.

BengangY · 2025-03-24T11:52:19Z

ocaml/xapi/xapi_host.ml

+  else
+    timeout
+
+let remove_disable_job ~__context ~self =


I would suggest renaming it to remove_disable_ssh_job.

BengangY · 2025-03-24T11:53:08Z

ocaml/xapi/xapi_host.ml

+  let task_name = Printf.sprintf "disable_ssh_for_host_%s" host_uuid in
+  Xapi_stdext_threads_scheduler.Scheduler.remove_from_queue task_name
+
+let schedule_disable_job ~__context ~self ~timeout =


I would suggest renaming it to schedule_disable_ssh_job.

BengangY · 2025-03-24T12:09:02Z

ocaml/xapi/xapi_host.ml

+        Db.Host.set_ssh_enabled_timeout ~__context ~self ~value:timeout
+  )
+
+let set_console_timeout ~__context ~self ~console_timeout =


Is this used for both "ssh session timeout" and "VNC session timeout"?

BengangY · 2025-03-24T12:22:13Z

ocaml/xapi/xapi_host.ml

+
+  let configure_timeout = function
+    | 0L ->
+        let script =


Could the script lines with 0L and timeout be combined into one line with a variable?

gangj · 2025-03-25T02:19:42Z

ocaml/xapi/xapi_host.ml

@@ -3112,22 +3114,167 @@ let emergency_clear_mandatory_guidance ~__context =
     ) ;
  Db.Host.set_pending_guidances ~__context ~self ~value:[]

+let validate_timeout timeout =


As it will not be used in other places and it is with a general name, pls make it an inner func of set_ssh_enable_timeout

gangj · 2025-03-25T02:38:49Z

ocaml/idl/datamodel_pool.ml

@@ -1571,6 +1571,33 @@ let disable_ssh =
    ~params:[(Ref _pool, "self", "The pool")]
    ~allowed_roles:_R_POOL_ADMIN ()

+let set_ssh_enable_timeout =
+  call ~name:"set_ssh_enable_timeout " ~lifecycle:[]


Remove the space in tail.

gangj · 2025-03-25T02:55:59Z

ocaml/xapi-cli-server/records.ml

@@ -204,6 +204,20 @@ let get_pbds_host rpc session_id pbds =
 let get_sr_host rpc session_id record =
  get_pbds_host rpc session_id record.API.sR_PBDs

+let get_unified_field ~rpc ~session_id ~getter ~transform ~default =


Would be better to change it to a more specified name, how about: get_common_value_from_all_hosts

gangj · 2025-03-25T02:57:12Z

ocaml/xapi-cli-server/records.ml

+      if List.for_all (( = ) first_value) all_values then
+        first_value
+      else
+        default


Print a log for the detailed differences in the hosts.

LunfanZhang · 2025-03-25T06:07:29Z

All codes are in only one commit. I would suggest split them into at least 3 commits

Hi @BengangY @lindig @gangj , Thanks for your comments above. to ensure rest (e.g., pool-join related) work is not blocked while waiting for the PR review, I will

split the data model changes into an independent PR and review data model first
convert the above PR into a draft one, before data model PR merged, I will use this PR and fix the comments about new added API
after data model PR is merged, I will close this PR and submit a other PR for new added API and CLI change.

robhoes · 2025-03-25T10:40:26Z

Close this for now, as this PR cannot be reviewed yet. PR #6388 needs to be reviewed first, and this PR conflicts with it.

BengangY self-requested a review March 24, 2025 10:35

BengangY reviewed Mar 24, 2025

View reviewed changes

lindig reviewed Mar 24, 2025

View reviewed changes

BengangY reviewed Mar 24, 2025

View reviewed changes

gangj reviewed Mar 25, 2025

View reviewed changes

LunfanZhang changed the title ~~CP-53477 Update Host/Pool Data model to Support Dom0 SSH Control~~ CP-53477 Update API/CLI to Support Dom0 SSH Control Mar 25, 2025

LunfanZhang marked this pull request as draft March 25, 2025 08:26

robhoes closed this Mar 25, 2025

CP-53477 Update API/CLI to Support Dom0 SSH Control #6385

CP-53477 Update API/CLI to Support Dom0 SSH Control #6385

Uh oh!

Conversation

LunfanZhang commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BengangY commented Mar 24, 2025

Uh oh!

robhoes commented Mar 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BengangY Mar 24, 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!

LunfanZhang Mar 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LunfanZhang Mar 25, 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!

BengangY Mar 24, 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!

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

LunfanZhang commented Mar 24, 2025 •

edited

Loading

BengangY Mar 24, 2025 •

edited

Loading

LunfanZhang Mar 25, 2025 •

edited

Loading

LunfanZhang Mar 25, 2025 •

edited

Loading

BengangY Mar 24, 2025 •

edited

Loading

LunfanZhang commented Mar 25, 2025 •

edited

Loading