Skip to content
This repository has been archived by the owner. It is now read-only.

Add AMI set entity and its create and delete endpoints #85

Merged
merged 4 commits into from Jul 4, 2016

Conversation

@anarute
Copy link
Contributor

anarute commented Jun 6, 2016

I was not sure about its endpoints, if I am using the correct AMI Set properties.


properties: {

amiSetId: base.Entity.types.String,

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

since we're in the AmiSet class, let's call this just 'id'

properties: {

amiSetId: base.Entity.types.String,
amis: base.Entity.types.JSON,

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

Can you write a comment here that describes what this object should look like? It's really helpful to know what to expect in the data when coding against this Entity in the future.

deferAuth: true,
scopes: [['aws-provisioner:manage-ami-set:<amiSetId>']],
input: 'create-ami-set-request.json#',
output: 'get-ami-set-response.json#',

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

These (input and output) point to the schemas we talked about today. You can just omit these lines for the moment, and then return to add schemas in a later pull request.

let input = req.body;
let amiSet = req.params.amiSetId;

input.lastModified = new Date();

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

A lastModified property is a good idea, but you'll need to include it in src/ami-set.js as well.

// Publish pulse message
await this.publisher.amiSetCreated({
amiSet: amiSetId,
});

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

I don't think we need to send a pulse message on AMI set creation. It doesn't hurt, and we could add it back in later.

For WorkerTypes, this message is required so that the provisioner backend can look at the new workerType and start creating EC2 instance for it. For new AmiSets, there's no need for the backend to do anything.

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

We don't actually consume any pulse messages in the provisioner. We scan the worker type table on each iteration, since we need to load all the worker types anyway to see what the max/min capacity, scaling ratio and instance types. We could maintain that state in the backend process, but that's not there yet and would be a fairly substantial code change. I don't know if the Azure overhead (tech or billing wise) is worth the trouble.

That said, I don't think that we really need to have pulse messages for AmiSets. The important Pulse messages are for WorkerType updates, since those are (well, should be) used to gracefully shutdown old instances when we update the worker type definition.

deferAuth: true,
scopes: [['aws-provisioner:manage-ami-set:<amiSetId>']],
input: undefined, // No input
output: undefined, // No output

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

You can just omit these

let that = this;
let amiSet = req.params.amiSetId;

if (!req.satisfies({amiSet: amiSetId})) { return undefined; }

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

Why undefined?

This comment has been minimized.

Copy link
@anarute

anarute Jun 6, 2016

Author Contributor

ops, sorry, I think I misunderstood this from worker-type.js

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

An understandable confusion -- WorkerType.create actually overrides the Entity.create method to give it a different function signature. That's not necessary for AmiSet.

src/main.js Outdated
@@ -11,6 +11,8 @@ let base = require('taskcluster-base');

let workerType = require('./worker-type');
let secret = require('./secret');
let workerState = require('./worker-state');

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

You should rebase your patch to the latest version of the upstream master branch. There was a change that I landed today that deletes the WorkerState entity, and it looks like you're adding it back here.

src/main.js Outdated
},
},

amiSet: {

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

We have a convention of anything that's a taskcluster-base.Entity using class casing, so please call this AmiSet here, and the other places that you are referring to the thing returned by AmiSet.setup instead of an instance of an AmiSet.

src/main.js Outdated
setup: async ({cfg}) => {
let amiSet = amiSet.setup({
account: cfg.azure.account,
table: cfg.app.workerStateTableName,

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

I think you'll need a different table name for ami sets :)

* by its AWS region.
*/

let amiSet = base.Entity.configure({

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

This should be capitalized (AmiSet). The idea is that this is a class, so just like in Python we use an initial capital letter.

amis: base.Entity.types.JSON,

},
});

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

The content of this module looks OK, but it doesn't export anything. There's some information on exports here:

https://www.sitepoint.com/understanding-module-exports-exports-node-js/

In this case, we want require('./ami-set') to return the configured entity, just like for require('./worker-type').

This comment has been minimized.

Copy link
@anarute

anarute Jun 6, 2016

Author Contributor

oh, right! so, I think I also need an AmiSet.create function, right?

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

You get that one for free -- base.Entity.configure(..) creates a class for you with a few methods (create, update, remove, etc.). Sadly there's basically no documentation for that (https://bugzilla.mozilla.org/show_bug.cgi?id=1278385).

src/main.js Outdated
},
},

amiSet: {

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

This should be capitalized (AmiSet).

src/main.js Outdated
});
return WorkerState;
},
},

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

I think this may be a merge error? @jhford just removed the WorkerState entity class. Awesome that you've rebased your changes against the latest master, by the way!

