Skip to content

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

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

Merged

Conversation

gangj
Copy link
Contributor

@gangj gangj commented Apr 2, 2025

After being ejected from a pool, a new host obj will be created with default settings in DB.

This commit configures SSH service in the ejected host to default state during pool eject.

@@ -2045,6 +2045,10 @@ let eject_self ~__context ~host =
control_domains_to_destroy
with _ -> ()
) ;
(* Restore SSH service to its default state: enabled with no timeout *)
Xapi_host.set_ssh_enabled_timeout ~__context ~self:host ~value:0L ;
Xapi_host.enable_ssh ~__context ~self:host ;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's your consideration here about the exceptions that host ssh operations may raise?

@robhoes
Copy link
Member

robhoes commented Apr 3, 2025

This implicitly encodes the default value inside the implementation of pool.eject. If we are ever changing the default, we will probably forget this. We should rely on defaults set in a single place.

Also, we must wrap this with an exception handler to ensure we don't fail the eject if this goes wrong.

@gangj gangj force-pushed the private/gangj/CP-53802 branch 2 times, most recently from c062060 to 741b7a0 Compare April 7, 2025 08:14
@@ -3051,7 +3052,8 @@ let t =
"The time in UTC after which the SSH access will be automatically \
disabled"
; field ~qualifier:DynamicRO ~lifecycle:[] ~ty:Int
~default_value:(Some (VInt 0L)) "console_idle_timeout"
~default_value:(Some (VInt Constants.default_console_idle_timeout))
"console_idle_timeout"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you replace them all? I remembered there are some default values to be set in last merged PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

In #6395, xapi_host.ml, dbsync_slave.ml ...
Also suggest splitting to a single commit for the default value

@gangj gangj force-pushed the private/gangj/CP-53802 branch from 741b7a0 to 9adf643 Compare April 7, 2025 09:25
gangj added 2 commits April 7, 2025 17:31
After being ejected from a pool, a new host obj will be created with
default settings in DB.

This commit configures SSH service in the ejected host to default state
during pool eject.

Signed-off-by: Gang Ji <[email protected]>
@gangj gangj force-pushed the private/gangj/CP-53802 branch from 9adf643 to 0bc2246 Compare April 7, 2025 09:32
@gangj gangj requested a review from robhoes April 7, 2025 12:32
Copy link
Contributor

@changlei-li changlei-li left a comment

Choose a reason for hiding this comment

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

Still not sure if Constants is the best place to define the default values, but no better suggestion now.

@gangj
Copy link
Contributor Author

gangj commented Apr 8, 2025

Still not sure if Constants is the best place to define the default values, but no better suggestion now.

I think Constants is the place for constants which do not need to be configured.

@gangj gangj merged commit 25ed999 into xapi-project:feature/configure-ssh-phase2 Apr 8, 2025
17 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.

3 participants