You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/12/02 00:22:02 UTC
[GitHub] [airflow] lsenjov opened a new pull request #19949: Only update label when adopting to prevent new pods
lsenjov opened a new pull request #19949:
URL: https://github.com/apache/airflow/pull/19949
Background:
We run a lot of dags/tasks (~600 dags with 50 tasks in each) on kubernetes, and the scheduler tends to crash from so many. Adoption is common.
On adoption, it would patch the pod, but when a specific field would get re-written (I'm not sure which, sorry) that would cause a new pod to be created instead.
This new pod would then change the status of the task instance, causing the pod to die with SIGTERM, and the task to fail.
This patch asks it to only update the label, which has removed the symptom (from 300+ daily failures to 0).
I only needed to do this for `adopt_launched_task`, but also did it for `adopt_finished_task` for consistency.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-1046128953
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-984177692
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] lsenjov commented on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
lsenjov commented on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-1046320055
This fell off my radar, apologies. What exactly am I required to do to finish this off?
Dropping the pod mutation and calling `patch_namespaced_pod` directly instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] JeremieDoctrine edited a comment on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
JeremieDoctrine edited a comment on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-1050714692
Our issue was actually the some environment variables were set twice (FERNET_KEY) once fixed the scheduler was able to adopt pod after a restart
https://github.com/apache/airflow/issues/20690 and https://github.com/airflow-helm/charts/issues/532
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] JeremieDoctrine commented on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
JeremieDoctrine commented on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-1050714692
Our issue was actually the some environment variables were set twice (FERNET_KEY) once fixed the scheduler was able to adopt pod after a restart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-1006163853
> I am not sure if this is the correct change, wdyt @jedcunningham @dstandish
The docstring in the updated method says this
> Patch existing pod so that the current KubernetesJobWatcher can monitor it via label selectors
Looking over the code, it does appear that `airflow-worker` is the only thing that would need to be patched in this scenario. The other selectors used to find pods (dag id, task id, run id) would not change in this scenario (scheduler death).
So this looks like it should be ok to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#discussion_r779139138
##########
File path: airflow/executors/kubernetes_executor.py
##########
@@ -737,7 +737,7 @@ def _adopt_completed_pods(self, kube_client: client.CoreV1Api) -> None:
kube_client.patch_namespaced_pod(
name=pod.metadata.name,
namespace=pod.metadata.namespace,
- body=PodGenerator.serialize_pod(pod),
+ body={"metadata": {"labels": {"airflow-worker": pod.metadata.labels['airflow-worker']}}},
Review comment:
Maybe we shouldn't mutate `pod` above and either use a variable or call `make_safe_label_value` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #19949: Only update label when adopting to prevent new pods
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19949:
URL: https://github.com/apache/airflow/pull/19949#issuecomment-989299026
I am not sure if this is the correct change, wdyt @jedcunningham @dstandish
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org