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

Fargate Agent configuration options enhancements #1614

Merged
merged 8 commits into from
Oct 14, 2019

Conversation

mhmcdonald
Copy link

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • [x ] adds new tests (if appropriate)
  • [x ] updates CHANGELOG.md (if appropriate)
  • [x ] updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

This PR makes a few changes to the Fargate Agent. The first change is that it allows subnets and security groups to be set based on environment variables. The second change is that it adds two AWS configuration options to the Fargate Agent: task_role_arn and execution_role_arn.

Why is this PR important?

This PR is important because Fargate requires the task definition to have execution role ARN to support ECR images. These configurations are necessary for the IAM role of the agent to be properly set.

@codecov
Copy link

codecov bot commented Oct 14, 2019

Codecov Report

Merging #1614 into master will decrease coverage by 0.01%.
The diff coverage is 88.23%.

@joshmeek
Copy link

This is really great @mhmcdonald! I'm excited to get these extra options into the Fargate Agent!

mypy is failing for two minor reasons:

src/prefect/agent/fargate/agent.py:104: error: Item "None" of "Optional[str]" has no attribute "split"
src/prefect/agent/fargate/agent.py:112: error: Item "None" of "Optional[str]" has no attribute "split"

Comment on lines 91 to 95
if not self.execution_role_arn:
self.logger.exception(
"Fargate requires task definition to have execution role ARN to support ECR images"
)
raise Exception("Fargate task execution role required")

Choose a reason for hiding this comment

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

I don't think this Exception needs to be raised however a warning could still be logged. The reason being (for example) in my set up I am using images based in DockerHub and this execution_role_arn wasn't required.

Copy link
Author

@mhmcdonald mhmcdonald Oct 14, 2019

Choose a reason for hiding this comment

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

Sounds good @joshmeek - I'm happy to remove the Exception and just log the warning. The reason we did this was because without the execution_role_arn, the iam role for the agent wasn't being set properly and this fixed it. In your aws environment is an iam role being used for the fargate tasks? Is it somehow defaulting to a iam role? (Just curious). I will update shortly.

Choose a reason for hiding this comment

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

For mine I'm am actually not using an IAM role so using one may actually warrant the need of the execution_role_arn I will test various set ups / configurations and see if that will affect it. Thanks for the feedback 👍

src/prefect/agent/fargate/agent.py Outdated Show resolved Hide resolved
src/prefect/agent/fargate/agent.py Outdated Show resolved Hide resolved
@joshmeek joshmeek merged commit 524b527 into PrefectHQ:master Oct 14, 2019
zanieb pushed a commit that referenced this pull request Apr 13, 2022
UI: orion-design release 0.1.71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants