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

Custom::ECSTaskDefinition resource #935

Merged
merged 1 commit into from
Jul 20, 2016
Merged

Conversation

ejholmes
Copy link
Contributor

@ejholmes ejholmes commented Jul 15, 2016

Re-opening #859

Now that we have more than 1 app that's running close to the CloudFormation template size limit, probably a good time to tackle this again.

This is enabled only when setting the ECS_TASK_DEFINITION=custom environment variable, the default will remain AWS::ECS::TaskDefinition.

TODO

  • More tests
  • Handle ClientException: TaskDefinition is inactive
  • Add BoolValue type.

"request", req,
"response", resp,
)
logger.Info(ctx, "cloudformation.provision")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed logging the full request and response, since there might be plain text secrets in it. But we should at least log some information about the request/response.

}

if c.Essential != nil {
essential = aws.Bool(*c.Essential == "true")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could also be a real bool. Need a BoolValue type.

@ejholmes ejholmes force-pushed the custom-task-definition branch 3 times, most recently from bded2af to 203054f Compare July 19, 2016 00:43
@ejholmes
Copy link
Contributor Author

For an app that used to generate a ~420kb template, this drops the template size down to ~80k. It does make stack updates slower, but I think that could be improved by removing the process specific Custom::ECSEnvironment's and merging them into the task definition.

@ejholmes
Copy link
Contributor Author

To improve performance, we should be able to do the following:

  1. Start up multiple workers calling ReceiveMessage.
  2. Remove process specific Custom::ECSEnvironment and just add those environment variables to the task definition directly.
  3. Handle updates of Custom::ECSEnvironment so it doesn't create a new physical resource id if Properties and OldProperties are identical.
  4. Remove the self imposed 10 second tick

@ejholmes
Copy link
Contributor Author

ejholmes commented Jul 19, 2016

#940 will improve the performance of custom resources significantly, but it does open things up for being rate limited easily, which will cause a stack to rollback.

We should probably pass an ecs client that has more forgiving retries. But before we increase the retry limit, we should have a heartbeat back into the sqs queue to increase the message visibility, since task definition registration isn't necessarily idempotent.

@@ -187,19 +189,47 @@ func (t *EmpireTemplate) Build(app *scheduler.App) (*troposphere.Template, error
}

func (t *EmpireTemplate) addScheduledTask(tmpl *troposphere.Template, app *scheduler.App, p *scheduler.Process) (taskDefinition string) {
taskDefinitionType := taskDefinitionType(app)
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of this variable shadowing

@mwildehahn
Copy link
Contributor

looks good, some minor comments above

@ejholmes
Copy link
Contributor Author

Ok @mhahn, think I addressed all the comments.

@mwildehahn
Copy link
Contributor

👍

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.

None yet

2 participants