Refactor monolithic ECS Operator into Operators, Sensors, and a Hook #25413

ferruzzi · 2022-07-29T21:26:49Z

The current EcsOperator is monolithic, does not have a hook, nor any sensors. This PR adds an ECS hook, modifies the existing EcsOperator to use it, adds a handful of other operators and sensors. The functionality of the existing EcsOperator was changed as little as possible. A future refactor of that operator might not be a bad idea, but adding the hook and sensors is the main goal of this PR.

Other changes:

Moved some methods from the EcsOperator module into the new EcsHook module as that felt like a better home for them.
Removes some old code that has been deprecated for multiple major releases.
- airflow/contrib/operators/ecs_operator.py was deprecated over 2 years ago
- tests/providers/amazon/aws/operators/test_ecs_system.py is also a few years old and not a proper system test by current standards.
Deprecates the name EcsOperator in favor of EcsRunTaskOperator to match the standard naming conventions since there are now multiple ECS-related operators.

closes: #24826

ferruzzi · 2022-07-29T22:04:10Z

Looks like I missed removing a deprecation somewhere, looking into it.

ferruzzi · 2022-07-30T02:25:14Z

Sorry about that, all passing now. 👍

eladkal

There are several places where code is removed.
We should deprecate first

eladkal · 2022-07-30T03:27:34Z

airflow/contrib/operators/ecs_operator.py

@@ -1,30 +0,0 @@
-#


This file can be removed only in Airflow 3 as its part of Airflow core so we can not remove it yet.

But it's so olllllldddddddd. Git Blame shows that it was deprecated at least 2 years ago. If it needs to stay, I'll add it back.

When I revert this, what is the best way to handle it? Should I import all of the new ECS Operators fro this PR and add them to __all__, or only update the names of the ones which were already included in there?

Regardless contrib files cant not be removed. In this specific case we must be backward compatible till Airflow 3

As for your question - in this file you need to support only classes that existed. We dont add new functionality

I have an idea for the future. Should we move alll the deprecated/contrib operators from 1.10 out to a separate "apache-contrib" package (when we split out providers).

That will allow to get rid of the contrib, and get rid of many 1.10 deprecations out of airlfow main repo at least (it can still be a dependency of Airflow package so that it is installed automatically when airflow is installed). @ashb @kaxil WDYT?

@eladkal - @potiuk mentioned wanting to get a provider package release out and Amazon would be getting a major release, I'm hoping to get this in on that but I'm not really sure what needs to be done here now. Any help would be appreciated.

It will take me a few days to get to this

I can take this one. But you need to rebase @ferruzzi - this one has problems from few days back when we fixed Flask 2.2 compatibility:

ACK. ~~Will rebase and do a force-push.~~ Done.

Thanks to both of you for the help.

I was a bit confused on how that chain of depredations should play out, but now with #25543 I'm doubly confused on how the two will interact.

ferruzzi · 2022-08-02T16:56:46Z

There are several places where code is removed. We should deprecate first

Unless I made a mistake, I only removed code which has been deprecated for quite some time.

[edit]
Check me if I've missed something:

Deleted:

airflow/contrib/operators/ecs_operator.py. Was included in the first version of airflow.tests.deprecated_classes from 6/19/20 so deprecated over two years ago (Discussed above, reverting this change)
'airflow.providers.amazon.aws.operators.ecs.EcsOperator.execute._xcom_set`: "private" method replaced inline with xcom_push
ECSOperator deprecation class: git blame shows it has been deprecated for one major release on the provider package
ECSTaskLogFetcher deprecation class: same as above
ECSProtocol deprecation class: same as above
tests/providers/amazon/aws/operators/test_ecs_system.py - This is from before AIP-47 and not how system tests work now. Once these changes are merged a new AIP-47-compliant system test will be added, but this PR felt like it was getting a little out of hand without also making those changes.

Moved or Renamed and deprecated:

airflow.providers.amazon.aws.operators.ecs.EcsProtocol
airflow.providers.amazon.aws.operators.ecs.EcsTaskLogFetcher
airflow.providers.amazon.aws.operators.ecs.EcsOperator

Moved from operator to the new hook but not deprecated:

I feel these two are "private" methods, though they don't have the underscore. I can definitely add deprecation classes for them if you'd like.

airflow.providers.amazon.aws.operators.ecs.should_retry
airflow.providers.amazon.aws.operators.ecs.should_retry_eni

ferruzzi · 2022-08-09T17:05:58Z

Rebased and fixed (??) the deprecation notice. This passes testing locally, let's see if it works.

potiuk · 2022-08-09T17:54:32Z

If the tests pass, do you think we should be fine with releasing it in this upcoming wave of providers?

potiuk · 2022-08-09T17:55:13Z

If the tests pass, do you think we should be fine with releasing it in this upcoming wave of providers?

Ah yeah. I see the comment now :)

eladkal · 2022-08-09T18:02:09Z

If the tests pass, do you think we should be fine with releasing it in this upcoming wave of providers?

I wont have time today to review the code.. i didnt do full review when I left the comment.

If the PR is OK from your side dont let me get in the way :)
I can chime in tommorow if my review is required.

ferruzzi · 2022-08-09T18:06:27Z

Alright, it just went green, so at least the CI is happy.

@jarek - My main reason for wanting it in this release was because it's removing deprecated code. Based on a chat with @eladkal, it doesn't sound like he thinks that's such a big deal. If you both think that isn't a good enough reason to hold up the release then by all means push on without it.

Adressed

potiuk · 2022-08-09T19:43:29Z

Hmm. Some conflicts after merging #25543 - will you rebase @ferruzzi ?

