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

AWS Batch JobQueue does not provide computeResources required for managed ComputeEnvironment #6615

Closed
ayush987goyal opened this issue Mar 7, 2020 · 5 comments · Fixed by #6616
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@ayush987goyal
Copy link
Contributor

ayush987goyal commented Mar 7, 2020

AWS Batch JobQueue does not provide computeResources required for managed ComputeEnvironment. Since JobQueue does not require the user to provide a computeEnvironmentOrder, the default ComputeEnvironment created by it should provide some computeResources as required by ComputeEnvironment.

Need to be fixed after the change in #6549

Reproduction Steps

  1. Create a JobQueue
  2. Try to deploy.

Error Log

You will get the following error

computeResources is missing but required on a managed compute environment

Environment

  • CLI Version : 1.27.0
  • Framework Version:
  • OS :
  • Language :

Other


This is 🐛 Bug Report

@andrestone
Copy link
Contributor

Hmm.. I see no solution other than creating an "unmanaged" computeEnvironment on the JobQueue constructor or make it a required property. Actually I see the latter case as more appropriate.

Defining default computeResources makes no sense to me.

Thoughts?

@ayush987goyal
Copy link
Contributor Author

ayush987goyal commented Mar 7, 2020

Yes I agree. Default computeResource is very rare and would only be used when the compute defaults actually match to what you would need (but maybe this is actually pretty common?). I think we can make computeEnvironments a required prop to the queue.

If not, we can accept vpc as a prop and create a default computeEnvironement that the ComputeEnvironment constructor creates (that also just accepts a vpc and nothing else as required props)

@iliapolo
Copy link
Contributor

iliapolo commented Mar 7, 2020

I agree that making the computeEnvironments property required at this stage is better. It makes more sense to me for any use case that isn't just sanity testing. Maybe we can think of a default compute environment later on. And even then I would probably prefer having something like ComputeEnvironment.lotsOfMemory() to help users, rather than implicitly creating the defaults.

@andrestone Feel like whipping up a PR?

@ayush987goyal
Copy link
Contributor Author

While we are at it, can we also add a small overview page with content similar to what I am including in example usage of batch with sfn here.

@iliapolo
Copy link
Contributor

iliapolo commented Mar 7, 2020

@ayush987goyal You're right. It was an oversight in the initial PR that introduced these L2's.

@andrestone Has already added some content to the page with the launch template support PR.

I wouldn't want to overwhelm this PR with adding all this content right now. I will create a separate issue for completing the README with all the necessary examples and explanations.

Sound good?

@mergify mergify bot closed this as completed in #6616 Mar 9, 2020
mergify bot pushed a commit that referenced this issue Mar 9, 2020
Fixes: #6615

BREAKING CHANGE: `computeEnvironments` is now required
eladb pushed a commit that referenced this issue Mar 9, 2020
Fixes: #6615

BREAKING CHANGE: `computeEnvironments` is now required
horsmand pushed a commit to horsmand/aws-cdk that referenced this issue Mar 9, 2020
)

Fixes: aws#6615

BREAKING CHANGE: `computeEnvironments` is now required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
3 participants