Add deferrable mode in RedshiftPauseClusterOperator #28850

pankajastro · 2023-01-11T08:56:34Z

Add a deferrable param in RedshiftPauseClusterOperator. This will help to run RedshiftPauseClusterOperator operator in async/deferrable mode.

Currently, AWS does not maintain the async version of boto3 so I need to depend on aiobotcore an async version for AWS botocore. Unfortunately, aiobotocore depend on botocore>=1.27.59,<1.27.60 while boto3 depend on botocore>=1.29.78,<1.30.0 and boto3 is core dependency in our provider so we can not add aiobotcore in core dependency.

In this PR, I'm adding aiobotocore as an additional dependency until aio-libs/aiobotocore#976 has been resolved.

I'm adding AwsBaseAsyncHook a basic async AWS hook. This will at present support the default botocore auth i'e if airflow connection is not provided then auth using ENV and if airflow connection is provided then basic auth with secret-key/access-key-id/profile/token and arn-method. maybe we can support the other auth incrementally depending on the community interest.

Because the dependency making things a little bit complicated so I have created a new test module deferrable inside AWS provider tests and I'm keeping the async-related test in this particular module. I have also added a new CI job to test/run the AWS deferable operator tests and I'm ignoring the deferable tests in other CI job test runs.

Add a trigger class which will wait until the Redshift pause request reaches a terminal state i.e paused or fail, We also have retry logic like the sync operator.

