CP-52074: Add API for start/stop systemd service sshd #6198
Conversation
ocaml/idl/datamodel_host.ml
Outdated
| ~params: | ||
| [ | ||
| (Ref _host, "self", "The host") | ||
| ; (ssh_status, "status", "Status of sshd service") |
There was a problem hiding this comment.
I would recommend to have 2 calls, one to turn it on, and one to turn it off. This is because enums tend to be problematic for upgrades, so I'd rather avoid them
There was a problem hiding this comment.
Given we are later going to have an auto mode, I would like to use a single API with different parameters (e.g. on, off, auto, etc). Then we only need to add 1 API instead of adding 3 APIs.
There was a problem hiding this comment.
Given we are later going to have an auto mode
This doesn't look like it's a small one off, but something that needs its design reviewed.
Please add the design to xapi's documentation so we can take an honest look at this code.
There was a problem hiding this comment.
I think this all looks reasonable. I don't think a detailed design would be required but some motivation and rationale would be good to have. If we already know that we want additional enums, it might be good to provide them on the API level already but to return an error like "not supported" for them. This would make update cases in the future simpler.
There was a problem hiding this comment.
Discussed the design today, we decided to move the 'auto' mode to another API. So the new added API only contains starting and stopping ssh. So I splitted the API to start_ssh and stop_ssh.
BengangY
force-pushed
the
private/bengangy/CP-52074
branch
2 times, most recently
from
96270dd to
36a05a1
Compare
ocaml/idl/datamodel_pool.ml
Outdated
| @@ -61,6 +61,15 @@ let telemetry_frequency = | |||
| ] | |||
| ) | |||
|
|
|||
| let ssh_status = | |||
There was a problem hiding this comment.
If we don't need more than on/off a simpler call ...-enable taking a boolean would suffice. But if we do need more, an enum could be the right choice. We probably should define all planned values right away and avoid extending the enum later, if possible.
BengangY
force-pushed
the
private/bengangy/CP-52074
branch
from
36a05a1 to
5b9f737
Compare
|
Maybe it's not needed but there is no API to observe the status of the SSH service; to be sure, you have to enable it before using it, |
BengangY
force-pushed
the
private/bengangy/CP-52074
branch
from
5b9f737 to
27f8377
Compare
BengangY
force-pushed
the
private/bengangy/CP-52074
branch
from
27f8377 to
3d25716
Compare
Signed-off-by: Bengang Yuan <[email protected]>
Signed-off-by: Bengang Yuan <[email protected]>
Signed-off-by: Bengang Yuan <[email protected]>
BengangY
force-pushed
the
private/bengangy/CP-52074
branch
from
3d25716 to
77005e9
Compare
| @@ -121,15 +121,19 @@ let stop ~service = | |||
| Xapi_stdext_unix.Unixext.unlink_safe destination ; | |||
| status | |||
|
|
|||
| let is_active ~service = | |||
| let check_service_status ~command ~service = | |||
There was a problem hiding this comment.
The function returns a boolean; the previous is_active name reflected this better. Maybe just call it status? This would look like status ~command:"is-enabled" ~service at a call site. It's a minor concern, though.
| @@ -1048,6 +1048,24 @@ let rec cmdtable_data : (string * cmd_spec) list = | |||
| ; flags= [Host_selectors] | |||
| } | |||
| ) | |||
| ; ( "host-start-ssh" | |||
There was a problem hiding this comment.
Would host-ssh-start/stop be the better name? This follows the hierarchical structure we use.
| @@ -3105,6 +3123,24 @@ let rec cmdtable_data : (string * cmd_spec) list = | |||
| ; flags= [] | |||
| } | |||
| ) | |||
| ; ( "pool-start-ssh" | |||
This PR is to add API and CLI which is used to enable and disable SSH on all hosts in a pool or on specified hosts within a pool.