ECS placement constraints/strategy #1059

ejholmes · 2017-03-14T01:16:25Z

This adds support to the procfile for specifying ECS placement constraints and placement strategies.

I cherry-picked #941 into this, since it makes it easier, so may be better to only look at the second commit.

ejholmes · 2017-03-14T02:28:20Z

One thing worth mentioning is that, I can see cases where you might want these to be runtime configurable per environment. For example, you might have a CPU intensive task that you want to run on a c4.8xlarge in production, but then only run on a t2.micro in staging.

I think, in these cases, the best thing to do is setup "named profiles" using container instance attributes, that map to their production/staging equivalents. For example, in the scenario above, I might add a profile=cpu attribute to container instances, where in production it's backed by c4.8xlarge, but only t2.micro in staging. Then I can use that profile in the Procfile:

web:
  ecs:
    placement_constraints:
      - { type: memberOf, expression: "attribute:profile = cpu" }

phobologic

When I saw this last night that was going to be my first comment - it seems likely that we'd want to control placement with config, rather than with the code.

I think your method for dealing with that is probably a good one, especially since it keeps things simple.

Only suggestion here is to at least update the Procfile documentation (btw, I don't think we have a Procfile specific doc - if not, its probably getting complicated enough it deserves it's own page :)).

Could you also put your suggestion for managing environment specific placements in that doc?

phobologic · 2017-03-14T16:54:19Z

server/cloudformation/ports.go

-	default:
-		err = fmt.Errorf("%s is not supported", req.RequestType)
-	}
+func (p *InstancePortsResource) Create(_ context.Context, req customresources.Request) (string, interface{}, error) {


Man - does your linter not freak out at you for not having comments describing exported methods? It gets annoying - I'm wondering if I should just find a way to disable it, or if we should try and get better about listening to it.

Yeah, I disabled that a long time ago because you just end up with a bunch of Thing does thing doc comments to appease the linter.

ejholmes · 2017-03-14T22:24:06Z

Good call. I'll update the procfile doc with information on that. The Procfile isn't documented in docs/ but there's a readme in ./procfile where all the options are documented, which the docs point to.

ejholmes · 2017-03-15T04:45:45Z

Another thing to think about here is that, if you're including a bunch of different instance types in a single ECS cluster, you probably want services that don't specify any placement constraints to default to some constraints, so they don't go bouncing around a bunch of different instance types.

Some kind of first class "profile" support, that maps to some ECS placement constraints might be a better way to go initially. So, when booting Empire, you'd give it a number of named "profiles", which map to ECS placement constraints, and then you specify a default profile. For example:

{
  "default": {
    "placement_constraints": [
      { "type": "memberOf", "expression": "attributes:ecs.instance-type = m3.*" }
    ]
  },
  "memory": {
    "placement_constraints": [
      { "type": "memberOf", "expression": "attributes:ecs.instance-type = x1.*" }
    ]
  }
}

Users would be able to define what "profile" to use for a process in the Procfile:

worker:
  profile: memory

Or they could change this at runtime:

$ emp scale worker --profile memory -a <app>

Or provide it via emp run:

$ emp run ./bin/job --profile memory -a <app>

Having a simple "named profile" provides users with a lot of simplicity, but still gives operators enough power to define as many profiles as needed, and document those within their org.

Food for thought 🤔

ejholmes · 2017-08-04T04:58:31Z

Alright, I'm feeling pretty good about this one now. I'll start doing some more testing, and giving it a whirl on our staging env.

phobologic

This looks good - it's a big change (seems like there was a bit of cleanup in this PR as well?).

phobologic · 2017-08-04T16:49:47Z

cmd/empire/factories.go

+			return nil, fmt.Errorf("unable to unmarshal placement constraints: %v", err)
+		}
+		log.Println(fmt.Sprintf("  DefaultPlacementConstraints: %v", placementConstraints))
+		return twelvefactor.Transform(s, setDefaultPlacementConstraints(placementConstraints)), nil


