Use waiters in ECS Operators instead of inner sensors #29761
Conversation
boring-cyborg
bot
added
area:providers
provider:amazon-aws
| if task_definition_state == EcsTaskDefinitionStates.INACTIVE: | ||
| # In some circumstances ECS Task Definition deleted immediately, | ||
| # and there is no reason wait for completion. |
There was a problem hiding this comment.
To be honest this happen almost always 🤣
| @@ -55,7 +55,7 @@ def should_retry_eni(exception: Exception): | |||
| return False | |||
|
|
|||
|
|
|||
| class EcsClusterStates(Enum): | |||
| class EcsClusterStates(str, Enum): | |||
There was a problem hiding this comment.
This for allow comparison EcsClusterStates.ACTIVE == "ACTIVE" unfortunetly class StrEnum available only in python 3.11
There was a problem hiding this comment.
Yeah, I was hoping to use StrEnum there but... not yet. :( I didn't know you could do it this way, nice.
Taragolis
force-pushed
the
ecs-operators-waiters
branch
from
1a1868b
to
7a81b8e
Compare
| @@ -55,7 +55,7 @@ def should_retry_eni(exception: Exception): | |||
| return False | |||
|
|
|||
|
|
|||
| class EcsClusterStates(Enum): | |||
| class EcsClusterStates(str, Enum): | |||
There was a problem hiding this comment.
Yeah, I was hoping to use StrEnum there but... not yet. :( I didn't know you could do it this way, nice.
| @@ -0,0 +1,81 @@ | |||
| { | |||
There was a problem hiding this comment.
Love seeing this put to use. I think it's so much cleaner in the implementation. 👍
| "expected": "MISSING", | ||
| "matcher": "pathAny", | ||
| "state": "failure", | ||
| "argument": "failures[].reason" |
There was a problem hiding this comment.
Hehe, I've looked into builtin botocore waiters for ECS: https://github.com/boto/botocore/blob/develop/botocore/data/ecs/2014-11-13/waiters-2.json
And after that I checked which state available for different API calls https://docs.aws.amazon.com/AmazonECS/latest/developerguide/api_failures_messages.html
| } | ||
| ] | ||
| }, | ||
| "task_definition_active": { |
There was a problem hiding this comment.
No failure states for this one? I think if the status goes to 'DELETE_IN_PROGRESS' it can fail early. Not sure where 'INACTIVE' is on the state flow on this call though.
There was a problem hiding this comment.
Yeah 'DELETE_IN_PROGRESS' it could be some kind failure step, I can't imagine how it possible that state changed to this state during create task definition, but why not to add it as failure.
There was a problem hiding this comment.
Pretty much my thought as well. I don't how it would get there, but if it did then something is clearly wrong so there's no point waiting for the timeout.
| } | ||
| ] | ||
| }, | ||
| "task_definition_inactive": { |
There was a problem hiding this comment.
Would 'DELETE_IN_PROGRESS' be a success state here?
There was a problem hiding this comment.
I'd rather say it intermediate state in case of delete task definition.
| @@ -39,7 +39,7 @@ | |||
| EcsTaskDefinitionStates, | |||
| should_retry_eni, | |||
| ) | |||
| from airflow.providers.amazon.aws.sensors.ecs import EcsClusterStateSensor, EcsTaskDefinitionStateSensor | |||
| from airflow.utils.helpers import prune_dict | |||
There was a problem hiding this comment.
Nice. I have a bunch of EMR waiters I have been workigb on converting over but put on pause to verify how they behave if None is passed in, but this would solve that nncely. I'll get the EMR ones up this week and tag you in them. 👍
Taragolis
force-pushed
the
ecs-operators-waiters
branch
from
7a81b8e
to
d3928d7
Compare
Co-authored-by: Niko Oliveira <onikolas@amazon.com>
|
ECS System test is failing after this merge, the |
And this a bit strange, because after this logger record, as I could see there is no external calls (to AWS) and only simple validation which should finished in a milliseconds |
|
Yep locally it also checked forever I guess it is combination of airflow serializer, templated fields and multi inheritance (which I add for enums)
|
closes: #29556
Use custom botocore.Waiters instead of inner sensors in ECS Operators