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

Setting podAnnotations for workers also adds them to other deployments such as scheduler #40804

Closed
2 tasks done
joshuaghezzi opened this issue Jul 16, 2024 · 7 comments
Closed
2 tasks done
Labels
area:helm-chart Airflow Helm Chart good first issue kind:bug This is a clearly a bug

Comments

@joshuaghezzi
Copy link
Contributor

joshuaghezzi commented Jul 16, 2024

Official Helm Chart version

1.14.0 (latest released)

Apache Airflow version

2.9.2

Kubernetes Version

EKS 1.30

Helm Chart configuration

executor: "KubernetesExecutor"

workers:
  podAnnotations:
    "karpenter.sh/do-not-disrupt": "true"

Docker Image customizations

Yes but not relevant to this I don't think.

What happened

Configuring podAnnotations for workers results in other deployments such as the scheduler also getting those annotations if using executor: "KubernetesExecutor"

What you think should happen instead

Only the worker pods should get those annotations.

How to reproduce

Configure the helm chart with executor and worker podAnnotations such as

executor: "KubernetesExecutor"

workers:
  podAnnotations:
    "karpenter.sh/do-not-disrupt": "true"

Anything else

We added this annotation in conjunction with updating our helm chart version from 1.13.1 to 1.14.0 so unsure if it occurs on earlier helm chart versions. Having a look at the release notes though it looks like this may potentially be the culprit #37352 since it looks like it overwrites airflowPodAnnotations with workers.podAnnotations.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@joshuaghezzi joshuaghezzi added area:helm-chart Airflow Helm Chart kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Jul 16, 2024
@shahar1 shahar1 removed the needs-triage label for new issues that we didn't triage yet label Jul 16, 2024
@romsharon98
Copy link
Contributor

I attempted to reproduce the issue in the main branch but was unsuccessful.
I modified the worker.podAnnotation as you suggested, and this is the result I got in the scheduler deployment annotation section:

annotations:
        checksum/metadata-secret: 8c8ccea43b91003b5ed19c1cb7ae904bdbee3fa66010c473b88fb196e3c25e59
        checksum/result-backend-secret: 98a68f230007cfa8f5d3792e1aff843a76b0686409e4a46ab2f092f6865a1b71
        checksum/pgbouncer-config-secret: 1dae2adc757473469686d37449d076b0c82404f61413b58ae68b3c5e99527688
        checksum/airflow-config: af9a572727db12dc95b758037573ef675d45e33c4acf8e5ee7b2d4184e228577
        checksum/extra-configmaps: e862ea47e13e634cf17d476323784fa27dac20015550c230953b526182f5cac8
        checksum/extra-secrets: e9582fdd622296c976cbc10a5ba7d6702c28a24fe80795ea5b84ba443a56c827
        cluster-autoscaler.kubernetes.io/safe-to-evict: "true"

Maybe you have changed your configuration in airflowPodAnnotations section?

@joshuaghezzi
Copy link
Contributor Author

@romsharon98 thanks for looking at this. We do not use airflowPodAnnotations so that wouldn't be the cause of that for us. I'm aware those annotations would apply to all deployments.

@joshuaghezzi
Copy link
Contributor Author

joshuaghezzi commented Jul 17, 2024

Can confirm this only occurs from 1.14.0, I tried testing it with 1.13.1 and the annotation only appeared on the worker pods as expected.

image

image

@joshuaghezzi
Copy link
Contributor Author

joshuaghezzi commented Jul 18, 2024

Another thing I've discovered, this only seems to happen in conjunction when configuring the the executor in the chart as well. Updated the issue to reflect this condition.

image

@potiuk
Copy link
Member

potiuk commented Jul 18, 2024

Anyone willing to provide a fix are welcome to.

@joshuaghezzi
Copy link
Contributor Author

Apologies, looks like this has actually been fixed after all with #40554. Just tested again with the main branch as opposed to latest helm chart release.

@jedcunningham
Copy link
Member

Yeah, I believe this was already fixed as you've found. I'm going to start a release for the helm chart tomorrow.

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 good first issue kind:bug This is a clearly a bug
Projects
None yet
Development

No branches or pull requests

5 participants