-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add annotations to Deployment #2477
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @beastob! |
|
I signed it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @beastob, you'll need to sign the CLA before this can move forward and I've added some comments on the code.
But before we look too much into the implementation could you add some documentation links to the description showing systems that require deployment level annotations over pod annotations?
charts/external-dns/values.yaml
Outdated
@@ -26,8 +26,12 @@ rbac: | |||
create: true | |||
additionalPermissions: {} | |||
|
|||
# Annotations to add to the Deployment | |||
annotations: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be better named as deploymentAnnotations
unless you've got a strong idiomatic precedent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added an example use case in my PR description above. And unfortunately, some tools only look for deploymentAnnotations and not PodAnnotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beastob have you looked at adding a kiwigrid/k8s-sidecar? This is how Grafana handles configmap reloads and it seems to be a better architecture with a single responsibility container in a pod rather than a central service? If this worked it'd be easy to add configuration to enable extra containers in the pod and documentation to show how it could be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose https://github.com/kiwigrid/k8s-sidecar works in a way that's similar to Spring-boot's ConfigMap/Secret watcher. My answer would be yes and no. It's because watchers like kiwigrid/k8s-sidecar would restart(I supposed?) the application container upon config changes, and if the deployment has replica>1, all pods/containers will all restart at the same time and causes short period of down time. However, for this project's I think adding an extraContainers
field is a good idea, for those who would like to add sidecars.
staketer-reloader on the other hand can allow a RollingUpgrade effect to restart all deployment pods. Nevertheless, I believe allowing a common field annotations
for Deployment shouldn't be a bad idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preferred use of kiwigrid/k8s-sidecar is to hit a webhook to trigger a config reload but I think it can manually re-start a pod if a webhook isn't available.
Does ExternalDNS support running multiple replicas? I'm pretty sure you should only have a single "active" replica and I don't think having a non-leader replica would add a benefit to the ExternalDNS workflow even if possible? Even then assuming multiple pods, if configuration has changed I doubt you'd want some using old config? Finally I can't see ExternalDNS as a tier 1 service, I can't see any use case where unavailable for a restart cycle is a problem?
Nevertheless, I believe allowing a common field annotations for Deployment shouldn't be a bad idea?
It's not very idiomatic and promotes the tight coupling of controller to pod which should be avoided as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ExternalDNS support running multiple replicas? I'm pretty sure you should only have a single "active" replica and I don't think having a non-leader replica would add a benefit to the ExternalDNS workflow even if possible? Even then assuming multiple pods, if configuration has changed I doubt you'd want some using old config? Finally I can't see ExternalDNS as a tier 1 service, I can't see any use case where unavailable for a restart cycle is a problem?
I agree that ExternalDNS in particular does not need RollingUpgrade. It's just that using a centralised service like stakater-reloader we don't have to worry if a particular application out of dozens can support hot config reload or not, and we don't need config extra sidecar individually to achieve config reload. Only annotations would be needed.
And in GitOps approach of managing the kubernates cluster, we would want as little manual operations as possible. For example, changing some config in Git and then pushing it. Then the applications should reload the configs without further human interactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can agree to disagree on this approach.
So just the CLA error to fix and then someone can enable the CI to run, the code itself looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see your concerns, but it's the lesser of two evils in our case. I will try to figure out the CLA problem later, and thanks for the discussion.
@@ -4,6 +4,10 @@ metadata: | |||
name: {{ include "external-dns.fullname" . }} | |||
labels: | |||
{{- include "external-dns.labels" . | nindent 4 }} | |||
{{- with .Values.annotations }} | |||
annotations: | |||
{{ toYaml . | indent 4 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{ toYaml . | indent 4 }} | |
{{- toYaml . | nindent 4 }} |
This chart favours nindent
for this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestions. I have changed to deploymentAnnotations
and now using nindent
@beastob you need to amend your git commit(s) with the |
Signed-off-by: beastob <beastob.mark1@gmail.com>
I signed it |
Thanks alot for the help. This is my first PR to a linuxfoundation project and needs some getting use to. |
I signed it |
@Raffo could you enable the workflows? |
Hi, do you guys have any updates on this PR ? I would appreciate having deployment annotations as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@stevehipwell we'll leave this to you to |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: beastob, stevehipwell The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Allow adding annotations to Deployment via Helm values.
This enables using some third-party tools which rely on adding annotations on
Deployment
,Statefulset
,Daemonset
. For example, adding annotations to external-secrets Deployment allow stakater-reloader to restart(thus reload configs) deployment upon ConfigMap or Secret modifications.Example use case
For situations when external-dns reads from config or secret, which in my case it's the Azure configuration Secret. If I wish to restart external-dns deployment upon config change in order for that config change to be applied, I can utilize the ability of stakater-reloader to do it by adding annotations to external-dns' Deployment.
Fixes #ISSUE(none)
Checklist