feat: support periodic ping to principal for keeping connection alive #288
Conversation
jparsai
requested review from
jannfis,
jgwest,
ishitasequeira and
chetan-rns
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
==========================================
- Coverage 49.65% 49.30% -0.35%
==========================================
Files 73 74 +1
Lines 6404 6449 +45
==========================================
Hits 3180 3180
- Misses 2959 3002 +43
- Partials 265 267 +2 ☔ View full report in Codecov by Sentry. |
|
gRPC has support for configuring keepalives. Do you think we should use it? |
jparsai
force-pushed
the
keep-alive
branch
2 times, most recently
from
bf4a65d to
032b731
Compare
@chetan-rns addressed your comment PTAL |
cmd/agent/main.go
Outdated
| @@ -176,6 +182,9 @@ func NewAgentRunCommand() *cobra.Command { | |||
| command.Flags().IntVar(&metricsPort, "metrics-port", | |||
| env.NumWithDefault("ARGOCD_AGENT_METRICS_PORT", cmd.ValidPort, 8181), | |||
| "Port the metrics server will listen on") | |||
| command.Flags().StringVar(&keepAlivePingInterval, "keep-alive-ping-interval", | |||
There was a problem hiding this comment.
The cobra framework has flags that take duration strings as arguments (Duration, DurationVar, etc). Using these might remove the need for parsing the duration string yourself.
There was a problem hiding this comment.
Updated it, but we have to parse for env variable. PTAL
Signed-off-by: Jayendra Parsai <[email protected]>
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func ParseKeepAlivePingInterval(interval string) time.Duration { |
There was a problem hiding this comment.
It might make sense to make this function more generic and part of the internal/env package, so that it streamlines with the rest of the code base.
| @@ -275,6 +280,9 @@ func NewPrincipalRunCommand() *cobra.Command { | |||
| command.Flags().BoolVar(&enableResourceProxy, "enable-resource-proxy", | |||
| env.BoolWithDefault("ARGOCD_PRINCIPAL_ENABLE_RESOURCE_PROXY", true), | |||
| "Whether to enable the resource proxy") | |||
| command.Flags().DurationVar(&waitDurationForAgentPing, "wait-duration-for-agent-ping", | |||
| cmdutil.ParseKeepAlivePingInterval(env.StringWithDefault("WAIT_DURATION_FOR_AGENT_PING", nil, "")), | |||
There was a problem hiding this comment.
I think WAIT_DURATION_FOR_AGENT_PING is not a good name for the environment variable. PTAL at how the other environment variables that we use as arguments are named, and make this one align.
| @@ -176,6 +183,9 @@ func NewAgentRunCommand() *cobra.Command { | |||
| command.Flags().IntVar(&metricsPort, "metrics-port", | |||
| env.NumWithDefault("ARGOCD_AGENT_METRICS_PORT", cmd.ValidPort, 8181), | |||
| "Port the metrics server will listen on") | |||
| command.Flags().DurationVar(&keepAlivePingInterval, "keep-alive-ping-interval", | |||
| cmdutil.ParseKeepAlivePingInterval(env.StringWithDefault("KEEP_ALIVE_PING_INTERVAL", nil, "")), | |||
There was a problem hiding this comment.
I think KEEP_ALIVE_PING_INTERVAL is not a good name for the environment variable. PTAL at how the other environment variables that we use as arguments are named, and make this one align.
This PR is to introduce a configurable periodic ping to keep connection alive between agent and principal using grpc keepalive feature.
By default it is disabled and to enable it you need to set value of
keep-alive-ping-intervalin startup script or set env variableKEEP_ALIVE_PING_INTERVALat agent side and at principal side you have to set value ofwait-duration-for-agent-pingin startup script or set value of env variableWAIT_DURATION_FOR_AGENT_PING.Fixes #171