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

Bug in LoadBalancedEc2Service -- must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified #2263

Closed
Fejuto opened this issue Apr 14, 2019 · 3 comments · Fixed by #2463 · May be fixed by MechanicalRock/account-reaper#6
Assignees
Labels
bug This issue is a bug.

Comments

@Fejuto
Copy link

Fejuto commented Apr 14, 2019

memoryLimitMiB is not optional at the moment while it should be; validate doesn't check for memoryReservationMiB.

validate() {
        const ret = super.validate();
        if (util_1.isEc2Compatible(this.compatibility)) {
            // EC2 mode validations
            // Container sizes
            for (const container of this.containers) {
                if (!container.memoryLimitSpecified) {
                    ret.push(`ECS Container ${container.node.id} must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified`);
                }
            }
        }
        return ret;
    }
  • CDK Version 0.28
@Fejuto Fejuto added the bug This issue is a bug. label Apr 14, 2019
@SoManyHs
Copy link
Contributor

SoManyHs commented Apr 16, 2019

Hi @Fejuto,

Could you include the example you are trying to run? This validation exists in the task-definition.ts construct, but just want to be clear on your use case. Thanks!

@Fejuto
Copy link
Author

Fejuto commented Apr 16, 2019

    new ecs.LoadBalancedEc2Service(this, 'Service', {
      memoryReservationMiB: 7500,
      cluster: gameCluster.cluster,
      image: ecs.ContainerImage.fromAsset(this, 'Image', {
        directory: '../cosnode'
      })
    })

causes

C:\Users\Fejuto\Documents\git\idlegame\cdk\node_modules\@aws-cdk\cdk\lib\synthesis.ts:51
        throw new Error(`Validation failed with the following errors:\n  ${errorList}`);
              ^
Error: Validation failed with the following errors:
  [GameStack/Service/TaskDef] ECS Container web must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified
    at Synthesizer.synthesize (C:\Users\Fejuto\Documents\git\idlegame\cdk\node_modules\@aws-cdk\cdk\lib\synthesis.ts:51:15)
    at App.run (C:\Users\Fejuto\Documents\git\idlegame\cdk\node_modules\@aws-cdk\cdk\lib\app.ts:74:27)
    at process.App.process.once (C:\Users\Fejuto\Documents\git\idlegame\cdk\node_modules\@aws-cdk\cdk\lib\app.ts:51:45)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at process.emit (events.js:211:7)
    at process.emit (C:\Users\Fejuto\Documents\git\idlegame\cdk\node_modules\source-map-support\source-map-support.js:465:21)

@SoManyHs
Copy link
Contributor

SoManyHs commented May 2, 2019

Hi @Fejuto, sorry for the delay in response. This should be fixed in #2463. Thanks for your patience!

@SoManyHs SoManyHs changed the title Bug in task-definition.js -- must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified Bug in LoadBalancedEc2Service -- must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified May 2, 2019
rix0rrr pushed a commit that referenced this issue May 6, 2019
Previously, only `memoryLimitMiB` was being set in the constructor. This would cause an unexpected validation error when a LoadBalancedEcsService was instantiated with `memoryReservationMiB` only specified. This fix correctly sets both fields.

Fixes #2263
SanderKnape pushed a commit to SanderKnape/aws-cdk that referenced this issue May 14, 2019
)

Previously, only `memoryLimitMiB` was being set in the constructor. This would cause an unexpected validation error when a LoadBalancedEcsService was instantiated with `memoryReservationMiB` only specified. This fix correctly sets both fields.

Fixes aws#2263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment