Add Google Compute Engine support

[mziccard]

Fixes #341
Fixes #718

This pull request adds a module to the library to support Google Compute Engine, as described in #718.
The module is so far capable of:

• list/create/delete/start/stop/reset instances
• list/create/delete disks
• list/create/update firewall rules
• attach/detach disks to/from instances

I tried to follow the coding style and added some documentation. The module lacks of tests (both unit and system). I plan to add some unit tests as soon as I have some time to.

Suggestions are more than welcome. Feel free to ask for changes/fixes/updates or apply them yourselves, I am happy either way:)

[jgeewax]

This is fantastic. Thank you so much :)

[stephenplusplus]

A while back (#341), @ryanseys worked on an API for Compute Engine support: https://gist.github.com/ryanseys/28c9a17fea00899f3ac7

Could you take a look over the hierarchy he proposed and let us know if you think keeping it flat like in this PR is better, or if we should switch over to that style? Any other thoughts or comparisons would be great to have!

[mziccard]

Personally, I have never thought of regions and zones as real "entities" in a cloud infrastructure. I always considered them as a mere geographical location where a resource (i.e. a real entity) resides. However, this is a personal thought that stems from my own work experience.

From a programming perspective I like @ryanseys hierarchical structure better as it gives a precise role to regions and zones and does not relegate them to string parameters. We should also take into account how we expect users to interact with cloud engine entities. On the one hand, it might be reasonable to assume that a user creates an instance via a zone entity as instances reside in zones. On the other hand, I also imagine that users tend to manipulate instances/disks in a project as a whole (possibly spread across zones/regions): a user might want to apply a tag to all instances in his project, regardless of the zone.

It might be interesting to implement the hierarchical structure while also providing capabilities to manipulate entities at the global (project) level when the APIs allow us to (e.g. listing instances and disks at project level might be super-useful).

[jgeewax]

AWS does quite a few things similarly from an API perspective. How do they do this in their Node SDK ?

[stephenplusplus]

It might be interesting to implement the hierarchical structure while also providing capabilities to manipulate entities at the global (project) level when the APIs allow us to (e.g. listing instances and disks at project level might be super-useful).

👍 That matches the approach we try to take throughout the library.

[mziccard]

@jgeewax It has been a while since I used them last but if I am not mistaken in the AWS SDK the region is a global configuration parameter (as the API key). All requests issued are then directed to the specified region. Then the structure is quite flat, you have an EC2 entity that offers methods to manage resources, zones are passed as string parameters to these methods.

@stephenplusplus It should not be too complicated to make this PR follow the hierarchical structure.

[mziccard]

With the last commit I added zone and region entities.

• listInstances and listDisks methods of a Zone object return instances and disks in that zone
• createInstance and createDisk are now Zone methods
• listInstances and listDisks on a Compute object perform an aggregated search

I avoided adding default values compute.allZones and compute.defaultZone as I think that aggregated functions should apply directly to a Compute object and that there's no need for a default zone when building a Compute object.

[stephenplusplus]

Thanks again, @mziccard! I'll be looking at this over the next few days/week. Sorry in advance for the lag!

[mziccard]

Guys, I added some more functionalities (Snapshots and Addresses). I think I'll stop adding stuff and wait for a review of yours so that I can align the code (and myself) to your coding style and apply your suggestions/fixes before going on.

I think next step would be adding an Operation class. Most APIs are asynchronous and return as a response an operation object. Users can then check the operation status and possible errors. We could also provide something (roughly speaking) like:

operation.onComplete(
    period, // The polling interval to update operation metadata
    function(error, operation) {
        // operation has status DONE and error is set if an error occurred
    });

Or even something more elaborated to stream also other events (such as the transition from PENDING to RUNNING or the operation's progress).

[jgeewax]

Re operations -- I think this is something we should totally do, might be a good idea to look across other APIs for where the concept of long-running operations exist and make sure that what we do fits for those also -- it's definitely going to be a common pattern.

[mziccard]

Hi guys. As you can see I added some more functionalities (Network) and addressed your comments. I started thinking about how to implement operation and would like to do some brainstorming before starting to code anything.
I went through the docs and identified 3 types of operations GlobalOperation, RegionOperation and ZoneOperation. The three JSON objects differ only in the fact that RegionOperation has a region field and ZoneOperation has a zone field. Given this small difference I am not so convinced that we need 3 classes one for each operation and I would rather go for a single Operation class. What do you think?
Operations can take time to complete (go to DONE status) so we need a method for the user to register a callback for the complete event. I had in mind something like:

// Interval is the optional polling period, default is 10
operation.onComplete(interval, function(error, warnings) {
  // operation metadata are updated and status==='DONE'
  // If error is set the operation failed.
  // If warnings is set the operation succeeded with warnings.
}); 

If no callback is provided the onComplete method returns a stream the user can register listeners on.

// Interval is the polling period, is optional
operation.onComplete(interval)
  .on('complete', function(error, warnings) {
    // operation metadata are updated and status==='DONE'
    // If error is set the operation failed.
    // If warnings is set the operation succeeded with warnings.
  })
  .on('progress', function(progress) {
    // progress is a monotonically increasing progress indicator in [0, 100]
  });

I am not so happy of the redundancy of operation.onComplete(interval).on('complete', .... We might prefer something like operation.stream(interval).on('complete', .... I do not have a clear idea in mind yet so comments that improve/change/trash my proposal are more than welcome:)

[callmehiphop]

Given this small difference I am not so convinced that we need 3 classes one for each operation and I would rather go for a single Operation class.

That sounds like a good plan to me.

I am not so happy of the redundancy of operation.onComplete(interval).on('complete', ....

We have a pretty elegant polling solution in the PubSub module, I think a similar approach would work well here.

var operation = compute.operation('myOperation', {
  type: 'global',
  interval: 99999
});

operation.on('done', function() {});
operation.on('error', function() {});

We can use the EventEmitter to watch for listeners and start/stop polling according to that.
Thoughts?

[mziccard]

@callmehiphop I saw what you did in the PubSub module and might be a good solution here. The difference is that an operation does not necessarily have an interval attribute when it is created. Let me clarify, in a method like getOperations (I assume we will have it for Compute, Zone and Region), you get and build some Operation objects but there's no interval provided there. What happens if the user then directly uses on are we going to use the default value? This is not a big issue but I feel like an operation has a different semantics from a subscription which is more event-oriented in its nature.

[callmehiphop]

We could either pass the interval into getOperations and apply it to all returned operations (not sure how good of an idea that is) or alternatively they could be set after the fact.

compute.getOperations(function(err, operations) {
  operations.forEach(function(operation) {
    operation.interval = 9999;
  });
});

/cc @stephenplusplus

[stephenplusplus]

I prefer the second option @callmehiphop suggested.

What happens when the user gets an Operation that is already completed or had errors?

getOperations(function(err, operations) {
  var aCompletedOperation = operations[0];

  aCompletedOperation.on('complete', function () {
    // immediately invoked with aCompletedOperation.metadata?
  });

  var aCompletedOperationWithErrors = operations[1];

  aCompletedOperationWithErrors.on('error', function() {
    // invoked once per aCompletedOperationWithErrors.errors[]?
  });
  aCompletedOperationWithErrors.on('complete', function() {
    // immediately invoked with aCompletedOperationWithErrors.metadata?
  });
});

Just for reference, a similar concept exists in BigQuery where certain actions return a Job instance which must be pinged for its completion status. We don't give the user a way to listen for an event when it is complete. Instead, the user just calls job.getMetadata or job.getQueryResults to check the status / get the results.

I think an EventEmitter for just-returned, still-running operations would be handy, but for complete operations would be pretty confusing. I'm leaning more towards putting the task of pinging for a complete status on the user and not developing a solution around this. But it would be great if there was a solution that wasn't awkward when applied to an already-completed operation.

[mziccard]

I would say that in case of an already completed operation we immediatelly fire all the events. But I agree we better go with just providing the getMetadata method and let the user handle the polling until we come up with a better solution. We might add an example in the docs that uses getMetadata in combination with setTimeout to check the status until the operation's status is DONE.

[stephenplusplus]

Sounds good to me 👍

[mziccard]

I think I found a problem while adding operations support that might need your intervention:)
When you do a GET on an operation (our operation.getMetadata) it returns a JSON object that describes it. If the operation failed its JSON object has an error field set and this causes the util.parseApiResp function to assume the API call we are performing failed and util.handleResp to run callback with error parameter set.
I wanted to go around this problem by checking in operation.getMetadata that the response was actually a well formed Operation and not an error. However I realized that in case of a (supposed) error util.handleResp calls the callback with only the error parameter set (no response). A possible fix could be returning the response even when an error is detected and let me handle mis-detected errors in operation.getMetadata. This change should be done by modifying line 193 of util.js (double check this as I only had a quick look at util.js):

callback(parsedApiResponse.err);

to

callback(parsedApiResponse.err, parsedApiResponse.body, parsedApiResponse.resp);

This change should not affect the library nor the users. Thoughts?

[stephenplusplus]

It was actually a bug that handleResp was only executing the callback with the parsed error, so we fixed it recently to return all parts (just like you suggested). See if re-basing fixes the problem.

[mziccard]

My bad, I did not notice that you had already fixed the problem. I rebased and everything worked, thanks. We now have operation support. For those APIs that are asynchronous (i.e. those that respond with an operation JSON) now the callback takes an operation parameter (instead of an apiResponse parameter) that can be used to check the state of the request. In the operation.getMetadata docs I put an example showing how to use the method to check the operation's status until it's DONE.

[stephenplusplus]

Perfect, thank you! If it's okay with you, @callmehiphop and I can take this over now to write the tests and get it merged in asap!

[mziccard]

@stephenplusplus Yeah that's more than okay with me. I feel a bit bad for making you and @callmehiphop test all that code:) But I guess this is fastest way to do it.

[stephenplusplus]

It's no problem, we're ecstatic you contributed all of this code for us!

[stephenplusplus]

Thanks again @mziccard! Going to close this PR in favor of #792.