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

Allow container mem_limit to be null #678

Merged
merged 1 commit into from
Dec 3, 2018
Merged

Allow container mem_limit to be null #678

merged 1 commit into from
Dec 3, 2018

Conversation

allisaurus
Copy link
Contributor

Previously, if no mem_limit was set on a container, the ecs-cli would default it to 512mb, even if task size memory was set. Additionally, if mem_reservation was set but mem_limit was not, the latter would be set to the same value.

This change leaves mem_limit unset (nil) in both cases.

Test 1: container mem_limit null when task level memory set

docker-compose.yml

version: '2'
services:
  wordpress:
    image: wordpress
    ports:
      - "80:80"

ecs-params.yml

version: 1
task_definition:
  ecs_network_mode: bridge
  task_size:
    cpu_limit: 250
    mem_limit: 1gb

CMD out put

$> ~\ecs-cli.exe compose create
INFO[0000] Using ECS task definition                     TaskDefinition="wordpress-test:6"

resulting task definition

{
  "containerDefinitions": [
    {
      "portMappings": [
        {
          "hostPort": 80,
          "protocol": "tcp",
          "containerPort": 80
        }
      ],
      "cpu": 0,
      "memory": null,
      "memoryReservation": null,
      "volumesFrom": [],
      "image": "wordpress",
      "essential": true,
      "name": "wordpress"
    }
  ],
  "memory": "1024",
  "compatibilities": [
    "EC2"
  ],
  "taskDefinitionArn": "arn:aws:ecs:us-west-2:##########:task-definition/wordpress-test:6",
  "family": "wordpress-test",
  "networkMode": "bridge",
  "cpu": "250",
  "revision": 6,
  "status": "ACTIVE",
}

Test 2: mem_reservation set but mem_limit not set

docker-compose.yml

version: '2'
services:
  wordpress:
    image: wordpress
    ports:
      - "80:80"

ecs-params.yml

version: 1
task_definition:
  ecs_network_mode: bridge
  services:
    wordpress:
      mem_reservation: 1gb

CMD output

$> ~\ecs-cli.exe compose create
INFO[0000] Using ECS task definition                     TaskDefinition="wordpress-test:8"

resulting task def

{
  "containerDefinitions": [
    {
      "portMappings": [
        {
          "hostPort": 80,
          "protocol": "tcp",
          "containerPort": 80
        }
      ],
      "cpu": 0,
      "memory": null,
      "memoryReservation": 1024,
      "image": "wordpress",
      "essential": true,
      "name": "wordpress"
    }
  ],
  "memory": null,
  "compatibilities": [
    "EC2"
  ],
  "taskDefinitionArn": "arn:aws:ecs:us-west-2:##########:task-definition/wordpress-test:8",
  "family": "wordpress-test",
  "networkMode": "bridge",
  "cpu": "250",
  "revision": 8,
  "status": "ACTIVE",
  "volumes": []
}

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

Copy link
Contributor

@SoManyHs SoManyHs left a comment

Choose a reason for hiding this comment

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

Code LGTM (modulo typo ;)! However could you update the commit message to include what you have in the PR description here? Also, I would change the commit title to be more explicit about memory limit not being set ("empty" confusing bc in go that means 0 for an int, not null).

E.g.

[Bugfix] Allow container memory limit to be null

Previously, if no mem_limit was set on a container, the ecs-cli would default it to 512mb, even if task size memory was set. Additionally, if mem_reservation was set but mem_limit was not, the latter would be set to the same value.

This change leaves mem_limit unset (nil) in both cases.

Resolves #570, #606

web := findContainerByName("web", containerDefs)

assert.NoError(t, err)
assert.Empty(t, web.Memory, "Expectd Memory to be nil")
Copy link
Contributor

Choose a reason for hiding this comment

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

EXPECT'D!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D:

Previously, if no mem_limt was set on a container, ecs-cli would default
it to 512mb, even if task size mem_limit was set. Additionally, if
mem_reservation only was set, mem_limit would be set to the same value.

This change leaves mem_limit unset (nil) in both cases.
Resolves aws#570, aws#606
@SoManyHs SoManyHs changed the title Allow empty mem_limit when mem_reservation or task_size.mem_limit is … Allow container mem_limit to be null Dec 3, 2018
@allisaurus allisaurus merged commit a31a2ca into aws:dev Dec 3, 2018
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.

2 participants