Skip to content

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

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

Conversation

gangj
Copy link
Contributor

@gangj gangj commented Apr 7, 2025

This is a follow up for #6395 (comment)

Useless here, the local DB will be dropped soon as the joinner will
switch to the remote DB of the new coordinator.

And latest_synced_updates_applied will be set to `unknown in host.create
in remote DB as default value.

Signed-off-by: Gang Ji <[email protected]>
@@ -1688,8 +1678,33 @@ let join_common ~__context ~master_address ~master_username ~master_password
"Unable to set the write the new pool certificates to the disk : %s"
(ExnHelper.string_of_exn e)
) ;
Db.Host.set_latest_synced_updates_applied ~__context ~self:me
~value:`unknown ;
let ssh_enabled_timeout =
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR moved ssh operations from create_or_get_host_on_master to join_common. I'm not familiar with the logic. Could you describe the difference in detail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are working for the pool join scenario, so join_common is where we should change. create_or_get_host_on_master is also called in join_common, while as it's name suggests, it is to create or get a host on the remote master, while the code we add here is related to the joiner, so we move the code out.

@gangj gangj force-pushed the private/gangj/CP-53711.2 branch from a106fae to abd6703 Compare April 7, 2025 10:27
@gangj gangj requested a review from robhoes April 7, 2025 12:31
in
match ssh_enabled with
| true ->
Xapi_host.enable_ssh ~__context ~self:me
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not highlighting that we have two API functions where one taking an argument would have been more convenient? I am not asking for a change but the API leads to verbose clients.

will persisit after it joins the new pool.
Please note that ssh_enabled_timeout needs be set before
update_non_vm_metadata(), inside which ssh_expiry is queried to
set to host obj in remote coordinator DB.
Copy link
Member

Choose a reason for hiding this comment

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

So now that we are inheriting the remote coordinator's ssh settings here, and write them to the local DB, we don't need to copy them from the remote again in L946–957?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see, we can query from local in L946–957!

@gangj gangj force-pushed the private/gangj/CP-53711.2 branch from abd6703 to d349779 Compare April 7, 2025 14:12
let ssh_enabled =
Client.Host.get_ssh_enabled ~rpc ~session_id ~self:remote_coordinator
in
let ssh_enabled = Db.Host.get_ssh_enabled ~__context ~self:host_ref in
Copy link
Member

@robhoes robhoes Apr 7, 2025

Choose a reason for hiding this comment

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

You can just use host.API.host_ssh_enabled etc below, as you already have the host record here.

@gangj gangj force-pushed the private/gangj/CP-53711.2 branch from d349779 to 5a533f5 Compare April 7, 2025 15:28
@gangj gangj merged commit b70078f into xapi-project:feature/configure-ssh-phase2 Apr 8, 2025
17 of 21 checks passed
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.

4 participants