This PR donates the following developed RedshiftPauseClusterOperatorAsync` in astronomer-providers repo to apache airflow.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

airflow/providers/amazon/aws/hooks/base_aws.py

airflow/providers/amazon/aws/hooks/redshift_cluster.py

phanikumv

Documentation is missing for the new flag deferrable_mode

airflow/providers/amazon/aws/hooks/base_aws.py

Taragolis · 2023-01-11T10:46:27Z

airflow/providers/amazon/provider.yaml

@@ -52,6 +52,7 @@ dependencies:
  - apache-airflow>=2.3.0
  - apache-airflow-providers-common-sql>=1.3.1
  - boto3>=1.24.0
+  - aiobotocore>=2.1.1


I think add this as core dependency for amazon-provider could broke some existed functionality.
At least until aiobotocore resolve this issue aio-libs/aiobotocore#976 and remove upper bound of botocore.

Because botocore is a main package for boto3 and if we can't upgrade both of them we can't add new features which introduced since botocore 1.28.0+

Changelogs:

botocore

boto3

hmm, how we can avoid this conflict?
should we keep this optional and move related imports to blocks where it getting used 🤷‍♂️ ?
once we will conclude on this I'll address the other comments

@kaxil @uranusjr @ashb any thoughts on this?

Make it an optional requirement

Maybe add a comment linking to that issue and a note to change it to a dependency once that is sorted?

Yeah we could catch that module not installed and raise appropriate exception, for example

airflow/airflow/providers/slack/notifications/slack_notifier.py

Lines 26 to 31 in a7e1cb2

try:

from airflow.notifications.basenotifier import BaseNotifier

except ImportError:

raise AirflowOptionalProviderFeatureException(

"Failed to import BaseNotifier. This feature is only available in Airflow versions >= 2.6.0"

)

But also need somehow resolve additional issues:

AFAIK in CI image only core providers dependencies installed (i guess we have some workaround). That mean we have an error in async Amazon providers tests because there is no aiobotocore

If we install aiobotocore in CI image than we also freeze version of botocore and boto3. As result we could miss if our tests not work with modern version of boto3 (we have some private method usage in Amazon Provider)

I'm not sure how to resolve this, may be @potiuk could suggest something

@potiuk WDYT ^^?

Sorry - I totally missed that. Yes we have full support for that:

raise AirflowOptionalProviderFeatureException as mentioned above

Add "additional-extra" in the amazon provider - same as pandas we already have there: https://github.com/apache/airflow/blob/main/airflow/providers/amazon/provider.yaml#LL342-L597C23

AFAIK in CI image only core providers dependencies installed (i guess we have some workaround). That mean we have an error in async Amazon providers tests because there is no aiobotocore

Yes. It is quite a bummer that aiobotocore limits botocore that much. Indeed if aiobotocore will be additional extra and will be missing in the image. We could add it, but as @Taragolis mentions, it would limit our botocore upgrades that happen regularly. A solution to that might be:

add exclusion in the tests when iobotocore is installed

implement an extra job in CI that installs iobotocore and then runs only the iobotocore tests.

Should be easy to do.

I have excluded the test from the regular provider CI job and added a ci. job to run aiobotocore-related tests. can you please re-review it. thanks

airflow/providers/amazon/aws/hooks/base_aws.py

airflow/providers/amazon/aws/hooks/redshift_cluster.py

airflow/providers/amazon/aws/hooks/base_aws.py

tests/providers/amazon/aws/triggers/test_redshift_cluster.py

airflow/providers/amazon/aws/triggers/redshift_cluster.py

pankajastro · 2023-02-24T06:55:09Z

Requesting re-review on this. Thank you!

tests/providers/amazon/aws/utils/compat.py

pankajastro · 2023-02-27T16:50:57Z

Hi @Taragolis can you please re-review it, thank you!

airflow/providers/amazon/provider.yaml

airflow/providers/amazon/aws/hooks/base_aws.py

Taragolis · 2023-02-27T19:47:13Z

.github/workflows/ci.yml

+  tests-aws-async-provider:
+    timeout-minutes: 50
+    name: "Pytest for AWS Async Provider"
+    runs-on: "${{needs.build-info.outputs.runs-on}}"
+    needs: [build-info, wait-for-ci-images]
+    if: needs.build-info.outputs.run-tests == 'true'


I guess it will run even if no changes in AWS Provider

yes, what we should do here?

I think we should we run only on Amazon changes, I guess @potiuk might suggest as original autor of selective check for providers

Taragolis · 2023-02-27T19:48:12Z

.github/workflows/ci.yml

+      - name: "Run AWS Async Test"
+        run: "breeze shell \
+        'pip install aiobotocore>=2.1.1 && pytest /opt/airflow/tests/providers/amazon/aws/deferrable'"


All other settings would not apply for this run, include collect warnings.
I do not know is it required right now or not

right, but I'm also not sure if that require right now.

I guess when this PR would be ready for merge it would be solved?

Taragolis · 2023-02-27T19:51:27Z

airflow/providers/amazon/provider.yaml

@@ -596,3 +597,4 @@ additional-extras:
  - name: pandas
    dependencies:
      - pandas>=0.17.1
+      - aiobotocore>=2.1.1


In additional if user post-install this dependency then step should be

remove boto3 and botocore

Install aiobotocore with appropriate version of boto3 and botocore

Otherwise this error/warning happen

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts. boto3 1.26.79 requires botocore<1.30.0,>=1.29.79, but you have botocore 1.27.59 which is incompatible.

do you mean in CI we should first remove botocore/boto3 and then install aiobototcore?
yes, the above warning/error will come but since I'm running a single command to install just one package aiobotocore it will install it. let me know if you want I can add step in CI to remove botocore/boto3 and then install the aiobotocore

Not only in CI, we should mention about this in documentation otherwise we have tons of issues/discussion opened by users who want to use deferrable AWS operators and now have potentially broken system.

Taragolis · 2023-02-27T19:56:43Z

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

+        deferrable: bool = False,
+        poll_interval: int = 10,


Just wondering how we would resolve in case if we want use more generic parameters wait_for_completion as well as waiter_delay and waiter_max_attempts.

@ferruzzi @vincbeck Are any plans for migrate other AWS Operators to use waiters?

Are any plans for migrate other AWS Operators to use waiters?

Personally, I think I'd like to see all operators which implement a wait_for_completion block to use a waiter (either an official one or a custom one) but it's a matter of time. I am working on moving the EMR operators over as I get time but it's not a huge priority (yet?) and maybe it's better suited to a community project with a discussion to track the progress like we've done in the past?

For the first part, I don't know offhand but @syedahsn is working on deferrable stuff on our end and might have a better answer there.

Any chance thatbotocore will have native asyncio support 🤔 Or it is just some kind of surprise 🤣

I can't speak for the botocore team on that, I don't have any contact with that team, so I wouldn't know any better than you on that one. I'm not aware of anything in that regard but I wouldn't really know. Syed is working on researching deferrable operators to start making the full suite of AWS operators deferrable though, so out of our team I'd say he's the closest to an expert on aiobotocore and related stuff.

It's been requested for a looong time: boto/botocore#458
We reached out to them and they said Botocore will eventually support async by default, but it isn't coming any time soon.

Yeah I think we do not have native support soon.
Just hope that point for some inside 🤣

Our team is currently working on implementing deferrable operators as well, and we're taking a slightly different approach. Rather than create async counterparts for all AWS hooks, we are planning to create an async_conn property similar to the existing conn property. The async_conn property will return a ClientCreatorContext object that can be used to create an aiobotocore client that can make async calls to the boto3 API. With this approach, we reduce a lot of complexity and code duplication by not having separate async hooks for every service. Additionally, we are focusing on using waiters to perform the polling operations so that most of the triggers will be in a standard format. The aiobotocore library has an AIOWaiter class that make asynchronous calls to the boto3 API which makes it very easy to implement most waiters. This also saves us from having to write custom describe calls for every service. We are currently reviewing the design, but will have a PR for the community shortly.

no time for review this one

kaxil

Need to add docs about the extra, and @potiuk can you verify the CI part please

kaxil · 2023-03-13T20:01:29Z

Merging this but will Jarek take the call on CI -- I don't fully like/agree with the CI steps but don't want to block this futher

potiuk · 2023-03-14T10:01:14Z

Merging this but will Jarek take the call on CI -- I don't fully like/agree with the CI steps but don't want to block this futher

Yeah. I completely missed the calling (when I was called 2 weeks ago) - sorry for that.

@pankajastro - can you please make a follow-up after this one (happy to halp to review and get it done).

I believe the right way to solve it will be to:

add aiobotocore to setup.py in this list (with the comment explaining in both - amazon provider.yaml and in the list, that the two shoudl be synchronized. We

# Dependencies needed for development only
devel_only = [

(unless it causes some conflicts, and then we can think what to do)

remove the dedicated job to run the tests (and check that in "Providers" section the deferrable tests are executed

Then it should work out-of-the-box and only when providers are modified.

@pankajastro - can you make a follow-up PR for that please?

pankajastro · 2023-03-14T10:07:58Z

Merging this but will Jarek take the call on CI -- I don't fully like/agree with the CI steps but don't want to block this futher

Yeah. I completely missed the calling (when I was called 2 weeks ago) - sorry for that.

@pankajastro - can you please make a follow-up after this one (happy to halp to review and get it done).

I believe the right way to solve it will be to:

add aiobotocore to setup.py in this list (with the comment explaining in both - amazon provider.yaml and in the list, that the two shoudl be synchronized. We
# Dependencies needed for development only
devel_only = [
(unless it causes some conflicts, and then we can think what to do)

remove the dedicated job to run the tests (and check that in "Providers" section the deferrable tests are executed

Then it should work out-of-the-box and only when providers are modified.

@pankajastro - can you make a follow-up PR for that please?

Thank you @potiuk for the feedback. sure, I'll file a PR for this soon.

potiuk · 2023-03-14T10:33:53Z

Hmm. Though I see (and remember now) there is this big issue with aiobotocore pinning botocore to specific version #30032 (comment) , so this is going to be more difficult. Stay tuned.

potiuk · 2023-03-14T12:41:14Z

I got a closer look at the problem and I think doing it the way I described in #28850 (comment) might be the right approach - let's see if others agree with it.

pankajastro · 2023-03-16T12:28:05Z

I got a closer look at the problem and I think doing it the way I described in #28850 (comment) might be the right approach - let's see if others agree with it.

Hi @potiuk I have cleanup up the CI part as suggested #30127 Now looks much better. can you please take a look

potiuk · 2023-03-16T13:44:02Z

Yep. Looks better:

Also as expected botocore/boto were downgraded in the process automatically in constraints:

https://github.com/apache/airflow/actions/runs/4436088571/jobs/7784082644?pr=30127

As unfortunate as it is, seems that currently we do not have any AWS providers that rely on features unavailable in this version and all tests pass, so having it in constraints as "reference" one is not a big issue.

The reall effect of doing it this way is simply slower pace of upgrading of the botocore/boto in our tests. Which is not as bad actually.

I thought about it and we could also make another change after #30127 is merged (@Taragolis @kaxil @o-nikolas ): We could add a single extra job (a little more complex than the one initially added by @pankajastro ) where we could start the image, remove aiobotocore, upgrade botocore and boto to latest compatible versions and run all amazon provider unit tests there (because of lack of aiobotocore, the tests for deferred operators would be skipped automatically)

This should be rather simiple to set-up and it would give us much more confidence that the operators we have not only work with the aiobotocore-compatible version but also with the "latest" version.

potiuk · 2023-03-16T14:42:51Z

This one shoudl do the trick (once succeeds) #30144

@pankajastro @Taragolis @kaxil @ferruzzi @o-nikolas @eladkal -> I think with additional tests on "latest" botocore/boto, we should be in a really good posistion, because all amazon-related PR will have to also pass non-deferrable tests for latest botocore/boto (so users will be safe to ypgrade to latest version of boto/botocore if needed).

boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Jan 11, 2023

pankajastro force-pushed the add_deferrable_mode branch from b27bcb1 to 473886d Compare January 11, 2023 08:57

phanikumv reviewed Jan 11, 2023

View reviewed changes

airflow/providers/amazon/aws/hooks/base_aws.py Outdated Show resolved Hide resolved

airflow/providers/amazon/aws/hooks/base_aws.py Outdated Show resolved Hide resolved

airflow/providers/amazon/aws/hooks/redshift_cluster.py Outdated Show resolved Hide resolved

phanikumv reviewed Jan 11, 2023

View reviewed changes

uranusjr reviewed Jan 11, 2023

View reviewed changes

airflow/providers/amazon/aws/hooks/base_aws.py Outdated Show resolved Hide resolved

Taragolis previously requested changes Jan 11, 2023

View reviewed changes

Taragolis reviewed Jan 11, 2023

View reviewed changes

tests/providers/amazon/aws/triggers/test_redshift_cluster.py Outdated Show resolved Hide resolved

vincbeck reviewed Jan 12, 2023

View reviewed changes

airflow/providers/amazon/aws/triggers/redshift_cluster.py Outdated Show resolved Hide resolved

kaxil mentioned this pull request Feb 2, 2023

Add deferrable BatchOperator #29300

Merged

pankajastro marked this pull request as ready for review February 14, 2023 13:35

pankajastro requested review from eladkal and o-nikolas as code owners February 14, 2023 13:35

pankajastro force-pushed the add_deferrable_mode branch from 473886d to deb74b8 Compare February 16, 2023 17:43

pankajastro marked this pull request as draft February 19, 2023 18:20

pankajastro force-pushed the add_deferrable_mode branch 3 times, most recently from 76ca0ce to 4d13c87 Compare February 21, 2023 09:46

pankajastro marked this pull request as ready for review February 23, 2023 22:57

pankajastro requested a review from ashb as a code owner February 23, 2023 22:57

phanikumv reviewed Feb 24, 2023

View reviewed changes

tests/providers/amazon/aws/utils/compat.py Outdated Show resolved Hide resolved

pankajastro force-pushed the add_deferrable_mode branch 2 times, most recently from 67db527 to b229b4b Compare February 27, 2023 16:49

pankajastro force-pushed the add_deferrable_mode branch 2 times, most recently from a656d11 to 39d30e6 Compare February 27, 2023 19:16

Taragolis reviewed Feb 27, 2023

View reviewed changes

pankajastro added 3 commits March 13, 2023 11:38

Add deferrable mode in RedshiftPauseClusterOperator

5469ad4

Apply review suggestion

4dd2f17

Remove init from async hook

a407e4c

pankajastro force-pushed the add_deferrable_mode branch from ae915af to a407e4c Compare March 13, 2023 06:08

Fix tests

a59aa0b

kaxil approved these changes Mar 13, 2023

View reviewed changes

pankajastro added 4 commits March 13, 2023 19:15

Add docs about extras

7508ad2

Fix docs

e7e1010

Fix docs

37c307a

Fix docs

1ba1df7

kaxil merged commit cf77c3b into apache:main Mar 13, 2023

kaxil deleted the add_deferrable_mode branch March 13, 2023 20:01

potiuk mentioned this pull request Mar 14, 2023

Add support for deferrable operators in AMPP #30032

Merged

pankajastro mentioned this pull request Mar 15, 2023

Remove redshift async test job #30127

Merged

eladkal mentioned this pull request Apr 2, 2023

Status of testing Providers that were prepared on April 02, 2023 #30427

Closed

64 tasks

pankajastro mentioned this pull request May 2, 2023

Add deferrable mode for S3KeySensor #31018

Merged

hussein-awala mentioned this pull request May 31, 2023

Add deferrable mode to EcsRunTaskOperator #31636

Closed

2 tasks

yjdlau mentioned this pull request Jun 27, 2023

Async S3 support needed #32174

Closed

2 tasks

abhishekbhakat mentioned this pull request Aug 9, 2023

Add AioRefreshableCredentials for AwsBaseHookAsync astronomer/astronomer-providers#1292

Closed

eladkal mentioned this pull request Nov 17, 2023

Implement login and logout in AWS auth manager #35488

Merged

	try:
	from airflow.notifications.basenotifier import BaseNotifier
	except ImportError:
	raise AirflowOptionalProviderFeatureException(
	"Failed to import BaseNotifier. This feature is only available in Airflow versions >= 2.6.0"
	)

Add deferrable mode in RedshiftPauseClusterOperator #28850

Add deferrable mode in RedshiftPauseClusterOperator #28850

Conversation

pankajastro commented Jan 11, 2023 • edited Loading

phanikumv left a comment

Choose a reason for hiding this comment

Taragolis Jan 11, 2023 • 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

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

pankajastro commented Feb 24, 2023

pankajastro commented Feb 27, 2023

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil left a comment • edited Loading

Choose a reason for hiding this comment

kaxil commented Mar 13, 2023

potiuk commented Mar 14, 2023

pankajastro commented Mar 14, 2023 • edited Loading

potiuk commented Mar 14, 2023

potiuk commented Mar 14, 2023

pankajastro commented Mar 16, 2023

potiuk commented Mar 16, 2023

potiuk commented Mar 16, 2023 • edited Loading

pankajastro commented Jan 11, 2023 •

edited

Loading

Taragolis Jan 11, 2023 •

edited

Loading

kaxil left a comment •

edited

Loading

pankajastro commented Mar 14, 2023 •

edited

Loading

potiuk commented Mar 16, 2023 •

edited

Loading