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: MultiNodeJobDefinition Incorrectly Requires InstanceType #27185

Closed
SeanStockwell opened this issue Sep 18, 2023 · 4 comments · Fixed by #27223
Closed

aws-batch: MultiNodeJobDefinition Incorrectly Requires InstanceType #27185

SeanStockwell opened this issue Sep 18, 2023 · 4 comments · Fixed by #27223
Labels
@aws-cdk/aws-batch Related to AWS Batch effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@SeanStockwell
Copy link

Describe the bug

It seems that the Batch MultiNodeJobDefinition CDK Construct requires instance type to be defined. However, instanceType is not required by the Batch AWS SDK.

This seems to be more than a documentation issue, as I'm unable to create a MultiNodeJobDefinition in my CDK package without specifying instance type.

This constraint takes away from one of the primary advantages to having Batch manage our EC2 resources in that they optimally assign batch jobs to instance types based on availability/resource requirements.

Expected Behavior

We shouldn't need to specify instance type. Batch should determine it based on job resource requirements.

Current Behavior

CDK requires instance type to be required at job definition level.

Reproduction Steps

Create a MultiNodeJobDefinition.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.96.2

Framework Version

No response

Node.js Version

18.x

OS

linux

Language

Typescript

Language Version

No response

Other information

No response

@SeanStockwell SeanStockwell added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 18, 2023
@github-actions github-actions bot added the @aws-cdk/aws-batch Related to AWS Batch label Sep 18, 2023
@indrora
Copy link
Contributor

indrora commented Sep 18, 2023

@indrora indrora added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 18, 2023
@SeanStockwell
Copy link
Author

The link you provided says that InstanceType is not required

@peterwoodworth
Copy link
Contributor

peterwoodworth commented Sep 18, 2023

Maybe @comcalvi can comment on why this is required in our construct, but as far as I can tell it isn't required in CloudFormation. I implemented this aspect on my stack and successfully deployed.

class BatchContainerDefinitionModifier implements cdk.IAspect {
  public visit(node: IConstruct): void {
    // will need to be stricter in search if you are creating other job definitions
    if (node instanceof batch.CfnJobDefinition) {
      node.addPropertyDeletionOverride('NodeProperties.NodeRangeProperties.0.Container.InstanceType')
      node.addPropertyDeletionOverride('NodeProperties.NodeRangeProperties.1.Container.InstanceType')
      // repeat for as many containers as necessary
    }
  }
}

cdk.Aspects.of(stack).add(new BatchContainerDefinitionModifier());

repro stack

    const def = new batch.MultiNodeJobDefinition(this, 'JobDefinition', {
      instanceType: new ec2.InstanceType('m5.large'),
    });

    def.addContainer({
      startNode: 0,
      endNode: 5,
      container: new batch.EcsEc2ContainerDefinition(this, 'multiContainer', {
        image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
        cpu: 256,
        memory: cdk.Size.mebibytes(2048),
      })
    });

    def.addContainer({
      startNode: 6,
      endNode: 10,
      container: new batch.EcsEc2ContainerDefinition(this, 'multiContainer2', {
        image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
        cpu: 256,
        memory: cdk.Size.mebibytes(2048),
      })
    });

@peterwoodworth peterwoodworth added p2 feature-request A feature should be added or improved. effort/small Small work item – less than a day of effort and removed bug This issue is a bug. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Sep 18, 2023
@mergify mergify bot closed this as completed in #27223 Sep 26, 2023
mergify bot pushed a commit that referenced this issue Sep 26, 2023
Make `instanceType` optional. It used to be required. It will default to a batch optimal instance type.

The breaking change is for `MultiNodeJobDefinition.instanceType`, which is now optional. This is not truly breaking because everyone has this property explicitly set now, and will continue to have it set after this change. 

Closes #27185.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-batch Related to AWS Batch effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants