Various fixes on ECS run task operator #31838
Conversation
… waiting for completion
boring-cyborg
bot
added
area:providers
area:system-tests
provider:amazon-aws
| @staticmethod | ||
| def _get_ecs_task_id(task_arn: str) -> str: | ||
| return task_arn.split("/")[-1] |
There was a problem hiding this comment.
doing this because I thought that remembering to update the ecs_task_id every time the arn gets changed was a bit brittle. This method makes the dependency arn->task_id explicit.
Yes it means we're going to recompute it each time we need it, but I think it's negligible.
| # A bit brutal to delete the whole group, I know, | ||
| # but we don't have the access to the arn of the task which is used in the stream name | ||
| # and also those logs just contain "hello world", which is not very interesting. | ||
| client.delete_log_group(logGroupName=group_name) |
There was a problem hiding this comment.
this is going to fail if the group does not exist, so in a way it makes sure the log configuration stays correct.
| if not self.wait_for_completion: | ||
| return | ||
|
|
There was a problem hiding this comment.
Whatever logs users were getting for the short period of time without a wait_for_completion they will no longer get. So we're calling it a bug fix with no deprecation?
There was a problem hiding this comment.
Yeah, idk, we may want to keep the existing behavior, but what I don't like about it is that it made the operator slower just for the sake of maybe getting a couple of logs...
Since we were starting the thread, which slept for 30 seconds (or configured value) before checking if it was stopped, this operator would take 30 seconds to return no matter what, when the job was done in a second and a half.
It's like "don't wait for completion but still wait a bit"
There was a problem hiding this comment.
I'd agree with Raph, I dont think this is a desired behavior but more an forgotten edge case. I would call it a bug fix
There was a problem hiding this comment.
Ack, I'll call that quorum then, let's call it a bug fix 👍
| if not self.wait_for_completion: | ||
| return | ||
|
|
There was a problem hiding this comment.
I'd agree with Raph, I dont think this is a desired behavior but more an forgotten edge case. I would call it a bug fix
This method is just causing trouble by handling several things, it's hiding the logic. A bug fixed in apache#31838 was reintroduced in apache#31881 because the check that was skipped on `wait_for_completion` was not skipped anymore. The bug is that checking the status will always fail if not waiting for completion, because obviously the task is not ready just after creation.
This method is just causing trouble by handling several things, it's hiding the logic. A bug fixed in #31838 was reintroduced in #31881 because the check that was skipped on `wait_for_completion` was not skipped anymore. The bug is that checking the status will always fail if not waiting for completion, because obviously the task is not ready just after creation.
This method is just causing trouble by handling several things, it's hiding the logic. A bug fixed in apache#31838 was reintroduced in apache#31881 because the check that was skipped on `wait_for_completion` was not skipped anymore. The bug is that checking the status will always fail if not waiting for completion, because obviously the task is not ready just after creation.
fixing a bunch of problems around the ECS run task operator:
The system tests were not properly configured for logs too. I added the necessary configuration, a cleanup step, and moved the sensor to ecs_fargate so that we can have the normal behavior querying logs in the vanilla ecs test.