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

fix: duplicated keys in annotations caused by airfowPodAnnotations value being overwritten by safeToEvict value of worker #40554

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

rzjfr
Copy link
Contributor

@rzjfr rzjfr commented Jul 2, 2024

as a result of f9db9c9952 commit, value of .Values.airflowPodAnnotations would be changed for all the other services, this would result in the services except workers, to have duplicate cluster-autoscaler.kubernetes.io/safe-to-evict as the first one would point to the safeToEvict value of the service itself and the second to .Values.airflowPodAnnotations which is overwritten by the pod template.

closes: #40553


@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jul 2, 2024
@rzjfr rzjfr force-pushed the fix-pod-annotations-safe-to-evict branch from 0028f3e to 7426af2 Compare July 2, 2024 15:56
@rzjfr rzjfr changed the title fix: pod annotation overwritten by safe to evict fix: duplicated keys in annotations caused by airfowPodAnnotations value being overwritten by safeToEvict value of worker Jul 2, 2024
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Could you add a test that fails without your fix?

@rzjfr
Copy link
Contributor Author

rzjfr commented Jul 3, 2024

Could you add a test that fails without your fix?

Hi @hussein-awala,
Thank you for reviewing the PR,

It's bit tricky to write a test to catch the key duplication as render_chart returns a dictionary. But since the in the order of annotations, .Values.airflowPodAnnotations comes after for example .Values.dagProcessor.safeToEvict (here) then the last one which is worker's safeToEvict value overwrites service's own safeToEvict .

So that is the only valid test which comes to my mind. It fails without the change in this PR.

I just added that in this commit, Please let me know if you had a better proposal.

@rzjfr
Copy link
Contributor Author

rzjfr commented Jul 4, 2024

I realized probably I should add bit of an explanation on the solution as well.

merge or mergeOverwrite actually combine the given parameters from right to left into the left most parameter. So what is actually happening is that pod-template is changing .Values.airflowPodAnnotations and since .Values.airflowPodAnnotations is being used in other services like scheduler, triggerer and dag-processor it causes issue. Since .Values.airflowPodAnnotations is being added separately after services' own safeToEvict annotation the result would have the safe to evict annotation at least twice. Actually from the two the one that matters most (the second one) is going to be the incorrect one (only if for example .Values.triggerer.podAnnotations is not set) as .Values.airflowPodAnnotations evaluated after services' own safe to evict annotation and .Values.airflowPodAnnotations would have workers' safe to evict value because pod-template has set it.

For example here you can see value of a (the left most variable) in the output.

in pod-template if we add a copy of Values.airflowPodAnnotations instead, the issue would be resolved.

@rzjfr rzjfr requested a review from jedcunningham July 9, 2024 07:07
@jedcunningham jedcunningham merged commit 5323471 into apache:main Jul 9, 2024
64 checks passed
@utkarsharma2 utkarsharma2 added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm chart: duplicate key in safe-to-evict annotation for services like scheduler
4 participants