Add automatic shutdown after inactivity #1590
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgjm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Martin Michaelis <[email protected]>
|
|
||
| use tokio::sync::{futures::Notified, Notify}; | ||
|
|
||
| /// Track activity and reacto to inactivity. |
There was a problem hiding this comment.
| /// Track activity and reacto to inactivity. | |
| /// Track activity and react to inactivity. |
| value_name("DELAY"), | ||
| )] | ||
| /// Automatically stop conmon-rs after DELAY seconds of inactivity. (0 = disabled) | ||
| shutdown_delay: f64, |
There was a problem hiding this comment.
I think we should make this something like Option<humantime::Duration>
There was a problem hiding this comment.
Hmm... the string representation of a go time.Duration is not compatible with the humantime::Duration parser. But we could write a custom "to string" method in go. Do we expect the conmon server to be invoked by humans or only by the go client?
There was a problem hiding this comment.
IMO we should only support go client
There was a problem hiding this comment.
And even for humans it is still relatively ergonomic to specify the duration in (fractional) seconds.
1.5 vs 1s 500ms
Especially considering that I expect the most common durations to be in the range of a few full seconds.
There was a problem hiding this comment.
Is specifying milliseconds really a use case? I'm also fine with using seconds only.
| @@ -95,6 +95,11 @@ type ConmonServerConfig struct { | |||
|
|
|||
| // Tracing can be used to enable OpenTelemetry tracing. | |||
| Tracing *Tracing | |||
|
|
|||
| // ShutdownDelay an be used to automatically stop the conmonrs server after DELAY seconds of inactivity. | |||
There was a problem hiding this comment.
Can we document what inactivity means? Like, which actions are considering conmon-rs to be active?
There was a problem hiding this comment.
+1 to what inactivity means.
What use cases is this feature trying to solve?
|
I just realised that the go client does not prevent the server from shutting itself down if no containers are running. The go client creates a new connection for each invoked method. Therefore in between method calls no connection is kept open and the server could shut itself down if no containers are running. A This won't happen with podman (podman itself is short lived) and CRI-O manages the conmon lifetime itself (does not need/use the automatic shutdown). But nevertheless I would expect that the Is there a reason we open a new connection for each RPC call? If not, the solution would be open a connection in the |
I think the main intention was for simplicity. If the connection dies then the client would have to create a new one. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This adds a new option to enable automatic shutdown: After a configurable period of inactivity the server automatically shuts itself down.
The following activity is tracked and prevents shutdown:
ReapableChildof the child reaper (= a running container)The new FD socket server also shuts itself down after 3 seconds of inactivity (= no connected clients).
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
The FD socket inactivity period is hardcoded as 3 seconds. This is enough time for the client to connect to the FD socket path, after the client invoked the
StartFdSocketRPC method. This could also be configurable.Does this PR introduce a user-facing change?