Allow separate Dag processor object and pod labels#70205
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
There was a problem hiding this comment.
Pull request overview
This PR aligns the Helm chart behavior with the documented contract that dagProcessor.labels applies to Dag processor Kubernetes objects (not just the Pods), by also merging those component-specific labels into the Dag Processor Deployment metadata labels while keeping component-over-global precedence.
Changes:
- Merge
.Values.dagProcessor.labelswith.Values.labelsforDeployment.metadata.labelsin the dag-processor deployment template. - Add a regression test covering global-only, component-only, and colliding label keys for
Deployment.metadata.labels. - Add a chart newsfragment documenting the user-visible Helm chart enhancement.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| chart/templates/dag-processor/dag-processor-deployment.yaml | Applies the same global+component label merge logic to Deployment metadata labels as already used for Pod template labels. |
| chart/tests/helm_tests/dagprocessor/test_labels_deployment.py | Adds coverage to ensure merged labels appear on the Deployment metadata and component labels override global keys. |
| chart/newsfragments/53190.feature.rst | Records the chart feature change in release notes. |
|
Could you please check discussions from previous proposed PRs for the relevant change #59290? |
Miretpl
left a comment
There was a problem hiding this comment.
Requesting changes to block accidential merge
|
Thanks @Miretpl — I read through #59290 and understand the hold now. That PR made the same change for the webserver and stalled on a design question that was never settled, so rather than re-land it for dag-processor I'd like a direction first. Today per-component labels (e.g. dagProcessor.labels) only land on the pod template (spec.template.metadata.labels); the Deployment's own metadata.labels gets global labels only. Two ways to close that gap: A) Merge component labels onto the Deployment metadata directly (what this PR does now). Simple, but it changes where existing users' labels land, so it's a backward-compatibility concern. B) Add a per-component podLabels section (like the existing annotations / podAnnotations split): labels go to the Deployment, podLabels to the pod, and when podLabels is unset, labels fall back to the pod level to preserve today's behavior. This is the approach Miretpl proposed in #59290. On scope, #59290 also concluded this should be applied across all components rather than one at a time, and target apiServer rather than webserver (webserver is going away with Airflow 2.x). Happy to broaden this PR accordingly. Could a committer confirm (1) approach A or B, and (2) whether you want the full multi-component change in one PR? I'll rework and rebase onto main once there's a direction. cc @jscheffl @potiuk @jedcunningham @romsharon98 |
|
My idea #59290 (comment) after becoming a Committer (I wasn't back then) is still the same. Separation, like in annotations, gives more flexibility, and I see cases when it would be nice to separate labels between the pod and the deployment itself. And if there will be a user who will want to have the same labels, there will be a possibility to set the same value on both levels. |
|
Thanks @Miretpl — agreed, the separation is the cleaner model. I'll go with podLabels:
I'll implement it on dag-processor here first as the reference pattern (values.yaml + template + tests), and once you're happy with it extend the same pattern to the other components, using apiServer rather than webserver. Prefer everything in one PR, or dag-processor first then follow-ups? |
In general small PRs are better, but whole thing will rather not be big, so I will adjust. Do as it will be more comfortable for you.
The Webserver will also be needed, but for the 1.2x line (on main we have 2.0) only. |
5346103 to
6957826
Compare
|
Implemented the
I also added values/schema documentation, a news fragment, and regression coverage for the fallback, separated-label, collision, and explicit-empty-map cases. The branch is rebased on current Validation completed:
Drafted-by: Codex (GPT-5) (no human review before posting) |
|
Verified on the rebased branch:
Backward compatibility: rendering the whole chart with the same values against current Tests: 7 passed in Ready for review @Miretpl. |
Add
dagProcessor.podLabelsso Deployment/object and pod labels can be configured separately.When
dagProcessor.podLabelsis unset, existing behavior remains unchanged:dagProcessor.labelsstays on Dag processor pods, while the Deployment receives only global labels. SettingpodLabels(including{}) opts into separation:dagProcessor.labelsapplies to the Deployment anddagProcessor.podLabelsapplies to pods. Component values retain precedence over global labels.closes: #53190
Tests:
breeze testing helm-tests --use-xdist(2095 passed)prek run --from-ref upstream/main --stage pre-commitprek run --from-ref upstream/main --stage manualbreeze ci selective-check --commit-ref HEADWas generative AI tooling used to co-author this PR?
Generated-by: Codex (GPT-5) following the guidelines
Drafted-by: Codex (GPT-5) (no human review before posting)