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

Script to check for worker capacity on cluster #110

Merged
merged 12 commits into from
Feb 10, 2017
Merged

Conversation

emilymdubois
Copy link
Contributor

@emilymdubois emilymdubois commented Feb 8, 2017

Refs #105

Initializing a PR for the worker capacity on cluster script.

  • Get cluster ARN given stack's name and region
  • Query worker capacity for each of the EC2s


/* Get the CloudFormation template */
var cloudformation = new AWS.CloudFormation({ region: argv.region });
cloudformation.describeStacks({ StackName: argv.stack }, function(err, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually doesn't always work. Most of the time our stacks use a parameter, but you can make a watchbot stack that doesn't.

Instead of describeStacks you should make a getTemplate API request. Use that to look up the Cluster that the Watcher is configured to launch its service into

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rclark yeah, I tried getTemplate and when I specified both Original and Processed, I never was able to access a cluster ARN, just a {"Ref": "Cluster"} 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example of the kind of request I'm making:

var AWS = require('aws-sdk');
var cloudformation = new AWS.CloudFormation({ region: argv.region });
cloudformation.getTemplate({
  StackName: argv.stack,
  TemplateStage: 'Processed'
}, function(err, res) {
  if (err) return callback(new Error(err));
  console.log(res.TemplateBody); // can't find cluster ARN, just references to it
});

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah 🤔

How about we add an output to watchbot.template() that we know will provide the ARN via describeStacks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rclark I pushed up a stop-gap that evaluates if .WatchbotService.Properties.Cluster is a string or object. If it's a string, return that; if it's an object, make a cloudformation.describeStacks request to get the parameter value for the reference variable. I think the risk, though, is that the cluster ARN may not be a stack parameter (e.g., it could be resolved through .Mappings), so I think the .Outputs method might be more reliable here 👍

var cloudformation = new AWS.CloudFormation({ region: argv.region });
cloudformation.describeStacks({ StackName: argv.stack }, function(err, res) {
if (err) return callback(new Error(err));
return callback(null, res.Stacks[0].Parameters.find(function(p) { return p.ParameterKey === 'Cluster' }).ParameterValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

These array manipulation functions are fun places to use => functions because the syntax is so brief:

.Parameters.find(function(p) { return p.ParameterKey === 'Cluster' })

could be

.Parameters.find((p) => p.ParameterKey === 'Cluster')

@jqtrde
Copy link
Contributor

jqtrde commented Feb 9, 2017

this will be so nice!

@emilymdubois
Copy link
Contributor Author

@rclark & @jakepruitt, I've got an initial pass at a capacity script. Here's what it does:

  1. Fetches the cluster ARN from the given stack's .Outputs property.
  2. Fetches the worker memory and CPU reservations from a cloudformation.getTemplate API call.
  3. For each instance in the cluster, fetches the memory and CPU .remainingResources.
  4. Determines how many workers could fit on each EC2 based on memory and based on CPU availability and requirements. The lowest value of the two is taken per instance, and summed together.

Do you see any issues with this approach before I write out tests?

Copy link
Contributor

@rclark rclark left a comment

Choose a reason for hiding this comment

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

Fetches the worker memory and CPU reservations from a cloudformation.getTemplate API call.

This could also fall victim to being a { "Ref": "SomethingElse" } rather than a number. What you'd really want to do is know the ARN of the worker's TaskDefinition, and make an ecs.describeTaskDefinitions request for that.

@emilymdubois
Copy link
Contributor Author

@rclark thanks for the feedback! Would you mind giving this a review?

One thing I noticed while testing is that a stack update alone didn't expose the cluster ARN via the Outputs property; I needed to delete and re-create my stack. Not sure if that's something worth mentioning in the readme, or is understood knowledge about CloudFormation.

Copy link
Contributor

@rclark rclark left a comment

Choose a reason for hiding this comment

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

Yeah CloudFormation will not perform an update if it doesn't do anything except add a stack output.

lib/capacity.js Outdated
var workerMemory = resources[i].find((e) => { return e.name === 'MEMORY'; }).integerValue / reservations.Memory;
taskCapacities.push(Number(Math.min(workerCpu, workerMemory).toFixed(0)));
}
return taskCapacities.reduce((a, b) => { return a + b; });
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to take the reduce journey a step further, you can avoid having 2 loops by either:

  • return Object.keys(resource).reduce(function(free, key) { ...
  • have the listInstances function return an array instead of an object indexed by instance id

@emilymdubois emilymdubois changed the title [wip] Script to check for worker capacity on cluster Script to check for worker capacity on cluster Feb 10, 2017
@emilymdubois
Copy link
Contributor Author

emilymdubois commented Feb 10, 2017

@rclark thanks for the review! Made some updates to make the listInstances and calculateRoom functions a little cleaner, and fixed a test bug.

Do you know of a way to make this logic a little cleaner with sinon functions? I tried a few different ways to return different callbacks on the first and second stub calls, but couldn't quite get it right.

@rclark
Copy link
Contributor

rclark commented Feb 10, 2017

I think the full-blown sinon-ified method would look like:

var describe = AWS.stub('ECS', 'describeContainerInstances');
describe.onCall(0).yields(null, fixtures.describeContainerInstances0);
describe.onCall(1).yields(null, fixtures.describeContainerInstances1);

Does that work?

@emilymdubois
Copy link
Contributor Author

emilymdubois commented Feb 10, 2017

@rclark, ah that does work! I was attempting the following, without realizing that assigning the stub to a variable name means you can't string the functions together:

var describe = AWS.stub('ECS', 'describeContainerInstances')
.onCall(0).yields(null, fixtures.describeContainerInstances0)
.onCall(1).yields(null, fixtures.describeContainerInstances1)

Anyway, I feel good about merging this! 👍

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.

3 participants