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

Support container dependency in ecs-params.yml #1105

Merged
merged 4 commits into from
Oct 31, 2020

Conversation

perfectayush
Copy link
Contributor

@perfectayush perfectayush commented Sep 18, 2020

Added support for a depends_on field for services in ecs-params.yml
This field maps to dependsOn field in task definition.

Tries to address #897 . Not supporting via docker-compose.yml's depends_on because there is no 1:1 mapping between ECS and docker-compose concepts. Also it makes more sense to leverage ECS healthchecks condition.


Testing

  • All unit tests passed locally except for "github.com/aws/amazon-ecs-cli/ecs-cli/modules/cli/regcreds", but it's failing on mainline as well. Travis CI tests are passing including regcreds.
  • Integration tests passed
  • Unit tests added for new functionality
  • Manual testing steps
  • Link to issue or PR for the integration tests:

Documentation

  • Contacted our doc writer
  • Updated our README

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

@perfectayush
Copy link
Contributor Author

Manual testing

Configuration

# docker-compose.yml 
version: '2'
services:
  web:
    image: ubuntu:18.04
    command: /bin/bash -c 'trap "echo exiting web" EXIT; sleep 10; echo running web; while true; do echo `date` web; sleep 30; done '
    mem_limit: 128m
    mem_reservation: 128m
    logging:
      driver: awslogs
      options:
        awslogs-group: xxxxxxxxx
        awslogs-region: xxxxxxxxx
        awslogs-stream-prefix: xxxxxxxxx
  nginx:
    image: ubuntu:18.04
    command: /bin/bash -c 'trap "echo exiting nginx" EXIT; sleep 10; echo running nginx; while true; do echo `date` nginx; sleep 30; done '
    mem_limit: 128m
    mem_reservation: 128m
    logging:
      driver: awslogs
      options:
        awslogs-group: xxxxxxxxx
        awslogs-region: xxxxxxxxx
        awslogs-stream-prefix: xxxxxxxxx
# ecs-params.yml
version: 1
task_definition:
  task_role_arn: xxxxxxxxxxxxxx
  task_execution_role: xxxxxxxxxxxxxx
  ecs_network_mode: awsvpc
  task_size:
    cpu_limit: 256
    mem_limit: 512
  services:
    web:
      essential: true
      depends_on:
        - container_name: nginx
          condition: HEALTHY
    nginx:
      essential: true
      healthcheck:
        test: ["CMD-SHELL", "echo success"]
        interval: 5s
        timeout: 10s
        retries: 2
        start_period: 30s
run_params:
  network_configuration:
    awsvpc_configuration:
      subnets:
        - xxxxxxxxxxx
      security_groups:
        - xxxxxxxxxxx

Test command

Service was setup using:

ecs-cli compose --debug -f docker-compose.yml -p test --ecs-params ecs-params.yml service up --deployment-max-percent 100 --deployment-min-healthy-percent 0 --launch-type FARGATE

Things tested:

  1. Launch of dependent service happens only once service is ascertained to be healthy
  2. Shutdown order is reverse of start order as defined by dependency

@bpottier
Copy link

Is there any progress on this?

@perfectayush
Copy link
Contributor Author

@bpottier if you require to use it, you can compile from this branch. We are using this patch in production without issues.

Looks like this project has been abandoned in favor of co-pilot. So not much active people from aws for an upstream merge.

@bpottier
Copy link

@bpottier if you require to use it, you can compile from this branch. We are using this patch in production without issues.

Looks like this project has been abandoned in favor of co-pilot. So not much active people from aws for an upstream merge.

Thank you. I might just do that. Shame that support has dropped off in favor of co-pilot which isn't even production ready.

@perfectayush
Copy link
Contributor Author

Thank you. I might just do that. Shame that support has dropped off in favor of co-pilot which isn't even production ready.

Well that's just nature of software. AWS will have their valid reasons. Let's be grateful that this source code is open and API for this was easy to extend. :)

@bpottier
Copy link

Thank you. I might just do that. Shame that support has dropped off in favor of co-pilot which isn't even production ready.

Well that's just nature of software. AWS will have their valid reasons. Let's be grateful that this source code is open and API for this was easy to extend. :)

Yeah, you're right.

@efekarakus
Copy link
Contributor

Hello 👋 !!!
AWS Copilot can be safely used for building and managing production ready containerized applications on Amazon ECS and AWS Fargate. We haven't introduced any breaking changes in the CLI unless it was for a bug fix. And we should have a stable release soon 🙂
However, for the future on using Docker Compose as the entry point for your development workflow we recommend checking out what our friends at Docker are doing with the ECS+Compose integration! For leaving feedback, please create an issue in the Docker Compose repository, they'd really appreciate it.

@allisaurus allisaurus self-assigned this Oct 30, 2020
Copy link
Contributor

@allisaurus allisaurus left a comment

Choose a reason for hiding this comment

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

LGTM! thanks very much for this contribution @perfectayush and apologies for the delay in reviewing it!

While @efekarakus comment re: the future of AWS Copilot and the ECS +Compose integrations holds true, we still intend to facilitate community updates like this.

@mergify mergify bot merged commit bbd651c into aws:mainline Oct 31, 2020
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.

4 participants