aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

feat(ecs): add `maxSwap` and `swappiness` properties to LinuxParameters

Open flavioleggio opened this issue 3 years ago • 13 comments

Add support to MaxSwap and Swappiness attributes in the LinuxParameters construct.

Closes #18460


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

flavioleggio avatar Jan 28 '22 00:01 flavioleggio

gitpod-io[bot] avatar Jan 28 '22 00:01 gitpod-io[bot]

Thanks for your review @madeline-k

  1. Have you deployed an ECS service with these parameters? and does it actually work as expected? I am asking because of these comments: #18460 (comment) and #18460 (comment). If there is some actions users need to take for this to work after deployment, we need to document it. Or, hopefully do it for them in the cfn template.

I deployed a test stack and the parameters were correctly defined in the resulting task definition. However, I had no chance to check if swap memory was available inside the container and I did not figure out how, actually. Maybe I should ssh into the cluster instance and exec a free command on the running test container?

  1. Please include an integration test.

I developed an integration test, but I have no permissions to create the VPC that I need for the ECS cluster in my company account, thus I cannot run the full integ test. What is the policy here? Should I commit the test without the corresponding .expected.json template? Wouldn't this make integ regression tests fail in CI?

flavioleggio avatar Feb 05 '22 19:02 flavioleggio

I developed an integration test, but I have no permissions to create the VPC that I need for the ECS cluster in my company account, thus I cannot run the full integ test. What is the policy here? Should I commit the test without the corresponding .expected.json template? Wouldn't this make integ regression tests fail in CI?

You're correct, you can't commit an integration test without the corresponding .expected.json template. Maybe I can help out here. Can you add the integration test to this PR? then I can run it and generate the expected.json template, and I'll push that to your branch.

madeline-k avatar Feb 23 '22 23:02 madeline-k

Ok, I’ll try to post it tomorrow

flavioleggio avatar Feb 23 '22 23:02 flavioleggio

// packages/@aws-cdk/aws-ecs/test/ec2/integ.swap-parameters.ts

import * as ec2 from '@aws-cdk/aws-ec2';
import * as cdk from '@aws-cdk/core';
import * as ecs from '../../lib';
import { LinuxParameters } from '../../lib';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'aws-ecs-integ');

const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 2 });

// ECS cluster to host EC2 task
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
cluster.addCapacity('DefaultAutoScalingGroup', {
  instanceType: new ec2.InstanceType('t2.micro'),
});

// define task to run the container
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDefinition', {
  networkMode: ecs.NetworkMode.AWS_VPC,
});

// define linux parameters to enable swap
const linuxParameters = new LinuxParameters(stack, 'LinuxParameters', {
  maxSwap: 5e3,
  swappiness: 90,
});

// define container with linux parameters
new ecs.ContainerDefinition(stack, 'Container', {
  image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
  linuxParameters,
  memoryLimitMiB: 256,
  taskDefinition,
});

// define a service to run the task definition
new ecs.Ec2Service(stack, 'Service', {
  cluster,
  taskDefinition,
});

app.synth();

flavioleggio avatar Feb 25 '22 09:02 flavioleggio

@flavioleggio Apologies for the long wait on this. I've run the test that you wrote and pushed the result to an expected file. Please take a look and ensure that this matches what you expect to see as output. Go ahead and assign this to me as a reviewer if everything looks good to you and the checks all pass.

TheRealAmazonKendra avatar Jul 14 '22 15:07 TheRealAmazonKendra

@flavioleggio Well, this is my bad. I didn't notice that the integration test was the old style and not the new. We'll need to update to the new style. I'll take care of that and run them for the expected output within the next week.

TheRealAmazonKendra avatar Jul 28 '22 19:07 TheRealAmazonKendra

@Mergifyio update

TheRealAmazonKendra avatar Jul 30 '22 20:07 TheRealAmazonKendra

update

✅ Branch has been successfully updated

mergify[bot] avatar Jul 30 '22 20:07 mergify[bot]

@Mergifyio update

TheRealAmazonKendra avatar Aug 10 '22 03:08 TheRealAmazonKendra

update

✅ Branch has been successfully updated

mergify[bot] avatar Aug 10 '22 03:08 mergify[bot]

@mrgrain Can you take another look? I've re-updated the tests.

TheRealAmazonKendra avatar Aug 10 '22 04:08 TheRealAmazonKendra

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation avatar Aug 31 '22 12:08 aws-cdk-automation

Hi @TheRealAmazonKendra

I updated the integration test snapshot with the new version and fulfilled your request on the parameter type. I think this is ready for approval.

flavioleggio avatar Sep 05 '22 12:09 flavioleggio

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify[bot] avatar Sep 06 '22 02:09 mergify[bot]

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3b2fb8c6384b63cd399eb6e187ab42ceac431116
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation avatar Sep 06 '22 03:09 aws-cdk-automation

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify[bot] avatar Sep 06 '22 03:09 mergify[bot]