This comment has been minimized.

Copy link
@anarute

anarute Jun 6, 2016

Author Contributor

yes, it is a merge error! probably it was from the commit last week and I didn't see this part was removed. Now I see the importance of sending small patches as soon as I can :)

].join('\n'),
}, async function (req, res) {
let input = req.body;
let amiSet = req.params.amiSetId;

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

I probably would call this amiSetId since you'll be creating an Entity and probably want this name for that, instead of aSet

src/main.js Outdated
setup: async ({cfg, WorkerType, Secret, ec2, stateContainer, validator, publisher, influx}) => {
requires: ['cfg', 'WorkerType', 'WorkerState', 'amiSet', 'Secret', 'ec2', 'validator', 'publisher', 'influx'],
setup: async ({cfg, WorkerType, WorkerState, amiSet, Secret, ec2, validator, publisher, influx}) => {

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

As above, I don't think you want WorkerState in here (@jhford please correct me if I'm wrong!)

This comment has been minimized.

Copy link
@jhford

jhford Jun 6, 2016

Contributor

Nope, she doesn't. I think that a merge went a little wrong, because I just today deleted the WorkerState on the master branch.

src/main.js Outdated
let reportInstanceStarted = series.instanceStarted.reporter(influx);

let router = await v1.setup({
context: {
WorkerType: WorkerType,
amiSet: amiSet,

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 6, 2016

Contributor

This too should be capitalized (AmiSet)

@jhford
Copy link
Contributor

jhford commented Jun 6, 2016

@anarute really good first pass! Most of the things that are needed are here. It seems like you and @djmitche are talking about json-schema for the AmiSets, which is a great idea. Otherwise, looking great!

@djmitche
Copy link
Contributor

djmitche commented Jun 6, 2016

OK that was a lot of comments, but this looks good!

If I remember correctly, you were able to run the tests for the provisioner. Do they still run after your changes?

You will also need some tests in test-src/manage_ami_set_test.js, similar to test-src/manageworkertype_test.js (but again, much shorter, since so far AMI sets are a lot less complicated than worker types). If it would help you out, I can write up a test for one of the API methods as an example.

@anarute
Copy link
Contributor Author

anarute commented Jun 7, 2016

Thank you @djmitche and @jhford for the comments and feedback! It was really helpful to understand things better. I think (if I have not added more things to fix) that I've covered most part of the issues, could you please review it again?

title: 'Create new AMI Set',
stability: base.API.stability.stable,
description: [
'Create an AMI Set. An AMI Set is a collection of AMIs with a single name.',
].join('\n'),
}, async function (req, res) {
let input = req.body;
let amiSet = req.params.amiSetId;
let AmiSetId = req.params.id;

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

AmiSet is a 'class', but this isn't referring to the class. This is referring to a string and so should be amiSetId or just id... either one is fine.

return;
}

// Create amiSet
let aSet;
let amiSetId;

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

this will refer to an AmiSet instance, so let's call it amiSet.

Basically, if a thing refers to a class (or in JS terms a constructor), it should be Caps

Here are some examples:

class ExampleClass { }

function ExampleConstructor () { }

let ExampleClass = require('./lib/example-class').ExampleClass;
let exampleInstance = new ExampleClass();

let exampleString = 'john';
src/main.js Outdated
account: cfg.azure.account,
table: cfg.app.workerStateTableName,
table: cfg.app.amiSetTableName,

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

We'll need to add this variable to the config file config.yml

id: base.Entity.types.String,
/* This is a JSON object which contains the AMIs of an AMI set keyed by
* their virtualization type and region. It is in the shape:
* {

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

This is not specific to your patch, but I'm sort of wondering if a structure like

{
   "us-west-1": {
      "hvm": "ami-111",
      "pv": "ami-222"
  },
   "us-east-1": {
      "hvm": "ami-111",
      "pv": "ami-222"
  }
}

or

[
   {
      "region": "us-west-1",
      "hvm": "ami-111",
      "pv": "ami-222"
   },
   {
      "region": "us-east-1",
      "hvm": "ami-111",
      "pv": "ami-222"
   }
]

The reason for flipping the hvm/pv around is that the structure goes from most general (region) to more specific (virtualization style) to most specific (ami id).

Whether we have a list of objects or a straigh mapping is something that you'll have to pick. A list of objects is really nice because you can do really neat things with Array.prototype.map and Array.prototype.filter. A mapping is really nice because it's easier to address the values and it is impossible to have duplicates. Error checking with the object will be easier, but the list of objects is more in line with what we do elsewhere in the provisioner and probably what I'd prefer to see.

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 7, 2016

Contributor

I tend to agree regarding the list of objects (the second option). That will also mean we don't need to change our schema when we add a new region. I also agree that we don't need to solve it in this patch.

src/main.js Outdated
@@ -124,13 +137,15 @@ let load = base.loader({
},

api: {
requires: ['cfg', 'WorkerType', 'Secret', 'ec2', 'stateContainer', 'validator', 'publisher', 'influx'],
setup: async ({cfg, WorkerType, Secret, ec2, stateContainer, validator, publisher, influx}) => {
requires: ['cfg', 'WorkerType', 'AmiSet', 'Secret', 'ec2', 'validator', 'publisher', 'influx'],

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

There's another merge issue here. stateContainer needs to be in both the requires list and the unpacked variable assignments in the setup function signature, but it's not. The usage of the stateContainer variable here isn't deleted, which is good.

@@ -580,6 +580,94 @@ api.declare({

api.declare({
method: 'put',
route: '/ami-set/:amiSetId',

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

let's go with just :id here as well. When inside the managing functions for a thing itself, i find it easier to just refer to it as 'id' or something, and use more descriptive names for the ids of things that are referenced as a part of a thing.

input.lastModified = new Date();

// Authenticate request with parameterized scope
if (!req.satisfies({amiSet: AmiSetId})) {

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

the property that is checked against (here amiSet) needs to match the parameterized name in the scopes list (here amiSetId, but should be id).

After renaming the amiSetId variables and scopes to id, the object to use here would be {id: id}. In es6+, if your property name and value are derived from the save variable, e.g. let x = 1; {x: x} then you can just do `let x = 1; {x}.

let amiSet = req.params.amiSetId;

try {
await this.amiSet.remove({amiSet: amiSetId}, true);

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

Remember that you're passing in the AmiSet entity class as a context variable to this function in src/main.js. This name here needs to be the name of the context object key, since what we do is take the 'context' object and assign all those values as properties on the this in this scope. In other words, you're looking for this.AmiSet.remove.

let reportInstanceStarted = series.instanceStarted.reporter(influx);

let router = await v1.setup({
context: {
WorkerType: WorkerType,
AmiSet: AmiSet,

This comment has been minimized.

Copy link
@jhford

jhford Jun 7, 2016

Contributor

Regarding my comment about this.AmiSet.remove, this is the place where you're creating the value this.AmiSet. This is magic done by the library, but fyi.

@anarute anarute force-pushed the anarute:add-ami-sets branch from 89c6db7 to 7bf3350 Jun 8, 2016
@anarute anarute force-pushed the anarute:add-ami-sets branch 5 times, most recently from 9fa0787 to 906fcd6 Jun 27, 2016
try {
await base.Entity.scan.call(this, {}, {
handler: function(item) {
amiSetList.push(item.json());

This comment has been minimized.

Copy link
@jhford

jhford Jun 28, 2016

Contributor

If this is a listing method, we should be pushing the item.id property instead of the complete item in a json-serializable format. If the goal is a load-all method, let's call it loadAll() to match the worker type here

This comment has been minimized.

Copy link
@djmitche

djmitche Jun 28, 2016

Contributor

I believe we already had some discussion of this in the PR? Many of the list methods in TC services return the entire object, e.g., auth.listClients. The reason not to do so is if it requires additional queries or a lot more bandwidth. Neither is the case here. We have the additional information about each AMI set returned from Azure, so why not provide it to the user?

'id',
'amis',
].every((key) => {
return _.isEqual(amiSet[key], input[key]);

This comment has been minimized.

Copy link
@jhford

jhford Jun 28, 2016

Contributor

I can't remember off hand, but we should ensure that _.isEqual is a deep equality operator, since an amiSet is an object which collects other objects.

This comment has been minimized.

Copy link
@djmitche
api.declare({
method: 'get',
route: '/ami-set/:id',
name: 'getAmiSet',

This comment has been minimized.

Copy link
@jhford

jhford Jun 28, 2016

Contributor

The WorkerType entity uses name: 'workerType'. Can we follow that pattern here?


let amiSet = await this.AmiSet.load({id: id});

await amiSet.modify(function(amiS) {

This comment has been minimized.

Copy link
@jhford

jhford Jun 28, 2016

Contributor

the name amiS sort of implies that it's a plural of amis but with confusing captalization. Instead, since we're passing in an instance of an AmiSet, let's use the amiSet variable name.

});

beforeEach(async () => {
await main('tableCleaner', {process: 'tableCleaner', profile: 'test'});

This comment has been minimized.

Copy link
@jhford

jhford Jun 28, 2016

Contributor

We should add the AmiSet entity to the tableCleaner component in src/main.js so that this table is actually cleaned out.

amiSet = await client.getAmiSet(id);
lastModified = amiSet.lastModified;
assume(await amiSet).to.deeply.equal({
amis: [

This comment has been minimized.

Copy link
@jhford

jhford Jun 28, 2016

Contributor

we use mock objects in a single file elsewhere to make it easier to update the format of the test objects without having to completely rewrite the test. In this case, the objects are simple enough that I don't know if it's worth doing here, but please take a look at the test-src/mock*.js files just to see how they're structured. Basically, we have a default that is returned, but you can overwrite the values you want overwritten. Not critical here, but worth looking at.

@anarute anarute force-pushed the anarute:add-ami-sets branch from 906fcd6 to 8d79eb7 Jun 28, 2016
@anarute
Copy link
Contributor Author

anarute commented Jun 28, 2016

@jhford I fixed the things you've mentioned:

@djmitche
Copy link
Contributor

djmitche commented Jun 28, 2016

If we need loadAll(), it's easy enough to add in a new PR :)

@jhford
Copy link
Contributor

jhford commented Jun 29, 2016

So there's a problem here. The create ami set endpoint has a json-schema that forces there to be an ID parameter in the request body. The problem is that we already have that specified in the parameters of the api method. The create schema should only have the amis object.

@jhford
Copy link
Contributor

jhford commented Jun 29, 2016

Ok. So I think the only issue left here is removing the createAmiSet json-schema allowing an ID value. The only place we should be supplying the id value is the parameter that we bake into the /ami-set/:id route.

I think with that change, i'm 99% sure that we're ready to merge!

@anarute anarute force-pushed the anarute:add-ami-sets branch from 8d79eb7 to a4049d6 Jun 30, 2016
@anarute
Copy link
Contributor Author

anarute commented Jun 30, 2016

@jhford I removed the id property from createAmiSet schema, could you review the code again, please?

- region
- hvm
- pv
lastModified:

This comment has been minimized.

Copy link
@jhford

jhford Jul 1, 2016

Contributor

Sorry that I didn't see this before, but we also should not have last modified as an allowed field in the request. we will generate this value inside the api handler, so forcing a value here that we won't end up using isn't ideal.

let input = req.body;
let id = req.params.id;

input.lastModified = new Date();

This comment has been minimized.

Copy link
@jhford

jhford Jul 1, 2016

Contributor

We shouldn't really be setting properties on the request body object. Also, this value isn't actually used in the AmiSet.create method call below

amiSet = await this.AmiSet.create({
id: id,
amis: input.amis,
lastModified: new Date(),

This comment has been minimized.

Copy link
@jhford

jhford Jul 1, 2016

Contributor

here's where we're actually using this value

This comment has been minimized.

Copy link
@anarute

anarute Jul 1, 2016

Author Contributor

you're right, I passed that out among all the changes! thanks for noticing

@@ -0,0 +1,107 @@
var slugid = require('slugid');

This comment has been minimized.

Copy link
@jhford

jhford Jul 1, 2016

Contributor

random nit, this file uses let and var. We should probably chose one and stick to it.

The difference in case you're unfamiliar is that var is in scope through the entire function. If you use var in an if, the variable is in scope through the entire function. Let is block scope, if you use let in an if, it's only in scope inside that if and those blocks (e.g. code inside of { }) that are nested.

Let is better than var! Also, if you're really curious do a search about 'variable hoisting' for more info on how var works.

This comment has been minimized.

Copy link
@anarute

anarute Jul 1, 2016

Author Contributor

thanks for the explanation!

debug('### Load amiSet (again)');
amiSet = await client.amiSet(id);
lastModified = amiSet.lastModified;
assume(await amiSet).to.deeply.equal({

This comment has been minimized.

Copy link
@jhford

jhford Jul 1, 2016

Contributor

we don't need to await amiSet because we already await'd it above.

This comment has been minimized.

Copy link
@jhford

jhford Jul 1, 2016

Contributor

Also, i think this is way outside the scope of this project, but one thing that I like to do is compare the amiSetChanged object itself. Doing that does require that you handle the lastModified stuff, as you do correctly below. Another option is to use Sinon to mock out the system clock. That's really out of scope and what you have here is fine. I might consider using _.default to grab the amis object from amiSetChanged

@anarute anarute force-pushed the anarute:add-ami-sets branch from a4049d6 to aba6495 Jul 1, 2016
@anarute anarute force-pushed the anarute:add-ami-sets branch from aba6495 to 72a05c9 Jul 1, 2016
@jhford jhford merged commit 72a05c9 into taskcluster:master Jul 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.