argo-helm icon indicating copy to clipboard operation
argo-helm copied to clipboard

Make Argo Workflows Deployment Restart on Config change using Helm ConfigMap annotation

Open ak-a opened this issue 3 months ago • 3 comments

Is your feature request related to a problem?

Using Argo-CD to deploy Argo Workflows means that it might not be automatically restarted if a configuration change is made that doesn't affect a deployment.

Related helm chart

argo-workflows

Describe the solution you'd like

Add an annotation to the Argo Workflows helm chart as documented: https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deployments Then the deployment will update when a ConfigMap updates and so a new deployment will rollout when using Argo CD.

Describe alternatives you've considered

Argo Workflows does have a config watcher, but this has been causing issues https://github.com/argoproj/argo-workflows/issues/11657. Whilst a fix is in the works, this still has load on a cluster. The Helm/yaml annotation method is simpler (IMHO) and much cheaper in terms of resource.

Additional context

No response

ak-a avatar Apr 04 '24 10:04 ak-a

I initially requested in the wrong repo https://github.com/argoproj/argo-workflows/issues/12876

ak-a avatar Apr 04 '24 10:04 ak-a

As I wrote in the upstream issue, this feature should be optional for the Controller as it is built into Workflows already. The value should also be explicitly documented as such. Note that there is also currently no way to turn off the built-in feature without turning off other features, like the semaphore ConfigMap watcher, per https://github.com/argoproj/argo-workflows/pull/12622.

This also would be good to have for the Server, which does not yet have a built-in watcher: https://github.com/argoproj/argo-workflows/issues/10970

and much cheaper in terms of resource.

For this part, I don't know that this is necessarily correct. Disabling the watcher will certainly use less constant memory, but it may not be significant. In terms of CPU, disk I/O, and network I/O, restarting the entire deployment instead of an in-memory hot reload is certainly more expensive (and creates churn on the cluster).

I think it would be more appropriate to call it a trade-off when it comes to performance.

You could also call it an optional workaround for a current bug (and any future such bugs), which is caused by an upstream k8s issue, and which can cause performance degradation.

agilgur5 avatar Apr 04 '24 15:04 agilgur5

I agree with @agilgur5. I think this should be a toggle(s) in the helm chart values file. Either use the config watcher to hot-reload config changes, or use the helm checksum/config annotation convention to automatically restart both the server and controller deployments on config changes. Note that we always need to use the checksum/config annotation for the server until https://github.com/argoproj/argo-workflows/issues/10970 has been resolved.

If the toggle is enabled:

  • Set the WATCH_CONTROLLER_SEMAPHORE_CONFIGMAPS env var to false for the controller.
  • Annotate both the server and controller deployments' pod templates with the checksum/config annotation to get them to automatically restart on config changes.

If the toggle is disabled (the default value):

  • Omit the WATCH_CONTROLLER_SEMAPHORE_CONFIGMAPS env var for the controller, or explicitly set it to true.
  • Annotate only the server deployment's pod template with the checksum/config annotation.

nhavens avatar Apr 04 '24 16:04 nhavens