CP-53711: Copy SSH settings from pool coordinator in pool join #6395

gangj · 2025-04-01T03:00:36Z

No description provided.

BengangY · 2025-04-01T03:27:03Z

ocaml/idl/datamodel_pool.ml

@@ -1286,14 +1294,14 @@ let sync_updates =
          param_type= String
        ; param_name= "username"
        ; param_doc= "The username of the remote pool"
-        ; param_release= numbered_release "25.6.0-next"
+        ; param_release= numbered_release "25.7.0"


Should we update these release versions when the feature branch is ready to be merged into the master branch?

This is a fixup for merged code, not from the feature branch.

I think it's easy to ignore changing the next to release version. Please remember do it when merging to master. BTW, there is gen_lifecycle to check datamodel_lifecycle.ml, see here. I think similar methods can be used for datamodel_pool and datamodel_host too. It can be considered in the future.

BengangY · 2025-04-01T03:40:09Z

"...will be dropped sson."
A spelling mistake in the description of the second commit.

lindig · 2025-04-01T09:54:31Z

ocaml/xapi/xapi_pool.ml

+        Client.Host.get_console_idle_timeout ~rpc ~session_id
+          ~self:remote_coordinator
+      in
+      (* Configure SSH service parameters in local DB to setup local SSH


"to set up" ("setup" is a noun but you need a verb here)

lindig · 2025-04-01T09:57:40Z

ocaml/xapi/xapi_pool.ml

+         out yet, the joiner will start SSH service with timeout
+         host.ssh_enabled_timeout, which means SSH service in the joiner will
+         be disabled later than in the new coordinator. *)
+      let ssh_expiry = Db.Host.get_ssh_expiry ~__context ~self:host_ref in


The joining host is going through a reboot. What is the general policy for SSH over a reboot? If SSH was enabled before reboot, what is the state after reboot?

The joining host will not reboot, only xapi will have a restart to finish the joining.
While a host will go through a reboot when it is ejected from a pool. And I think in current design, SSH service status will be kept after reboot:

If SSH service is enabled without timeout or disabled, it will not change after reboot.

If SSH service is being enabled with a timeout, after reboot, it will be enabled with remaining timeout, and be disabled after time is up.

So the current policy is to sync the same status, including ssh_expiry, from the pool coordinator to the newly joined host. It relies on the reboot mechanism to ensure SSH is disabled eventually. Essentially, the newly joined host will follow the same process as the pool coordinator: reboot, check if the expiry time is greater than the current time, and trigger a disable.

So the current policy is to sync the same status, including ssh_expiry, from the pool coordinator to the newly joined host.

No, ssh_expiry will be now + ssh_enabled_timeout for the new joined host as we discussed offline, pls check the code.

Add "Changed" records for 2 APIs which were missed. Fix "param_release" for 3 added parameters. Signed-off-by: Gang Ji <[email protected]>

gangj · 2025-04-02T03:35:18Z

Force push to re-base to latest feature branch, which merged the latest master to fix the build failure.

changlei-li · 2025-04-02T07:52:46Z

ocaml/idl/datamodel_pool.ml

@@ -1286,14 +1294,14 @@ let sync_updates =
          param_type= String
        ; param_name= "username"
        ; param_doc= "The username of the remote pool"
-        ; param_release= numbered_release "25.6.0-next"
+        ; param_release= numbered_release "25.7.0"


I think it's easy to ignore changing the next to release version. Please remember do it when merging to master. BTW, there is gen_lifecycle to check datamodel_lifecycle.ml, see here. I think similar methods can be used for datamodel_pool and datamodel_host too. It can be considered in the future.

ocaml/xapi/xapi_pool.ml

BengangY · 2025-04-02T08:31:30Z

ocaml/tests/common/test_common.ml

@@ -170,13 +170,16 @@ let make_host ~__context ?(uuid = make_uuid ()) ?(name_label = "host")
    ?(external_auth_service_name = "") ?(external_auth_configuration = [])
    ?(license_params = []) ?(edition = "free") ?(license_server = [])
    ?(local_cache_sr = Ref.null) ?(chipset_info = []) ?(ssl_legacy = false)
-    ?(last_software_update = Date.epoch) ?(last_update_hash = "") () =
+    ?(last_software_update = Date.epoch) ?(last_update_hash = "")


Why do we add these parameters to make_host? I don't see them been used.

They are used below: in line 181 and 182.

BengangY · 2025-04-02T08:53:02Z

ocaml/xapi/xapi_pool.ml

+      Db.Host.set_ssh_enabled ~__context ~self:host_ref ~value:ssh_enabled ;
+      Db.Host.set_ssh_enabled_timeout ~__context ~self:host_ref
+        ~value:ssh_enabled_timeout ;
+      Db.Host.set_console_idle_timeout ~__context ~self:host_ref


