Skip to content

feat(argo-cd): Support livenessProbe for Application Controller#2978

Closed
yu-croco wants to merge 2 commits intoargoproj:mainfrom
yu-croco:add-livenessprobe-to-argocd-controller
Closed

feat(argo-cd): Support livenessProbe for Application Controller#2978
yu-croco wants to merge 2 commits intoargoproj:mainfrom
yu-croco:add-livenessprobe-to-argocd-controller

Conversation

@yu-croco
Copy link
Copy Markdown
Collaborator

@yu-croco yu-croco commented Oct 13, 2024

Resolves #2973

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

Signed-off-by: yu-croco <yu.croco@gmail.com>
Comment on lines +315 to +317
httpGet:
path: /healthz
port: metrics
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +313 to +316
livenessProbe:
httpGet:
path: /healthz
port: metrics
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yu-croco yu-croco marked this pull request as ready for review October 13, 2024 14:08
Copy link
Copy Markdown
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should place a (prominent) note that this can have side effects.

We once had the probes on the application-controller but due to an (upstream) issue, upstream removed it and thus we also removed it in the helm chart.

Current controller liveness probe is a noop just checking if the metric-server is able to receive requests.

In cases when the controller is overloaded reconciling large queues restarting the Pod is more harmful than letting it running. In discussion with @alexmt we understand that liveness probe should be removed from the controller to prevent it from being restarted.

Signed-off-by: Leonardo Luz Almeida leonardo_almeida@intuit.com

Xref:

@yu-croco
Copy link
Copy Markdown
Collaborator Author

@mkilchhofer Thank you for your review with the backgroud.
It seems that it's not worth to take risk even it's optional... 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add liveness probe to application-controller deployment and statefulset templates

3 participants