Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for EKS operator #944

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

VolkerSchiewe
Copy link

@VolkerSchiewe VolkerSchiewe commented May 7, 2024

Description

We are using MWAA in combination with EKS so that all our dags in airflow are running in our EKS. We would like to use the same setup with cosmos.

What changes?

  • New EKSOperator classes (inheriting from KubernetesOperators) - Based on the original EksOperator
  • Tests
  • Adjusted documentation

Related Issue(s)

Breaking Change?

No - only an additional feature

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

@VolkerSchiewe VolkerSchiewe requested a review from a team as a code owner May 7, 2024 08:12
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 7, 2024
Copy link

netlify bot commented May 7, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 6faf960
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/664739102707d100088384f3

@dosubot dosubot bot added area:docs Relating to documentation, changes, fixes, improvement area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc area:testing Related to testing, like unit tests, integration tests, etc labels May 7, 2024
@tatiana tatiana added this to the 1.5.0 milestone May 7, 2024
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, @VolkerSchiewe !
Please, could you address the tests currently failing?

@VolkerSchiewe
Copy link
Author

Thanks for the contribution, @VolkerSchiewe ! Please, could you address the tests currently failing?

Hi @tatiana thanks for getting back to me! I was already working on it, but I ran into this issue: apache/airflow#39103

Seems to be affecting the latest version of the amazon airflow provider. I already tried with pinning an older version of xmlsec1 with no success :/

@tatiana
Copy link
Collaborator

tatiana commented May 10, 2024

Hi @VolkerSchiewe , just a head's up: we're wrapping up the Cosmos 1.4 release and either myself or @pankajkoti will support you on this next week!

@VolkerSchiewe
Copy link
Author

Looking in to the failing tests I'm not sure where the problems are coming from 🤔

Unit tests:
Seem to be mostly fine. The 1-2 tests that are failing might just be a version incompatibility. Maybe we can exclude this somehow in the compatibility matrix

Integration tests:
I think the tests that I added should not be executed in integration, but I think the imports are still executed and failing because I'm missing the amazon packages in the integration environment. Should I add a try ... except around the import to not import the package if it is not installed? How do you handle those cases normally?

Also let me know if the test coverage is enough. I tried to cover the parts that are special for the eks operator, but didn't want to copy everything from the KubernetesOperator since it's tested there already.

@pankajkoti
Copy link
Contributor

pankajkoti commented May 14, 2024

It appears that earlier versions of Flask are not compatible with newer versions of Jinja2; and hence we have the failure in our tests

These are the constraints for Airflow 2.3 which suggests Flask 1.1.2 and Jinja2 3.0.3, however dbt-core is upgrading it to jinja2-3.1.4 (Jinja2<4,>=3.1.3 (from dbt-core)) which is not supported on Flask 1.1.2 that comes with Airflow.

@VolkerSchiewe
Copy link
Author

Hey there, the only failing parts are the tests with Airflow 2.3 and python 3.9. For some reason the install step fails with

pip._vendor.resolvelib.resolvers.ResolutionTooDeep: 200000

when installing the test requirements . I don't really understand why and how to solve this. Is this problem you ran into earlier already?

@tatiana tatiana added the triage-needed Items need to be reviewed / assigned to milestone label May 17, 2024
@VolkerSchiewe
Copy link
Author

Fixing boto3 in the runs with airflow 2.3 solved the problem 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Relating to documentation, changes, fixes, improvement area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc area:testing Related to testing, like unit tests, integration tests, etc cosmos epic-assigned size:L This PR changes 100-499 lines, ignoring generated files. triage-needed Items need to be reviewed / assigned to milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants