Skip to content
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

Horustctl #200

Closed
wants to merge 11 commits into from
Closed

Horustctl #200

wants to merge 11 commits into from

Conversation

FedericoPonzi
Copy link
Owner

@FedericoPonzi FedericoPonzi commented Aug 23, 2023

Motivation and Context

Because Horust is designed to be run inside containers, one could handle the lifecycle by for example restarting the docker container. In some cases, instead of restarting the whole container, one would want to restart a single service - or inspect its status etc. This PR will add support for horustctl, a command line that will allow to interact with horust.
Issue: #31

Description

I've reorganized the project to have three crates. Horust and horustctl are the two binaries. The interactions is accomplished using Unix Domain Sockets (UDS) and using Protobuf for the message definition. The Commands crate implements the required code and testing to use the uds.

On startup, Horust will create a new socket (and its folder if doesn't exists) in /var/run/horust/horust-<pid>.sock. This will allow using multiple horust instances on the same machine. The folder is configurable on both server and client.

On client side, if no pid is specified horustctl will automatically search the folder and pick the only socket. If more than one socket is available, the command will fail.

I've decided to use protobuf3 for the definition of the messages, it turns out is quite verbose at least in rust, because all the messages are optional by default. If anyone wants to implement their own horustctl / client, they can reuse the protobuf.

I've considered adding tokio, but it's not worth bringing the runtime in just to handle this part of the program. There are also few interactions expected with this api, so I don't foresee performance issues.

The interactions are one shot: send a request, close the write socket, read the response. At least for now I don't have use case in mind that would require further interactions on the same connection.

I've also included the generated messages.rs module just to make browsing easier, I guess it's not a best practice to include generated files, but I don't see drawbacks with this.

Design

For now, the status works but It's still missing the update part. I'm thinking of merging the current changeset, and create a new pr to add the update part which seems a complicated change by itself.

For the status, now it just reports the status of the services. But I'm wondering if more info could be added.
Like:

<pid> - path/to/socket/horust-pid.sock
services-path: /services/path /services/path /services/path
start time: 
services: 
x - pid - running
y - pid - running
z - pid - terminated

Again, I'm thinking of merging this changeset and create a new pr.

How Has This Been Tested?

in one terminal:

./target/debug/horust -- bash

in another terminal:

./target/debug/horustctl status bash
[2023-08-24T09:59:45Z DEBUG horustctl] args: HourstctlArgs { pid: None, sockets_folder_path: "/var/run/horust/", socket_path: None, commands: Status(StatusArgs { service_name: Some("bash") }) }
[2023-08-24T09:59:45Z DEBUG horustctl] Socket filename: "/var/run/horust/hourst-2586329.sock"
[2023-08-24T09:59:45Z DEBUG horustctl] Status command received: StatusArgs { service_name: Some("bash") }
[2023-08-24T09:59:45Z DEBUG horustctl] uds path : "/var/run/horust/hourst-2586329.sock"
[2023-08-24T09:59:45Z DEBUG horust_commands_lib] Sending message: HorustMsgMessage { message_type: Some(Request(HorustMsgRequest { request: Some(StatusRequest(HorustMsgServiceStatusRequest { service_name: "bash" })) })) }
[2023-08-24T09:59:46Z DEBUG horust_commands_lib] Received message: HorustMsgMessage { message_type: Some(Response(HorustMsgResponse { response: Some(StatusResponse(HorustMsgServiceStatusResponse { service_name: "bash", service_status: Running })) })) }
[2023-08-24T09:59:46Z INFO  horust_commands_lib::client] Client: received: HorustMsgMessage { message_type: Some(Response(HorustMsgResponse { response: Some(StatusResponse(HorustMsgServiceStatusResponse { service_name: "bash", service_status: Running })) })) }
Current status for 'bash' is: 'RUNNING'.

I've also added relevant integration tests.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Documentation (a change to man pages or other documentation)

Todo:

  • Update the documentation accordingly.
  • Added unit/integration tests to cover all changes.
  • Update CI to build horustctl (install protobuf compiler)
  • Bump hourst version and create release (+publish to crates.io)

@FedericoPonzi FedericoPonzi force-pushed the horustctl branch 4 times, most recently from 6cdbbff to 640f413 Compare August 23, 2023 20:31
@FedericoPonzi FedericoPonzi force-pushed the horustctl branch 2 times, most recently from 48a6270 to df1041e Compare September 28, 2023 19:12
@vjeson
Copy link

vjeson commented Oct 27, 2024

When can the pull request be merged? Apart from the feature, I would like to restart one of the services, not all of them.

@eirnym
Copy link

eirnym commented Oct 28, 2024

I have a suggestion to take refactoring such as splitting to smaller modules and making more cleaner out of this PR.

Also plan on commands to support and make tool modular would be a nice idea:

  • Others would be informed about process
  • Parts could be added by a single PRs, not with one massive rework
  • Others could participate if they would like to.

As a critique of snipped you proposed, I'd like to have 2 status commands: one as in systemdctl, other is just for status without verbose output by default (which will be used by me in most of the cases).

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