Fix duplicate pod creation in KubernetesJobOperator#53368
Fix duplicate pod creation in KubernetesJobOperator#53368shahar1 merged 1 commit intoapache:mainfrom
Conversation
0e04769 to
a9ad704
Compare
d6d3fa4 to
8b43bca
Compare
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_job.py
Outdated
Show resolved
Hide resolved
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Outdated
Show resolved
Hide resolved
30445de to
f123b1a
Compare
|
In reference to #49899 - I think a change is still necessary to avoid creating multiple pods when parallelism is not set. Do we want to change the control flow to use |
You could start with handling only the case where |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_job.py:921
- The condition
if do_xcom_push and not parallelismis checking ifparallelismis falsy (0, None, False, etc.). However, with the change to defaultparallelism=1, this condition will never be true whenparallelism=1is set. The logic should checkif do_xcom_push and parallelism == 1instead to handle the single pod case correctly.
if do_xcom_push and not parallelism:
mock_extract_xcom.assert_called_once()
elif do_xcom_push and parallelism is not None:
assert mock_extract_xcom.call_count == parallelism
else:
mock_extract_xcom.assert_not_called()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Show resolved
Hide resolved
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Show resolved
Hide resolved
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Show resolved
Hide resolved
shahar1
left a comment
There was a problem hiding this comment.
Almsost there!
I've revised my comment from a previous review (apologies for that!).
Please review my current comments + address Copilot comments.
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Show resolved
Hide resolved
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Outdated
Show resolved
Hide resolved
2c57cda to
d561015
Compare
d561015 to
b0f5289
Compare
|
LGTM, I'm making sure with the other contributors that handling If no strong objections are given in the next 1-2 days, or there are additional approvals for this PR by then - I'm ok with merging it as-is. |
|
@stephen-bracken / @shahar1 as concerns raised in https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1767429130216899 - one thing to consider maybe is - I'd propose to - adding a newsfragment such that it is highlighted in the changelog of the provider. TLDR: Requesting to add a newsfragment to highlight this interface change. UPDATE: Args, providers have no newsfragmen, add it to changelogs like in https://github.com/apache/airflow/pull/59143/changes#diff-24cff4e7b7926b95f4efef45da9f9d6f43b237b5143990b1554113251cb2c12eR30 for example that it is included in next providers wave. |
8befa0f to
5a29826
Compare
@jscheffl Thanks, added a changelog note. |
5a29826 to
50261b1
Compare
50261b1 to
6b3dc30
Compare
|
Drafting the PR following the author's request to make some changes, please do not merge |
6b3dc30 to
faae3ae
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
|
Great job @stephen-bracken ! |
Fix KubernetesJobOperator.get_or_create_pod() sometimes creating duplicate pods.
(re-raised from #52885)
during
execute()the KubernetesJobOperator attempts to find the pod associated with the Job object usingself.get_or_create_pod(). If Kubernetes is being slow then the Job object will not create a pod before this method gets called, which will result in the underlyingfind_pod()method returningNone, and a duplicate headless pod being created for this task.This PR removes references to the
get_or_create_pod()method in favour ofKubernetesJobOperator.get_pod()to prevent creating headless pods.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.