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 2022/06/02 16:40:35 UTC
[GitHub] [airflow] dstandish opened a new pull request, #24117: Handle incompatible pickled executor config after k8s upgrade
dstandish opened a new pull request, #24117:
URL: https://github.com/apache/airflow/pull/24117
From time to time k8s library objects change their attrs. If executor config is stored with old version, and unpickled with new version, we can get attribute errors that can crash the scheduler (see https://github.com/apache/airflow/issues/23727).
Here we update handling so that we fail the task but don't crash the scheduler.
--
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] uranusjr commented on a diff in pull request #24117: Don't crash scheduler if exec config has old k8s objects
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24117:
URL: https://github.com/apache/airflow/pull/24117#discussion_r889867966
##########
airflow/models/taskinstance.py:
##########
@@ -400,6 +400,13 @@ def key(self) -> "TaskInstanceKey":
return self
+def executor_config_comparator(x, y):
+ try:
+ return x == y
+ except AttributeError:
Review Comment:
Instead of catching AttributeError here (which is confusing), can this failover happen in `__eq__` 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] dstandish commented on a diff in pull request #24117: Handle incompatible pickled executor config after k8s upgrade
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24117:
URL: https://github.com/apache/airflow/pull/24117#discussion_r888453876
##########
airflow/models/taskinstance.py:
##########
@@ -400,6 +400,13 @@ def key(self) -> "TaskInstanceKey":
return self
+def executor_config_comparator(x, y):
+ try:
+ return x == y
+ except AttributeError:
Review Comment:
cus it blows up when you call to_dict and that's how `__eq__` is defined:
```python
def __eq__(self, other):
"""Returns true if both objects are equal"""
if not isinstance(other, V1Container):
return False
return self.to_dict() == other.to_dict()
```
if you look at the to_dict code in on a kube object in the k8s library, it relies on on the current api structure and calls getattr for each attr that should be there, and if version not compatble, it will try to access an attr that isn't there.
it's confusing... trying to see what we can do about this.
--
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 merged pull request #24117: Don't crash scheduler if exec config has old k8s objects
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #24117:
URL: https://github.com/apache/airflow/pull/24117
--
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 #24117: Don't crash scheduler if exec config has old k8s objects
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24117:
URL: https://github.com/apache/airflow/pull/24117#issuecomment-1155694410
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 diff in pull request #24117: Handle incompatible pickled executor config after k8s upgrade
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24117:
URL: https://github.com/apache/airflow/pull/24117#discussion_r888586939
##########
airflow/models/taskinstance.py:
##########
@@ -400,6 +400,13 @@ def key(self) -> "TaskInstanceKey":
return self
+def executor_config_comparator(x, y):
+ try:
+ return x == y
+ except AttributeError:
Review Comment:
Ah, yes, that makes sense, thanks. Didn't think of the version compatibility aspect of `__eq__`.
--
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] uranusjr commented on a diff in pull request #24117: Don't crash scheduler if exec config has old k8s objects
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24117:
URL: https://github.com/apache/airflow/pull/24117#discussion_r891923071
##########
airflow/models/taskinstance.py:
##########
@@ -400,6 +400,13 @@ def key(self) -> "TaskInstanceKey":
return self
+def executor_config_comparator(x, y):
+ try:
+ return x == y
+ except AttributeError:
Review Comment:
Is the exception raised from PickleType? In that case I guess a comment here is the best we can do.
--
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 diff in pull request #24117: Handle incompatible pickled executor config after k8s upgrade
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24117:
URL: https://github.com/apache/airflow/pull/24117#discussion_r888441855
##########
airflow/models/taskinstance.py:
##########
@@ -400,6 +400,13 @@ def key(self) -> "TaskInstanceKey":
return self
+def executor_config_comparator(x, y):
+ try:
+ return x == y
+ except AttributeError:
Review Comment:
Wait, how can this raise AttributeError?
--
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 a diff in pull request #24117: Don't crash scheduler if exec config has old k8s objects
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24117:
URL: https://github.com/apache/airflow/pull/24117#discussion_r890484560
##########
airflow/models/taskinstance.py:
##########
@@ -400,6 +400,13 @@ def key(self) -> "TaskInstanceKey":
return self
+def executor_config_comparator(x, y):
+ try:
+ return x == y
+ except AttributeError:
Review Comment:
yeah if we were using our own PickleType then we'd be able to do that. but we're using sqlalchemy's PickleType. Seems that it's not worth subclassing when the class already provides a mechanism (i.e. compator) for handling this kind of thing. WDYT?
--
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