…right?

ferruzzi · 2022-08-09T20:50:26Z

@potiuk Rebase done, passes all CI and static tests locally

zachliu · 2022-08-26T22:07:09Z

airflow/providers/amazon/aws/operators/ecs.py

+        self.aws_conn_id = kwargs.get('aws_conn_id', DEFAULT_CONN_ID)
+        self.region = kwargs.get('region')
+        super().__init__(**kwargs)


oh man, you passed all kwargs including the aws_conn_id to the BaseOperator
now 80% of my dags (i use EcsOperator heavily) are failing with

Broken DAG: [/usr/local/airflow/dags/my_dag.py] Traceback (most recent call last): File "/usr/local/airflow/.local/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 390, in apply_defaults result = func(self, **kwargs, default_args=default_args) File "/usr/local/airflow/.local/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 743, in __init__ raise AirflowException( airflow.exceptions.AirflowException: Invalid arguments were passed to EcsOperator (task_id: my-task). Invalid arguments were: **kwargs: {'aws_conn_id': 'aws_default'}

is there a fix? will 2 simple pops do the job?

nvm, #25989

ferruzzi requested a review from mik-laj as a code owner July 29, 2022 21:26

boring-cyborg bot added area:providers kind:documentation provider:amazon-aws AWS/Amazon - related issues labels Jul 29, 2022

eladkal previously requested changes Jul 30, 2022

View reviewed changes

vincbeck approved these changes Aug 2, 2022

View reviewed changes

ferruzzi force-pushed the ferruzzi/ecs-operator-cleanup branch from 9e07d5f to d1e27e0 Compare August 5, 2022 16:52

ferruzzi mentioned this pull request Aug 9, 2022

Prepare docs for new providers release (August 2022) #25618

Merged

ferruzzi force-pushed the ferruzzi/ecs-operator-cleanup branch from d1e27e0 to ea74ca3 Compare August 9, 2022 17:05

potiuk requested a review from eladkal August 9, 2022 17:53

potiuk approved these changes Aug 9, 2022

View reviewed changes

ferruzzi added 7 commits August 9, 2022 13:00

Refactor monolithic ECS Operator into Operators, Sensors, and a Hook

0e445bd

Added base classes to test_project_structure.py

709f871

Comma, comma, comma chameleon

6e16bc4

Revert removing airflow/contrib/operators/ecs_operator.py

b1a1a23

Revert removing airflow/contrib/operators/ecs_operator.py, but do it …

e35d7b1

…right?

Fix deprecations correctly, maybe

67eb8fd

Fix imports from a lazy rebase

bbd6efb

ferruzzi force-pushed the ferruzzi/ecs-operator-cleanup branch from ea74ca3 to bbd6efb Compare August 9, 2022 20:46

potiuk merged commit 8a1b7d4 into apache:main Aug 9, 2022

This was referenced Aug 10, 2022

Status of testing Providers that were prepared on August 10, 2022 #25634

Closed

Status of testing Providers that were prepared on August 10, 2022 #25640

Closed

ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 15, 2022

zachliu reviewed Aug 26, 2022

View reviewed changes

ephraimbuddy added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:misc/internal Changelog: Misc changes that should appear in change log labels Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor monolithic ECS Operator into Operators, Sensors, and a Hook #25413

Refactor monolithic ECS Operator into Operators, Sensors, and a Hook #25413

ferruzzi commented Jul 29, 2022

ferruzzi commented Jul 29, 2022

ferruzzi commented Jul 30, 2022

eladkal left a comment

eladkal Jul 30, 2022 •

edited

Loading

ferruzzi Aug 2, 2022

ferruzzi Aug 2, 2022

eladkal Aug 2, 2022 •

edited

Loading

potiuk Aug 2, 2022

ferruzzi Aug 5, 2022

eladkal Aug 5, 2022

potiuk Aug 5, 2022

ferruzzi Aug 5, 2022 •

edited

Loading

ferruzzi Aug 5, 2022

ferruzzi commented Aug 2, 2022 •

edited

Loading

ferruzzi commented Aug 9, 2022

potiuk commented Aug 9, 2022

potiuk commented Aug 9, 2022

eladkal commented Aug 9, 2022

ferruzzi commented Aug 9, 2022

potiuk commented Aug 9, 2022

ferruzzi commented Aug 9, 2022

zachliu Aug 26, 2022 •

edited

Loading

zachliu Aug 27, 2022

Refactor monolithic ECS Operator into Operators, Sensors, and a Hook #25413

Refactor monolithic ECS Operator into Operators, Sensors, and a Hook #25413

Conversation

ferruzzi commented Jul 29, 2022

ferruzzi commented Jul 29, 2022

ferruzzi commented Jul 30, 2022

eladkal left a comment

Choose a reason for hiding this comment

eladkal Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladkal Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferruzzi Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferruzzi commented Aug 2, 2022 • edited Loading

Deleted:

Moved or Renamed and deprecated:

Moved from operator to the new hook but not deprecated:

ferruzzi commented Aug 9, 2022

potiuk commented Aug 9, 2022

potiuk commented Aug 9, 2022

eladkal commented Aug 9, 2022

ferruzzi commented Aug 9, 2022

potiuk commented Aug 9, 2022

ferruzzi commented Aug 9, 2022

zachliu Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladkal Jul 30, 2022 •

edited

Loading

eladkal Aug 2, 2022 •

edited

Loading

ferruzzi Aug 5, 2022 •

edited

Loading

ferruzzi commented Aug 2, 2022 •

edited

Loading

zachliu Aug 26, 2022 •

edited

Loading