Man, so totally unrelated to this review in particular, but this finally helped me nail down why I really dislike the GoLang pattern of using 1-2 character variables. Take s for example here. In the review, I can't see what s is without expanding the lines above twice. If it had been named scheduler, I wouldn't even have to expand it, it would have made perfect sense in it's own context, which makes for easier code reviews. Python has it right ;)

That said, this overall method is getting a little long. Worth refactoring to break it up into smaller methods?

Yeah, I can see how that could make reviewing difficult. I tend to just follow the language communities preference for things like this. In the case of Go: https://github.com/golang/go/wiki/CodeReviewComments#receiver-names.

In regards to breaking it up, there's not much within this that's re-usable, so breaking it up might make it hard to follow.

Yeah, I don't get their dislike of descriptive names. Since when did we run out of bytes for descriptive code?! :)

phobologic · 2017-08-04T16:57:51Z

cmd/empire/main.go

@@ -334,6 +335,12 @@ var EmpireFlags = []cli.Flag{
 		EnvVar: "EMPIRE_ECS_DOCKER_CERT_PATH",
 	},
 	cli.StringFlag{
+		Name:   FlagECSPlacementConstraintsDefault,
+		Value:  "",
+		Usage:  "ECS placement constraints to set when a process does not set any.",


Could you include a "If this is not set, then this will happen" bit here?

Yep. I'll expand on this before it gets merged.

phobologic · 2017-08-04T17:00:55Z

runner.go

-	proc := Process{
-		Quantity: 1,
-	}
+	var proc Process


same question about this method - it's getting a little long, worth breaking it up?

phobologic · 2017-08-04T17:06:38Z

scheduler/docker/docker.go

-	}); err != nil {
-		return fmt.Errorf("error pulling image: %v", err)
-	}
+	for _, p := range app.Processes {


This seems like a pretty big change - there will only ever be a single process, right? Is this just short circuiting needing the p since that's already represented as an array in app.Processes and it only has a single value when Run is called?

I kinda addressed this below, but the overall change is basically a noop, since Empire core only ever passes 1 process, so the end result is the same. It does mean that Empire core could pass multiple processes to run them at once, but primarily this was done just to simplify the method signature since the manifest already contains all the process information.

ejholmes · 2017-08-09T00:13:38Z

@phobologic I just rebased this into smaller logical commits if it makes reviewing a little easier. The actual change to implement ECS placement constraints and strategies is in the last commit, which is relatively small. Commits 1-4 probably don't need too much attention, since they're mostly internal changes to make the last commit easier.

The first commit is just Move ECSService and InstancePort resources to higher level interface. #941, which has already been approved/tested, so can probably be ignored.
The second commit should probably get some attention. It simplifies the scheduler.Run method to only take a Manifest instead of both a Manifest and a Process, since the Manifest already contains the list of processes to run. It also fixes a "bug" so that emp run takes stored process configuration (e.g. size constraints) into account when running named procs.
Commits 3 just cleans up some legacy issues with scheduler.Restart. I probably shouldn't include it in this PR.

I'll follow up on the rest of the review comments once we've had a chance to test this in our staging env.

phobologic

Looked at commit 5 and yeah, looks good.

ejholmes · 2017-08-09T06:34:57Z

scheduler/cloudformation/template.go

+					"Expression": c.Expression,
+				})
+			}
+			serviceProperties["PlacementConstraints"] = placementConstraints


This should actually be added to the task definition, rather than the service. Reason being, this isn't updatable on a service, which would require a replacement of the service. If we do it on the task definition, then no replacement required. Placement strategies can only be applied to the service, so that would need to remain.

…visioner. Previously, these resources managed the logic for performing resource replacement themselves. After this change, they are based on the `privisioner` type, which makes it simpler to determine what requires a replacement by using a hash.

