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

operator: Fix update of labels and annotations of PodTemplates #9860

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

xperimental
Copy link
Collaborator

What this PR does / why we need it:

While testing the zone-awareness PR I noticed that the labels and annotations on the pods did not disappear when I deactivated the zone-awareness again. This turned out to be an issue with how we generally update existing Deployments/StateFulSets: We merge the existing PodTemplateSpec with the desired one.

I don't think we need the merge at that point, because the operator should be the complete owner of that attribute, so we should be able to completely replace the values instead of merging with the existing one.

Which issue(s) this PR fixes:

Special notes for your reviewer:

The situation with the annotations and labels of our resources is a bit confusing. On one hand we have mergeWithOverride code at the beginning of the MutateFuncFor function but on the other hand almost every one of the per-type mutate* functions contains code that overwrites existing labels/annotations. I think it should be either one or the other.

On a related note, maybe it makes sense to keep existing labels/annotations on the resources ("merging" them with the existing ones, but explicitly keep track of our labels/annotations, so that we remove our annotations/labels when they are not needed anymore but don't touch "other people's annotations/labels". This would make the merge code a bit longer though.

This PR remains a draft until the question about how to proceed with labels/annotations is cleared up.

Checklist

Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

Well caught! Apart from thinking that we might hit Hyrum's Law I'm on the side of just setting the labels/annotations. I checked on prom-operator and there we also "just" set labels (at least on the Prometheus StatefulSet).

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 12, 2023
@xperimental xperimental force-pushed the operator-no-merge-on-update branch from 181ae4c to 0150a95 Compare July 12, 2023 17:19
@xperimental xperimental changed the title operator: Do not use mergo for updating existing resources operator: Fix update of labels and annotations of PodTemplates Jul 12, 2023
@xperimental xperimental marked this pull request as ready for review July 12, 2023 19:34
@xperimental xperimental requested a review from a team as a code owner July 12, 2023 19:34
@periklis periklis merged commit 25c26ec into grafana:main Jul 13, 2023
@xperimental xperimental deleted the operator-no-merge-on-update branch July 13, 2023 10:02
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.

3 participants