-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Bugfix: Fix overriding pod_template_file
in KubernetesExecutor
#15197
Conversation
This feature was added in apache#11784 but it was broken as it got `pod_template_override` from `executor_config` instead of `pod_template_file`.
@@ -125,7 +125,7 @@ name ``base`` and a second container containing your desired sidecar. | |||
:end-before: [END task_with_sidecar] | |||
|
|||
You can also create custom ``pod_template_file`` on a per-task basis so that you can recycle the same base values between multiple tasks. | |||
This will replace the default ``pod_template_file`` named in the airflow.cfg and then override that template using the ``pod_override_spec``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pod_override_spec
never existed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice catch!
Please find my 2 cents.
}, | ||
), | ||
spec=k8s.V1PodSpec( | ||
containers=mock.ANY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible to make the assertion more granular? Container spec is worth testing carefully, e.g. the image used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was in two minds with that -- I added it back in 855d401
assert executor.task_queue.empty() | ||
|
||
executor.execute_async( | ||
key=('dag', 'task', datetime.utcnow(), 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be redundant, but to be on more safe side, we may want to assert the execution date here as well, rather than just having mock.ANY
.
We have have something like below here
execution_date = datetime.utcnow()
executor.execute_async(
key=('dag', 'task', execution_date, 1),
... ...
Then in mock_run_pod_async.assert_called_once_with()
, have
annotations={
'dag_id': 'dag',
'execution_date': execution_date.isoformat(),
... ...
}
and
labels={
'airflow-worker': '5',
'airflow_version': mock.ANY,
'dag_id': 'dag',
'execution_date': pod_generator.datetime_to_label_safe_datestring(execution_date),
... ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 855d401
serviceAccountName: airflow-worker | ||
serviceAccount: airflow-worker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was needed after Helm chart change: 23768f6
However since we were not using the pod_template_file
in the test we didn't find that it was broken
…bernetesExecutor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
…che#15197) This feature was added in apache#11784 but it was broken as it got `pod_template_override` from `executor_config` instead of `pod_template_file`. closes apache#14199 (cherry picked from commit 5606137) (cherry picked from commit 091fae90a0a564e2da92ead7dd5be2c1e8b56301)
This feature was added in #11784 but
it was broken as it got
pod_template_override
fromexecutor_config
instead of
pod_template_file
.closes #14199
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.