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

Chart: Allow AWS ECS Executor #38524

Merged
merged 2 commits into from
Jun 5, 2024
Merged

Conversation

LipuFei
Copy link
Contributor

@LipuFei LipuFei commented Mar 27, 2024

Make it possible to use the alpha/experimental AWS ECS Executor with the Helm chart. (link: https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/executors/ecs-executor.html)

The changes are:

  1. Add the following executors into the allowed executor values:
    • airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor
    • airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor
  2. Truncate the executor label value to a max of 50 63 characters because Kubernetes doesn't allow a label value with more than 50 63 characters.
  3. Updated the Helm chart README.

I've tested this setup and it works. These are the minimum changes I need in the Helm chart.

chart/values.schema.json Outdated Show resolved Hide resolved
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

@LipuFei can you add tests for when this executor is set? Following LocalExecutor is probably a reasonable way to find place where cases should be added.

Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Putting a request change on this to ensure it doesn't get merged before a fix to shorten the aws module paths (#39093)

chart/README.md Outdated Show resolved Hide resolved
@LipuFei
Copy link
Contributor Author

LipuFei commented Apr 18, 2024

@LipuFei can you add tests for when this executor is set? Following LocalExecutor is probably a reasonable way to find place where cases should be added.

@jedcunningham I didn't create the AWS executors, so I probably can only create some very basic tests... Maybe @o-nikolas can help here?

@o-nikolas
Copy link
Contributor

Oops, my PR closed this one accidentally, re-opened!

@LipuFei
Copy link
Contributor Author

LipuFei commented May 2, 2024

Hi @eladkal @o-nikolas @jedcunningham , I have updated my changes and the PR description. Could you check it again? Thanks.

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Thanks @LipuFei. Can you add tests for this also? Might be worth running around looking at LocalExecutor cases, as that's the most similar from the charts perspective I think.

This test case should be updated as well. Can you harden it (make it fail if supported executors aren't all listed), or simplify it?

I expect we may need some rbac changes as well for these executors? Is that something you've investigated?

chart/README.md Outdated Show resolved Hide resolved
chart/templates/scheduler/scheduler-deployment.yaml Outdated Show resolved Hide resolved
@LipuFei LipuFei force-pushed the chart/allow-ecs branch 2 times, most recently from 8e2c35a to faa46e6 Compare May 13, 2024 09:07
@LipuFei
Copy link
Contributor Author

LipuFei commented May 13, 2024

Thanks @LipuFei. Can you add tests for this also? Might be worth running around looking at LocalExecutor cases, as that's the most similar from the charts perspective I think.

This test case should be updated as well. Can you harden it (make it fail if supported executors aren't all listed), or simplify it?

I expect we may need some rbac changes as well for these executors? Is that something you've investigated?

Hi @jedcunningham , I've updated the tests, including testing the supported executor.

For AWS ECS Executor, there's no need for RBAC changes. I haven't tested the Batch Executor, but I think it works in similar ways.

@LipuFei LipuFei force-pushed the chart/allow-ecs branch from faa46e6 to 2cda81f Compare May 13, 2024 09:27
@jedcunningham
Copy link
Member

For rbac I think thinking of this, but looks like we are good there.

@LipuFei
Copy link
Contributor Author

LipuFei commented May 28, 2024

I've updated this PR. All checks have passed.

From my point of view, this PR is ready for merge 😄 if there's no further comments...

@jedcunningham jedcunningham merged commit ef1c9a2 into apache:main Jun 5, 2024
69 checks passed
fdemiane pushed a commit to fdemiane/airflow that referenced this pull request Jun 6, 2024
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 1, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
@LipuFei LipuFei deleted the chart/allow-ecs branch August 16, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants