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

ECS Service Placement Strategy Forcing Service Rebuild #11644

Closed
hikerspath opened this issue Feb 2, 2017 · 8 comments
Closed

ECS Service Placement Strategy Forcing Service Rebuild #11644

hikerspath opened this issue Feb 2, 2017 · 8 comments
Assignees

Comments

@hikerspath
Copy link

hikerspath commented Feb 2, 2017

We are observing a rebuild of ECS services that are making use of the new placement_strategy options. Basically seems to be a case-test issue if not something more. The app only accepts lowercase field values as your documentation was updated to state (#11338). However there appears to be a case mis-match that is causing the service to rebuild on a static (no changes to apply) run of terraform apply.

Terraform Version

$ terraform -v
Terraform v0.8.5

Affected Resource:

  • aws_ecs_service

Terraform Configuration:

resource "aws_ecs_service" "test" {
  name            = "test-ecs-service"
  cluster         = "${aws_ecs_cluster.main.id}"
  task_definition = "${aws_ecs_task_definition.test.arn}"
  desired_count   = 1
  iam_role        = "${aws_iam_role.ecs_service.name}"

  placement_constraints {
    type = "memberOf"
    expression = "attribute:ecs.availability-zone in [${var.availability_zones}]"
  }

  placement_strategy {
    type = "binpack"
    field = "memory"
  }

  load_balancer {
    target_group_arn = "${aws_alb_target_group.test.id}"
    container_name   = "${data.template_file.task_definition.vars.container_name}"
    container_port   = "REDACTED"
  }

  depends_on = [
    "aws_iam_role_policy.ecs_service",
    "aws_alb_listener.front_end",
  ]
}

Debug:

-/+ aws_ecs_service.test
    cluster:                                     "arn:aws:ecs:eu-west-1:__REDACTED__:cluster/test-ecs-cluster" => "arn:aws:ecs:eu-west-1::__REDACTED__:cluster/test-ecs-cluster"
    deployment_maximum_percent:                  "200" => "200"
    deployment_minimum_healthy_percent:          "100" => "100"
    desired_count:                               "1" => "1"
    iam_role:                                    "test-ecs-role" => "test-ecs-role"
    load_balancer.#:                             "1" => "1"
    load_balancer.547002511.container_name:      "oauth2-proxy" => "oauth2-proxy"
    load_balancer.547002511.container_port:      ":__REDACTED__" => ":__REDACTED__"
    load_balancer.547002511.elb_name:            "" => ""
    load_balancer.547002511.target_group_arn:    "arn:aws:elasticloadbalancing:eu-west-1::__REDACTED__:targetgroup/alb-ecs-test/427ac4d7c7b99332" => "arn:aws:elasticloadbalancing:eu-west-1::__REDACTED__:targetgroup/alb-ecs-test/:__REDACTED__"
    name:                                        "test-ecs-service" => "test-ecs-service"
    placement_constraints.#:                     "1" => "1"
    placement_constraints.2052447874.expression: "attribute:ecs.availability-zone in [eu-west-1a,eu-west-1b,eu-west-1c]" => "attribute:ecs.availability-zone in [eu-west-1a,eu-west-1b,eu-west-1c]"
    placement_constraints.2052447874.type:       "memberOf" => "memberOf"
    placement_strategy.#:                        "1" => "1"
    placement_strategy.3304928532.field:         "" => "memory" (forces new resource)
    placement_strategy.3304928532.type:          "" => "binpack" (forces new resource)
    placement_strategy.345693218.field:          "MEMORY" => "" (forces new resource)
    placement_strategy.345693218.type:           "binpack" => "" (forces new resource)
    task_definition:                             "arn:aws:ecs:eu-west-1::__REDACTED__:task-definition/test_td:6" => "arn:aws:ecs:eu-west-1::__REDACTED__:task-definition/test_td:6"

Expected Behavior

Should not destroy a service that is not technically changing anything (No terraform code was altered in the static run)

Note: If you remove the placement_strategy block and build, ensuing builds function as expected. It is only with the placement strategy that you see the failures noted.

Actual Behavior

Service is destroyed and re-created at every apply regardless of whether something in the service has changed.

Steps to Reproduce

  1. terraform apply (First to build the ECS service)
  2. terraform apply (Second time which should result in no changes)
@grubernaut
Copy link
Contributor

Hi @hikerspath, thanks for the issue!

This was recently fixed by @catsby in #11565, and should be included in the next minor version of Terraform (v0.8.6). Closing for now, but happy to discuss further!

@nytins
Copy link

nytins commented Feb 25, 2017

Hi @grubernaut
This issue still exist in Terraform v0.8.7

Snippet from the terraform plan

    placement_strategy.#:                "1" => "1"
    placement_strategy.1676812570.field: "instanceid" => "" (forces new resource)
    placement_strategy.1676812570.type:  "spread" => "" (forces new resource)
    placement_strategy.3946258308.field: "" => "instanceId" (forces new resource)
    placement_strategy.3946258308.type:  "" => "spread" (forces new resource)

@ligustah
Copy link

ligustah commented Mar 2, 2017

I'm seeing this as well. Is there any workaround until the bug is fixed (other than not using placement strategies at all) ?

@charlieoleary
Copy link
Contributor

Also seeing this in 0.8.7.

@eddgrant
Copy link

Seeing this in 0.8.8, based on the output I'm seeing (below) it appears that a unique placement_strategy id is being generated on every TF invocation, which is causing TF to think it needs to replace the aws_ecs_service.

placement_strategy.#:                      "1" => "1"
placement_strategy.1676812570.field:       "instanceid" => "" (forces new resource)
placement_strategy.1676812570.type:        "spread" => "" (forces new resource)
placement_strategy.3946258308.field:       "" => "instanceId" (forces new resource)
placement_strategy.3946258308.type:        "" => "spread" (forces new resource)

@eddgrant
Copy link

eddgrant commented Mar 13, 2017

Sorry, please disregard the above comment, I don't think it's particularly informative. I have just realised I had previously specified my field value as 'instanceId' (capitalised 'I'), this is correct as per the AWS docs. If I change this to 'instanceid' (all lower case) then plan correctly determines that no changes are to be made, however this doesn't fix the issue as, if I try to destroy and rebuild my environment from scratch, I get the this error:

InvalidParameterException: spread field 'instanceid' is invalid.

This suggests that AWS does care about the input being in the correct case which makes me think that 7e9bfda won't work in the event that the field value contains any uppercase characters as it simply lowercases one side of the equality argument.

I would offer to try and help resolve the issue but I don't (yet) know Go! Hope this helps someone get closer to a fix implementation though.

KensoDev added a commit to KensoDev/terraform that referenced this issue Mar 17, 2017
ECS has some incosistency in the way it sends down
field names and theway it expectes them in the api.

`MEMORY` and `CPU` are expected capitalized and `instanceId` should be sent as
is.

The previous fixed lowercased everything, meaning `instanceId` is being
saved as `instanceid` and causing a change in the resource.

PRs and issues

* hashicorp#12199
* hashicorp#11644
* hashicorp#11844
stack72 pushed a commit that referenced this issue Mar 23, 2017
AWS API requires ECS placement strategies "field" attribute to be
"memory" or "cpu" (lowercase) when type=bin, but these read back as
"MEMORY" and "CPU" (uppercase) respectively.

PR #11565 (which fixed separately reported #11644) deals with this by
always lowering the case of the resource received from the API, but this
breaks for other "field" values (e.g. "instanceId" -> "instanceid").

This PR only lowers the case of the returned resource when field
"MEMORY" or "CPU". Haven't checked if any other fields need this
treatment.
@stack72
Copy link
Contributor

stack72 commented Mar 23, 2017

Closed via #12998

@ghost
Copy link

ghost commented Apr 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants