You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "fweilun (via GitHub)" <gi...@apache.org> on 2025/07/09 09:43:06 UTC
[PR] Fix ecs retry condition [airflow]
fweilun opened a new pull request, #53080:
URL: https://github.com/apache/airflow/pull/53080
Related issue: https://github.com/apache/airflow/issues/52943
Draft PR.
This PR adds a EcsCannotPullContainerError exception to handle scenarios where ECS tasks fail to start due to image pull issues (e.g., CannotPullContainerError).
Test added: ✅ test_should_retry_eni_false_for_pull_failure
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "dominikhei (via GitHub)" <gi...@apache.org>.
dominikhei commented on code in PR #53080:
URL: https://github.com/apache/airflow/pull/53080#discussion_r2195791545
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/ecs.py:
##########
@@ -29,6 +33,9 @@
def should_retry(exception: Exception):
"""Check if exception is related to ECS resource quota (CPU, MEM)."""
Review Comment:
Just a tiny nit, but maybe adjust the docstring to incorporate the new behavior?
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "dominikhei (via GitHub)" <gi...@apache.org>.
dominikhei commented on code in PR #53080:
URL: https://github.com/apache/airflow/pull/53080#discussion_r2195791545
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/ecs.py:
##########
@@ -29,6 +33,9 @@
def should_retry(exception: Exception):
"""Check if exception is related to ECS resource quota (CPU, MEM)."""
Review Comment:
Just a tiny knit, but maybe adjust the docstring to incorporate the new behavior?
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on PR #53080:
URL: https://github.com/apache/airflow/pull/53080#issuecomment-3067546978
Close this one as #53083 will resolve it.
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #53080:
URL: https://github.com/apache/airflow/pull/53080#discussion_r2194615015
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/ecs.py:
##########
@@ -29,6 +33,9 @@
def should_retry(exception: Exception):
"""Check if exception is related to ECS resource quota (CPU, MEM)."""
+ if isinstance(exception, EcsCannotPullContainerError):
+ return False
+
if isinstance(exception, EcsOperatorError):
Review Comment:
IMO, let the `EcsCannotPullContainerError` error fail fast instead of retrying should be fine right ?
Based on the [Documentation - CannotPullContainer task errors in Amazon ECS](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_cannot_pull_image.html), it's more like configuration error from user instead of system instability.
cc @o-nikolas , @eladkal
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #53080:
URL: https://github.com/apache/airflow/pull/53080#issuecomment-3054926306
also, CI needs to be fixed
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on PR #53080:
URL: https://github.com/apache/airflow/pull/53080#issuecomment-3060418723
> I wonder whether removing the retry mechanism might make more sense here. #53083
+0 for leaving retry to task level retry by Airflow instead of retrying at `_check_success_task ` function level. Both works, no strong option for them.
It seems to be more simple. However, as mentioned in https://github.com/apache/airflow/pull/53083#pullrequestreview-3007609304 removing the function level retry mechanism will introduce breaking change.
IMO, if we go for removing function level retry mechanism (#53083) then we might need to close this one.
Or, if we decide to remain the function level retry for compatibility, I think by adding `CannotPullContainerError` str in `should_retry_eni` function
https://github.com/apache/airflow/blob/d3dbc28effaee0e02f0d419b150c11d708d53377/providers/amazon/src/airflow/providers/amazon/aws/hooks/ecs.py#L40-L47
remove the following explicit error handling and remove the `EcsCannotPullContainerError`
```python
if "CannotPullContainerError" in task.get("stoppedReason", ""):
raise EcsCannotPullContainerError(
f"The task failed to start due to: {task.get('stoppedReason', '')}"
)
```
might be more simple and more generic.
Then we can go through the possible error of `stoppedReason` and add the missing retryable error ( `CannotPullContainerError` in this case ) to `["network interface provisioning", "ResourceInitializationError"]` list to make it more robust ( maybe make this list as an enum or global constant ).
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 closed pull request #53080: Fix ecs/EcsRunTaskOperator retry condition
URL: https://github.com/apache/airflow/pull/53080
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #53080:
URL: https://github.com/apache/airflow/pull/53080#issuecomment-3063229031
> > I wonder whether removing the retry mechanism might make more sense here. #53083
>
> +0 for leaving retry to task level retry by Airflow instead of retrying at `_check_success_task ` function level. Both works, no strong option for them.
>
> It seems to be more simple. However, as mentioned in [#53083 (review)](https://github.com/apache/airflow/pull/53083#pullrequestreview-3007609304) removing the function level retry mechanism will introduce breaking change.
>
> IMO, if we go for removing function level retry mechanism (#53083) then we might need to close this one.
>
> Or, if we decide to remain the function level retry for compatibility, I think by adding `CannotPullContainerError` str in `should_retry_eni` function
>
> https://github.com/apache/airflow/blob/d3dbc28effaee0e02f0d419b150c11d708d53377/providers/amazon/src/airflow/providers/amazon/aws/hooks/ecs.py#L40-L47
>
> remove the following explicit error handling and remove the `EcsCannotPullContainerError`
>
> ```python
> if "CannotPullContainerError" in task.get("stoppedReason", ""):
> raise EcsCannotPullContainerError(
> f"The task failed to start due to: {task.get('stoppedReason', '')}"
> )
> ```
>
> might be more simple and more generic.
>
> Then we can go through the possible error of `stoppedReason` and add the missing retryable error ( `CannotPullContainerError` in this case ) to `["network interface provisioning", "ResourceInitializationError"]` list to make it more robust ( maybe make this list as an enum or global constant ).
Just to +1 again, full reply [here](https://github.com/apache/airflow/pull/53083#pullrequestreview-3007609304) I think we should make every effort to simplify this one. If the mechanism is _truly_ broken then we can just mark it as a bug fix and then we don't need to worry about breaking changes. It's just determining if it's broken in all/most cases which needs confirming by perhaps @vladimirbinshtok (the original requester). Once we have that we can go with [this PR](https://github.com/apache/airflow/pull/53083).
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "fweilun (via GitHub)" <gi...@apache.org>.
fweilun commented on PR #53080:
URL: https://github.com/apache/airflow/pull/53080#issuecomment-3052144114
> Nice! Thanks for the PR.
>
> How about adding a new test to check whether does `_check_success_task` raise the new `EcsCannotPullContainerError` exception?
>
> Here are some good references to add test case against `_check_success_task`:
>
> https://github.com/apache/airflow/blob/7f6bc14c7ad659af70ebe24df2f0147947afd5d1/providers/amazon/tests/unit/amazon/aws/operators/test_ecs.py#L447-L464
Done! Test case added.
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53080:
URL: https://github.com/apache/airflow/pull/53080#discussion_r2196326425
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/ecs.py:
##########
@@ -29,6 +33,9 @@
def should_retry(exception: Exception):
"""Check if exception is related to ECS resource quota (CPU, MEM)."""
+ if isinstance(exception, EcsCannotPullContainerError):
+ return False
+
if isinstance(exception, EcsOperatorError):
Review Comment:
I think there're cases suitable for retry with a reasonable wait time. e.g.,
[ERROR: toomanyrequests: Too Many Requests or You have reached your pull rate limit.](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_cannot_pull_image.html#container-pull-too-many-requests)
--
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
Re: [PR] Fix ecs/EcsRunTaskOperator retry condition [airflow]
Posted by "fweilun (via GitHub)" <gi...@apache.org>.
fweilun commented on PR #53080:
URL: https://github.com/apache/airflow/pull/53080#issuecomment-3058263180
I wonder whether removing the retry mechanism might make more sense here.
https://github.com/apache/airflow/pull/53083
--
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