set_console_idle_timeout also updates /root/.bashrc. Should here call the API host.set_console_idle_timeout instead of updating DB directly?

Yes, it is clear with API call, changed to that.

BengangY · 2025-04-02T08:55:23Z

ocaml/xapi/xapi_pool.ml

+        Client.Host.get_ssh_enabled ~rpc ~session_id ~self:remote_coordinator
+      in
+      let ssh_enabled_timeout =
+        Client.Host.get_ssh_enabled_timeout ~rpc ~session_id


Will Client.Host.get_ssh_enabled_timeout send API call to the remote coordinator? If so, it will send 4 times API calls. Could it just send one time API call to fetch all these 4 parameters?

I think we can't do that without querying all the records. Or how?

Could Client.Host.get_record work?

That will query all the records of the host, I think it is expensive.

During pool join, create a new host obj in the remote pool coordinator DB with the same SSH settings as pool coordinator. Also configure SSH service locally before xapi restart which will persist after xapi restart. Signed-off-by: Gang Ji <[email protected]>

robhoes · 2025-04-02T13:38:27Z

ocaml/xapi/xapi_pool.ml

+      ( match ssh_enabled with
+      | true ->
+          Xapi_host.enable_ssh ~__context ~self:host_ref
+      | false ->
+          Xapi_host.disable_ssh ~__context ~self:host_ref
+      ) ;


This doesn't really belong here in this create_or_get_host_on_master function, where the purpose is to create a host record for joining host on the in pool's DB. It would be better to factor out these additional side effects to keep the logic clean(er).

From L958 actually.

BengangY reviewed Apr 1, 2025

View reviewed changes

gangj force-pushed the private/gangj/CP-53711 branch from bbfe5a4 to 01fe4e3 Compare April 1, 2025 04:36

lindig approved these changes Apr 1, 2025

View reviewed changes

Fixup

db38cb0

Add "Changed" records for 2 APIs which were missed. Fix "param_release" for 3 added parameters. Signed-off-by: Gang Ji <[email protected]>

gangj force-pushed the private/gangj/CP-53711 branch from 01fe4e3 to bd6c586 Compare April 2, 2025 03:33

gangj force-pushed the private/gangj/CP-53711 branch from bd6c586 to a09a2ed Compare April 2, 2025 03:43

changlei-li reviewed Apr 2, 2025

View reviewed changes

BengangY reviewed Apr 2, 2025

View reviewed changes

changlei-li approved these changes Apr 2, 2025

View reviewed changes

gangj force-pushed the private/gangj/CP-53711 branch from a09a2ed to 30bd072 Compare April 2, 2025 09:18

gangj force-pushed the private/gangj/CP-53711 branch from 30bd072 to a875364 Compare April 2, 2025 09:21

BengangY approved these changes Apr 2, 2025

View reviewed changes

gangj merged commit 6e6c0ed into xapi-project:feature/configure-ssh-phase2 Apr 2, 2025
17 checks passed

robhoes reviewed Apr 2, 2025

View reviewed changes

gangj mentioned this pull request Apr 7, 2025

CP-53711: Apply SSH settings in joiner before update_non_vm_metadata #6408

Merged

changlei-li mentioned this pull request Apr 7, 2025

CP-53802: Restore SSH service to default state in pool eject #6399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-53711: Copy SSH settings from pool coordinator in pool join #6395

CP-53711: Copy SSH settings from pool coordinator in pool join #6395

gangj commented Apr 1, 2025 •

edited

Loading

BengangY Apr 1, 2025

gangj Apr 1, 2025

changlei-li Apr 2, 2025

BengangY commented Apr 1, 2025

lindig Apr 1, 2025

lindig Apr 1, 2025

gangj Apr 2, 2025 •

edited

Loading

LunfanZhang Apr 2, 2025

gangj Apr 2, 2025 •

edited

Loading

gangj commented Apr 2, 2025

changlei-li Apr 2, 2025

BengangY Apr 2, 2025

gangj Apr 2, 2025 •

edited

Loading

BengangY Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025

BengangY Apr 2, 2025

gangj Apr 2, 2025

BengangY Apr 2, 2025

gangj Apr 2, 2025 •

edited

Loading

robhoes Apr 2, 2025

robhoes Apr 2, 2025

gangj Apr 7, 2025

CP-53711: Copy SSH settings from pool coordinator in pool join #6395

CP-53711: Copy SSH settings from pool coordinator in pool join #6395

Conversation

gangj commented Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BengangY commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangj Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangj Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

gangj commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangj Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

BengangY Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangj Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangj commented Apr 1, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading

BengangY Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading