Ecs scheduling strategy #4825

erks · 2018-06-13T10:36:24Z

Fixes #4820
Depends on #4823

Changes proposed in this pull request:

add support for ECS schedulingStrategy

Output from acceptance testing: Handled via daily acceptance testing

christofferh · 2018-06-13T12:27:46Z

aws/resource_aws_ecs_service.go

@@ -321,6 +328,15 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
 		input.LaunchType = aws.String(v.(string))
 	}

+	if v, ok := d.GetOk("scheduling_strategy"); ok {


Should deploymentConfiguration be taken into account too?

If deploymentConfiguration is specified, the maximum percent parameter must be 100. The default value for a daemon service for maximumPercent is 100%. The default value for a daemon service for minimumHealthyPercent is 0% for the AWS CLI, the AWS SDKs, and the APIs, and 50% for the AWS Management Console.

https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs_services.html#service_scheduler_daemon

I was thinking about that too, but I'm not sure what the general handlings of this are in other places. I'm obviously relying on AWS doing the validation and returning the error for this. But if the best practice here is to do the validation on the terraform side, I can add it. Let me know.

The scheduling_strategy attribute is always going to exist since it has a Default -- you can use input.SchedulingStrategy = d.Get("scheduling_strategy").(string) instead of working with d.GetOk() or add it to the CreateEcsService struct directly with the same d.Get()

bflad · 2018-06-13T14:10:55Z

#4823 has been merged so this is good for rebasing 👍

erks · 2018-06-13T17:31:39Z

@bflad do you have any pointer here on how best I can ignore desired_count on create/update/read/etc when scheduling_strategy is DAEMON? Also, should we validate deploymentConfiguration according to https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs_services.html#service_scheduler_daemon ?

tomelliff · 2018-06-14T10:37:25Z

aws/resource_aws_ecs_service.go

@@ -17,6 +17,8 @@ import (
 	"github.com/hashicorp/terraform/helper/validation"
 )

+const schedulingStrategyDaemon = "DAEMON"


You should just be able to use ecs.SchedulingStrategyDaemon (and its equivalent ecs.SchedulingStrategyReplica) here instead.

tomelliff · 2018-06-14T10:50:37Z

aws/resource_aws_ecs_service_test.go

+  %s
+}
+`, clusterName, tdName, svcName, schedulingStrategySnippet, desiredCountSnippet)
+}


I waver on this a bit and am personally a little inconsistent but I think I prefer having a bit of repetition in the test config code rather than more complicated dynamic config, particularly when it involves something as slightly convoluted as this.

More of a thought/comment than a critique but I think I find the tests a nice form of documentation about how to use things/how things work but this is a bit harder to follow here.

I'll change it, as it seems other existing functions are also favoring repetition here.

tomelliff · 2018-06-14T10:51:00Z

website/docs/r/ecs_service.html.markdown

 * `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.
+* `scheduling_strategry` - (Optional) The scheduling strategy to use for the service. The valid values are `REPLICA` and `DAEMON`. Defaults to `REPLICA`.


nit: scheduling_strategry -> scheduling_strategy

tomelliff · 2018-06-14T10:57:17Z

aws/resource_aws_ecs_service.go

+				Optional: true,
+				ForceNew: true,
+				Default:  "REPLICA",
+			},


This isn't done consistently but I find https://github.com/terraform-providers/terraform-provider-aws/blob/41a726c851769570036f30cd83ffc2b68a47c8dd/aws/resource_aws_lb_listener.go#L50-L52 is a nice convenience for users.

Not sure if it's worth validating this so we can generate failures at plan time because it comes with the trade off that if AWS adds another scheduling strategy we need a code change and release to support it.

Will add the the StateFunc. Thanks for the advice @tomelliff .

Regarding the validation, that's also why I'm a bit hesitant about adding. I think it might come down to consistency, i.e. whether this provider performs validations like this somewhere else.

erks · 2018-06-18T23:02:13Z

@bflad any input on the validation question(s) above? or any overall feedback. happy to address them, if any, so we can move forward.

tomelliff · 2018-06-19T06:27:36Z

I wouldn't bother validating the deployment configuration. You can't test for it at plan time and the AWS API should return a suitable error that explains what's wrong if it fails.

That said, if for some reason the AWS API error message isn't clear (some of them can be a bit opaque) then it might be worth throwing an error or testing for the error code and providing a different error message.

bflad

Hi @erks and @tomelliff -- I left some initial feedback/questions below. Please let us know if you have any questions or do not have time to implement the feedback.

bflad · 2018-06-19T19:44:01Z

