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

feat(helm): add lifecycle hooks to alloy container #1774

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

etiennep
Copy link
Contributor

@etiennep etiennep commented Sep 26, 2024

PR Description

Which issue(s) this PR fixes

Fixes #1140
Fixes #1673

Adds lifecycle hooks to the alloy container to allow graceful target de-registration when using with AWS LoadBalancer Controller ingress.

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@etiennep etiennep requested a review from a team as a code owner September 26, 2024 21:42
@CLAassistant
Copy link

CLAassistant commented Sep 26, 2024

CLA assistant check
All committers have signed the CLA.

@mattdurham
Copy link
Collaborator

This seems reasonable but looping in @petewall for any thoughts.

@@ -183,6 +191,9 @@ controller:
# -- Configures the DNS policy for the pod. https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-dns-policy
dnsPolicy: ClusterFirst

# -- Termination grace period in seconds for the Grafana Alloy pods.
terminationGracePeriodSeconds: null
Copy link
Contributor

Choose a reason for hiding this comment

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

should this have a default value? 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to Kubernetes documentation, the default is 30s. https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/

I will add a note to the comment.

@petewall
Copy link
Contributor

petewall commented Oct 1, 2024

This seems reasonable but looping in @petewall for any thoughts.

Left a few comments. All in all, looks good to me.

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

LGTM

@mattdurham
Copy link
Collaborator

@clayton-cornell for review of docs.

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

LGTM

@mattdurham
Copy link
Collaborator

Seeing some helm linting errors.

@etiennep
Copy link
Contributor Author

etiennep commented Oct 1, 2024

I think the linting issues should be fixed now @mattdurham

Thank you.

@mattdurham mattdurham merged commit 03da0f1 into grafana:main Oct 2, 2024
15 checks passed
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.

add lifecycle on pod_template Container lifecycle hook support in helm chart
5 participants