This commit simplifies the `twelvefactor.Scheduler.Run` interface method to only take a `twelvefactor.Manifest` instead of both a `twelvefactor.Manifest` and `twelvefactor.Process`. The manifest contains the processes that should be run. The commit also changes the `emp run` functionality, so that when a "named" command is run, it will use the stored process configuration (e.g. constraints) instead of the defaults.

This commit simplifies the `twelvefactor.Scheduler.Restart` method to only pass the App ID, instead of a full manifest. The existing implementation required the full manifest for the legacy ECS scheduler, which is no longer required in the CloudFormation world.

This commit adds a `twelvefactor.Transform` method, which returns a wrapped `twelvefactor.Scheduler` that will transform the `twelvefactor.Manifest` before passing it to the downstream scheduler. This can be used to, for example, add default placement constraints to processes that don't define any.

This commit adds support for specifying ECS placement constraints and placement strategies in the extended Procfile format.

ejholmes · 2017-08-10T02:04:56Z

This is working pretty nicely in our staging environment. I'll go ahead and pull this in and fix any issues the crop up before the next release.

phobologic approved these changes Mar 14, 2017

View reviewed changes

ejholmes force-pushed the placement branch from 6a59c02 to a9b3cba Compare July 25, 2017 01:54

ejholmes force-pushed the placement branch 2 times, most recently from c0dca8e to bbf2b3b Compare August 4, 2017 04:56

phobologic approved these changes Aug 4, 2017

View reviewed changes

ejholmes force-pushed the placement branch from bbf2b3b to 6b21790 Compare August 9, 2017 00:06

ejholmes added the status/3-docs-review label Aug 9, 2017

phobologic approved these changes Aug 9, 2017

View reviewed changes

ejholmes commented Aug 9, 2017

View reviewed changes

ejholmes force-pushed the placement branch from 537490d to 1afd535 Compare August 10, 2017 01:36

ejholmes added 6 commits August 9, 2017 18:55

Adds support for ECS placement constraints and strategies.

3944dcb

This commit adds support for specifying ECS placement constraints and placement strategies in the extended Procfile format.

Adds docs about ECS specific configuration in the Procfile.

c6d149c

ejholmes force-pushed the placement branch from 1afd535 to c6d149c Compare August 10, 2017 01:59

ejholmes merged commit 914b5dc into master Aug 10, 2017

ejholmes deleted the placement branch August 10, 2017 02:05

ejholmes mentioned this pull request Sep 19, 2017

v0.13.0 #1107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECS placement constraints/strategy #1059

ECS placement constraints/strategy #1059

ejholmes commented Mar 14, 2017

ejholmes commented Mar 14, 2017 •

edited

Loading

phobologic left a comment

phobologic Mar 14, 2017

ejholmes Mar 14, 2017

ejholmes commented Mar 14, 2017

ejholmes commented Mar 15, 2017

ejholmes commented Aug 4, 2017

phobologic left a comment

phobologic Aug 4, 2017

phobologic Aug 4, 2017

ejholmes Aug 9, 2017

phobologic Aug 9, 2017

phobologic Aug 4, 2017

ejholmes Aug 9, 2017

phobologic Aug 4, 2017

phobologic Aug 4, 2017

ejholmes Aug 9, 2017

ejholmes commented Aug 9, 2017 •

edited

Loading

phobologic left a comment

ejholmes Aug 9, 2017

ejholmes commented Aug 10, 2017

ECS placement constraints/strategy #1059

ECS placement constraints/strategy #1059

Conversation

ejholmes commented Mar 14, 2017

ejholmes commented Mar 14, 2017 • edited Loading

phobologic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejholmes commented Mar 14, 2017

ejholmes commented Mar 15, 2017

ejholmes commented Aug 4, 2017

phobologic 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

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

ejholmes commented Aug 9, 2017 • edited Loading

phobologic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejholmes commented Aug 10, 2017

ejholmes commented Mar 14, 2017 •

edited

Loading

ejholmes commented Aug 9, 2017 •

edited

Loading