website/docs/r/ecs_service.html.markdown

 * `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.
+* `scheduling_strategy` - (Optional) The scheduling strategy to use for the service. The valid values are `REPLICA` and `DAEMON`. Defaults to `REPLICA`.


We should add this note from the API documentation: Fargate tasks do not support the DAEMON scheduling strategy.

bflad · 2018-06-19T19:45:42Z

aws/resource_aws_ecs_service_test.go

+				Config: testAccAWSEcsServiceWithDefaultSchedulingStrategy(clusterName, tdName, svcName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSEcsServiceExists("aws_ecs_service.ghost", &service),
+					resource.TestCheckResourceAttr("aws_ecs_service.ghost", "scheduling_strategy", "REPLICA"),


Instead of a whole new acceptance test and test config for the default value, you can just add this attribute check to the _basic test 👍

Did you mean _basicImport? I can't find the _basic test here. Although _basicImport doesn't seem to be the right place.

Ah sorry, this resource's testing could use some standardization with everything else. The "basic" test is currently TestAccAWSEcsService_withARN (you'll notice the seemingly odd check for service_registries.# in there - it should really have all the resource attribute default checks).

bflad · 2018-06-19T19:47:53Z

aws/resource_aws_ecs_service.go

+				Optional: true,
+				ForceNew: true,
+				Default:  ecs.SchedulingStrategyReplica,
+				StateFunc: func(v interface{}) string {


Instead of using StateFunc, we should instead prefer to validate the input value with case sensitivity using the below ValidateFunc:

ValidateFunc: validation.StringInSlice([]string{ ecs.SchedulingStrategyDaemon, ecs.SchedulingStrategyReplica, }, false),

bflad · 2018-06-19T19:55:56Z

aws/resource_aws_ecs_service.go

@@ -489,7 +508,11 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error {
 		d.Set("task_definition", taskDefinition)
 	}

-	d.Set("desired_count", service.DesiredCount)


What value for service.DesiredCount gets returned when in DAEMON mode? Is it 0 or the number of container instances? We should generally try to call d.Set() if possible.

In my testing, it's the number of container instances, which can change periodically.

Adding the following DiffSuppressFunc to the desired_count, deployment_minimum_percent, and deployment_maximum_percent attributes, we can continue to always call d.Set("desired_count", service.DesiredCount) (along with the others) and remove the conditional in the read function. This will remove the breaking change for outputs that depend on the desired_count attribute always existing.

DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { if d.Get("scheduling_strategy").(string) == ecs.SchedulingStrategyDaemon { return true } return false },

bflad · 2018-06-19T19:59:29Z

aws/resource_aws_ecs_service.go

@@ -709,7 +732,9 @@ func resourceAwsEcsServiceUpdate(d *schema.ResourceData, meta interface{}) error
 		Cluster: aws.String(d.Get("cluster").(string)),
 	}

-	if d.HasChange("desired_count") {


If the DesiredCount returning from the API is nil or 0 when in DAEMON mode, we should be able to leave the old logic here. 👍

In my testing, it's the number of container instances.

bflad · 2018-06-19T20:03:33Z

aws/resource_aws_ecs_service.go

@@ -321,6 +328,15 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
 		input.LaunchType = aws.String(v.(string))
 	}

+	if v, ok := d.GetOk("scheduling_strategy"); ok {


The scheduling_strategy attribute is always going to exist since it has a Default -- you can use input.SchedulingStrategy = d.Get("scheduling_strategy").(string) instead of working with d.GetOk() or add it to the CreateEcsService struct directly with the same d.Get()

bflad · 2018-06-21T03:58:10Z

aws/resource_aws_ecs_service_test.go

+]
+DEFINITION
+}
+resource "aws_ecs_service" "ghost" {


This test resource needs to reduce the deployment_maximum_percent from the default of 200:

=== RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- FAIL: TestAccAWSEcsService_withDaemonSchedulingStrategy (5.71s) testing.go:518: Step 0 error: Error applying: 1 error(s) occurred: * aws_ecs_service.ghost: 1 error(s) occurred: * aws_ecs_service.ghost: InvalidParameterException: The daemon scheduling strategy does not support values above 100 for maximumPercent on a deployment configuration. Change the maximumPercent value to 100, and try again

e.g. deployment_maximum_percent = 100

It might also be worth noting this in the attribute documentation and/or adding a daemon example to the documentation.

done and added the doc/example.

This acceptance test is still failing:

=== RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- FAIL: TestAccAWSEcsService_withDaemonSchedulingStrategy (5.99s) testing.go:518: Step 0 error: Error applying: 1 error(s) occurred: * aws_ecs_service.ghost: 1 error(s) occurred: * aws_ecs_service.ghost: InvalidParameterException: Both maximumPercent and minimumHealthyPercent cannot be 100 as this will block deployments. status code: 400, request id: d5daf50b-7949-11e8-a362-3ff63025fa34 "tf-acc-svc-w-ss-daemon-8yeejuf7"

By changing the create function to never include DeploymentConfiguration when set in DAEMON scheduling strategy, it then received this error during acceptance testing:

=== RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- FAIL: TestAccAWSEcsService_withDaemonSchedulingStrategy (12.56s) testing.go:579: Error destroying resource! WARNING: Dangling resources may exist. The full state and error is shown below. Error: Error applying: 1 error(s) occurred: * aws_ecs_service.ghost (destroy): 1 error(s) occurred: * aws_ecs_service.ghost: InvalidParameterException: The daemon scheduling strategy does not support a desired count for services. Remove the desired count value and try again status code: 400, request id: d91107b6-794b-11e8-b194-535f28f11fde

The deletion function tries to drain any ACTIVE status services by setting their DesiredCount to 0 before deletion. Ignoring DAEMON scheduling strategy for that call, the testing passes as expected:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsService_withDaemonSchedulingStrategy' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withDaemonSchedulingStrategy -timeout 120m === RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (15.51s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 15.549s

Fixes hashicorp#4820

TheSimpleZ · 2018-06-26T12:28:27Z

Hi! What is the status on this PR? Is it going to be merged anytime soon or should i find a workaround for setting up daemons in aws?

bflad · 2018-06-26T13:42:16Z

We should be able to get this released tomorrow, more shortly.

bflad

Thanks so much for your work here @erks! This was most of the way there but it turns out needed some additional work to properly support the full Terraform resource lifecycle as found via the acceptance testing while also continuing to support desired_count in outputs. To get this functionality moved ahead, I added an additional commit after yours to finish up the working implementation: aca44f4

Additional comments provided for context. 🚀

21 tests passed (all tests)
=== RUN   TestAccAWSEcsServiceDataSource_basic
--- PASS: TestAccAWSEcsServiceDataSource_basic (12.26s)
=== RUN   TestAccAWSEcsService_basicImport
--- PASS: TestAccAWSEcsService_basicImport (12.67s)
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (13.32s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (15.50s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (15.61s)
=== RUN   TestAccAWSEcsService_withReplicaSchedulingStrategy
--- PASS: TestAccAWSEcsService_withReplicaSchedulingStrategy (15.63s)
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (21.71s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (31.14s)
=== RUN   TestAccAWSEcsService_withDaemonSchedulingStrategy
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (33.46s)
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (33.62s)
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (33.66s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (41.34s)
=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (43.46s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (47.66s)
=== RUN   TestAccAWSEcsService_withServiceRegistries_container
--- PASS: TestAccAWSEcsService_withServiceRegistries_container (47.53s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (63.54s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (71.76s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (132.42s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (224.11s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (244.02s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (248.50s)

bflad · 2018-06-26T14:11:47Z

aws/resource_aws_ecs_service.go

@@ -489,7 +508,11 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error {
 		d.Set("task_definition", taskDefinition)
 	}

-	d.Set("desired_count", service.DesiredCount)


Adding the following DiffSuppressFunc to the desired_count, deployment_minimum_percent, and deployment_maximum_percent attributes, we can continue to always call d.Set("desired_count", service.DesiredCount) (along with the others) and remove the conditional in the read function. This will remove the breaking change for outputs that depend on the desired_count attribute always existing.

DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { if d.Get("scheduling_strategy").(string) == ecs.SchedulingStrategyDaemon { return true } return false },

bflad · 2018-06-26T14:36:03Z

aws/resource_aws_ecs_service_test.go

+]
+DEFINITION
+}
+resource "aws_ecs_service" "ghost" {


This acceptance test is still failing:

=== RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- FAIL: TestAccAWSEcsService_withDaemonSchedulingStrategy (5.99s) testing.go:518: Step 0 error: Error applying: 1 error(s) occurred: * aws_ecs_service.ghost: 1 error(s) occurred: * aws_ecs_service.ghost: InvalidParameterException: Both maximumPercent and minimumHealthyPercent cannot be 100 as this will block deployments. status code: 400, request id: d5daf50b-7949-11e8-a362-3ff63025fa34 "tf-acc-svc-w-ss-daemon-8yeejuf7"

By changing the create function to never include DeploymentConfiguration when set in DAEMON scheduling strategy, it then received this error during acceptance testing:

=== RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- FAIL: TestAccAWSEcsService_withDaemonSchedulingStrategy (12.56s) testing.go:579: Error destroying resource! WARNING: Dangling resources may exist. The full state and error is shown below. Error: Error applying: 1 error(s) occurred: * aws_ecs_service.ghost (destroy): 1 error(s) occurred: * aws_ecs_service.ghost: InvalidParameterException: The daemon scheduling strategy does not support a desired count for services. Remove the desired count value and try again status code: 400, request id: d91107b6-794b-11e8-b194-535f28f11fde

The deletion function tries to drain any ACTIVE status services by setting their DesiredCount to 0 before deletion. Ignoring DAEMON scheduling strategy for that call, the testing passes as expected:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsService_withDaemonSchedulingStrategy' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withDaemonSchedulingStrategy -timeout 120m === RUN TestAccAWSEcsService_withDaemonSchedulingStrategy --- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (15.51s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 15.549s

bflad · 2018-06-26T14:37:01Z

aws/resource_aws_ecs_service_test.go

+  cluster = "${aws_ecs_cluster.default.id}"
+  task_definition = "${aws_ecs_task_definition.ghost.family}:${aws_ecs_task_definition.ghost.revision}"
+  scheduling_strategy = "DAEMON"
+  deployment_maximum_percent = 100


Instead of requiring a special configuration for min/max, we'll setup the resource to properly ignore those configurations and differences 👍

erks · 2018-06-26T16:35:42Z

thanks, @bflad

bflad · 2018-06-27T14:06:41Z

This has been released in version 1.25.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-05T17:11:53Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost added size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Jun 13, 2018

christofferh reviewed Jun 13, 2018

View reviewed changes

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/ecs Issues and PRs that pertain to the ecs service. labels Jun 13, 2018

erks force-pushed the ecs_scheduling_strategy branch from 54ca210 to 0fb8f5f Compare June 13, 2018 15:59

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Jun 13, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 13, 2018

tomelliff reviewed Jun 14, 2018

View reviewed changes

erks force-pushed the ecs_scheduling_strategy branch from 0502e9d to a546ea6 Compare June 14, 2018 15:31

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 14, 2018

bflad reviewed Jun 19, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 19, 2018

erks force-pushed the ecs_scheduling_strategy branch from a546ea6 to 8f0f728 Compare June 19, 2018 20:37

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 19, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 21, 2018

bflad reviewed Jun 21, 2018

View reviewed changes

Touch Ungboriboonpisal added 5 commits June 21, 2018 00:22

aws_ecs_service: add support for scheduling_strategy

fc1679d

Fixes hashicorp#4820

auto ignore desired count for DAEMON scheduling strategy

7b0975d

update tests

6b5fa9e

do not require desired_count in DAEMON scheduling strategy

81e7818

code review feedback

fdcbea6

erks force-pushed the ecs_scheduling_strategy branch from 8f0f728 to fdcbea6 Compare June 21, 2018 07:22

ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 21, 2018

bflad added this to the v1.25.0 milestone Jun 26, 2018

bflad approved these changes Jun 26, 2018

View reviewed changes

bflad merged commit fdcbea6 into hashicorp:master Jun 26, 2018

bflad added a commit that referenced this pull request Jun 26, 2018

Update CHANGELOG for #4825

1665116

erks deleted the ecs_scheduling_strategy branch June 26, 2018 16:35

ghost locked and limited conversation to collaborators Apr 5, 2020

		* `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.
		* `scheduling_strategry` - (Optional) The scheduling strategy to use for the service. The valid values are `REPLICA` and `DAEMON`. Defaults to `REPLICA`.

		* `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.
		* `scheduling_strategy` - (Optional) The scheduling strategy to use for the service. The valid values are `REPLICA` and `DAEMON`. Defaults to `REPLICA`.

Ecs scheduling strategy #4825

Ecs scheduling strategy #4825

Conversation

erks commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jun 13, 2018

erks commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erks Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

erks commented Jun 18, 2018

tomelliff commented Jun 19, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erks Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erks Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSimpleZ commented Jun 26, 2018

bflad commented Jun 26, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erks commented Jun 26, 2018

bflad commented Jun 27, 2018

ghost commented Apr 5, 2020

erks Jun 14, 2018 •

edited

Loading

tomelliff commented Jun 19, 2018 •

edited

Loading

erks Jun 19, 2018 •

edited

Loading

erks Jun 19, 2018 •

edited

Loading