compute: VM.waitFor() #2047

NickZ · 2017-03-04T10:29:49Z

See issue #2015

This pull request implements VM.waitFor(). This function waits for the VM to be in a specified state, then calls the callback (or resolves the promise).

example:

vm.waitFor("RUNNING", options, (err, metadata) =>{
  if (!err)
    //VM is running.
});

It performs this by calling getMetadata() in the background at a specified interval (currently 2000 milliseconds).

It takes in an options object, with a parameter timeout. This is by default set to 300, but it can be set to a maximum of 600 or a minimum of 0.

Regarding Events

It was requested in the issue that I implement this using events. After some thought, I've decided against using events for this. I think in this case they offer too many opportunities for unexpected behavior and/or hammering the API.

Events work on things like Operations because the event flow goes like this:

running->complete OR running->error.

Once the Operation is completed (or returned with an error), it will stay like that, forever. If a user calls operation.on("complete"), there's no problem with having to keep polling for statuses after it's reached "complete" or "error", because it will stay there.

However, with a VM, it can go from RUNNING to SUSPENDED to STOPPED and back any number of times. The state is transient, and isn't as simple to handle as an Operation.

Say a user calls vm.once("RUNNING"). This is fine because we will wait until it enters the state "RUNNING", and the event triggers, and stops polling because there are no more listeners.

However, say that a user calls vm.on("RUNNING"). It would be a reasonable assumption for the user to expect this to trigger every time the VM enters the "RUNNING" state, since they know that the VM state is transient. Because of this, this will require us to keep polling for status, forever, until the user removes the listener. This means a lot of superfluous API requests, and angry Google engineers.

We could just treat every "on" as "once", and explicitly say so in the API documentation. However, I don't know of many things that do this, and it would be unexpected by the user who is used to "on" meaning "on", unless they carefully read the documentation.

We could keep the number of api requests to a minimum to not hammer the server, say, every 15 seconds, but it's possible for the VM to transition states and back in that amount of time, and the event wouldn't get triggered.

We could carry around every single operation that is performed on the VM and emit when those have emitted, but that would require a large amount of architecting, and we want this to work independent of any operation executed in the script (say, a user stops and starts the VM in the web console).

This is not to say that events should not be implemented, I just couldn't find satisfactory solutions for these issues, and that's why I decided not to implement them. If you come up with some solutions for these issues, let me know and I can take another crack at implementing this with events.

Design Considerations.

So anyway, this is what I kept in mind when I designed VM.waitFor().

I didn't want to passively enable people to ping the API continously, so I added an options field with a timeout parameter. By default, this is 300 seconds, but the maximum is 600 seconds.

Since it's a one-time event, it's predictable: it will callback when it reaches the specified state with the metadata, or it will return an error. It's independent of any operation, and will work whether or not the user had performed any operations on the VM in the script.

Issues

I'm not quite sure that I have error handling correct. If it times out, it passes back an error with a name field set to "WaitForTimeout". I'm not sure if this is the correct procedure, so if you could please take a look.

stephenplusplus · 2017-03-24T18:33:52Z

Hey @NickZ, just wanted to let you know I appreciate this PR very much, and your level of thought into it. We've been on a bit of a release-after-release streak lately, so it's been hard to prioritize the review for this, but I will get to it, and your help has not been taken for granted. Thanks for your patience!

lukesneeringer

Thank you so much for your PR, and I am sorry it took us awhile to review -- that is on me.

I have offered a few comments, but this is generally in good shape. Let me know if you have any objections to the feedback offered.

packages/compute/src/vm.js

+/**
+ * Interval for polling during waitFor.
+ */
+var WAIT_FOR_POLLING_INTERVAL = 2000;


packages/compute/src/vm.js

+ *  - "SUSPENDED"
+ *  - "TERMINATED"
+ * @param {object=} options - Configuration object.
+ * @param {number} options.timeout - The number of seconds to wait until


packages/compute/src/vm.js

+  else if (options.timeout > 600) {
+    options.timeout = 600;
+  }
+  // If timeout is less than 0, set to 0 seconds.


packages/compute/src/vm.js

+    callback: callback
+  });
+
+  if (this.hasActiveWaiters === false) {


packages/compute/src/vm.js

+ *   var metadata = data[0];
+ * });
+ */
+VM.prototype.waitFor = function(status, options, callback) {


NickZ · 2017-03-27T23:40:44Z

@lukesneeringer what do you think about the error messages? Do you think they look OK?

lukesneeringer · 2017-03-28T00:18:22Z

Am I blind? I do not see error messages. :boggle:

packages/compute/src/vm.js

+              self.waiters[index].timeout) {
+          self.waiters[index].callback({
+            name: 'WaitForTimeout'
+          },null);


packages/compute/src/vm.js

+  if (VALID_STATUSES.indexOf(status) === -1) {
+    callback({
+      name: 'StatusNotValid'
+    }, null);


NickZ · 2017-03-28T00:36:38Z

@lukesneeringer Apologies, I added comments to point out where they were.

lukesneeringer · 2017-03-28T13:58:39Z

Ah, okay. I was looking for sentences when I was scanning.

I am fine with the codes, but send an Error object, not a plain dictionary-like object.

lukesneeringer · 2017-04-04T15:53:31Z

@stephenplusplus This looks good to me. Do you see anything?

stephenplusplus · 2017-04-04T17:08:31Z

Tests! I'll tag in and get on that. Thanks @NickZ!

Previously, this would set the timeout to the default of 300 if the timeout was set to 0. Fixed so that it only checks to see if timeout is not set. Added JSdoc comments clarifying that if the options.timeout is set to 0, it will poll once and then timeout if the state is not reached.

WAIT_FOR_POLLING_INTERVAL and VALD_STATUSES should be set to private.

An invalid status now throws an Error, since this is a problem with how the function is called (like detatchDisk() does) A time out will return an Error in the callback , with the appropriate message, code, and name.

There is no apiResponse passed to the callback.

While iterating over all the waiters, if we happen to remove a waiter, it would skip the next item on the next iteration because the indexes changed. So, I changed it to wait until after it finishes iterating over all the waiters in the array to remove them. I also changed it to use a 'for of' loop since there was no longer a reason to keep the index around.

googlebot · 2017-04-07T19:47:51Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

stephenplusplus · 2017-04-07T19:48:29Z

@NickZ please take a look at my latest commit to see that all functionality is as intended.

coveralls · 2017-04-07T20:12:30Z

Coverage remained the same at 100.0% when pulling f46147c on NickZ:VM_WAITFOR into aa9525d on GoogleCloudPlatform:master.

NickZ · 2017-04-07T22:07:30Z

Looks good to me. I am ok with these changes.

stephenplusplus · 2017-04-07T22:08:25Z

Awesome, thanks for the idea and all of the work on this!

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 4, 2017

stephenplusplus added api: compute Issues related to the Compute Engine API. Type: Enhancement labels Mar 6, 2017

lukesneeringer suggested changes Mar 27, 2017

View reviewed changes

NickZ commented Mar 28, 2017

View reviewed changes

packages/compute/src/vm.js Outdated

self.waiters[index].timeout) {

self.waiters[index].callback({

name: 'WaitForTimeout'

},null);

This comment was marked as spam.

Sign in to view

NickZ commented Mar 28, 2017

View reviewed changes

packages/compute/src/vm.js Outdated

if (VALID_STATUSES.indexOf(status) === -1) {

callback({

name: 'StatusNotValid'

}, null);

This comment was marked as spam.

Sign in to view

lukesneeringer approved these changes Apr 4, 2017

View reviewed changes

Nick Zatkovich and others added 10 commits April 7, 2017 13:29

compute: VM.waitFor()

1854a24

Fix linting errors

6f2541f

Fix coding style lint errors

7531026

Fix JSDocs comments

6cffe29

WAIT_FOR_POLLING_INTERVAL and VALD_STATUSES should be set to private.

Fix error handling

15ce07b

An invalid status now throws an Error, since this is a problem with how the function is called (like detatchDisk() does) A time out will return an Error in the callback , with the appropriate message, code, and name.

Fix errors in waitFor() documentation examples

9aeead3

fix JSDocs code examples

bf0095b

There is no apiResponse passed to the callback.

add tests

1d81e0d

stephenplusplus force-pushed the VM_WAITFOR branch from 6aa4859 to 1d81e0d Compare April 7, 2017 19:47

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Apr 7, 2017

remove includes() usage

f46147c

stephenplusplus merged commit 20cc892 into googleapis:master Apr 7, 2017

stephenplusplus mentioned this pull request Apr 7, 2017

Feature request: Add "wait for VM running" function #2015

Closed

sofisl pushed a commit that referenced this pull request Nov 17, 2022

compute: VM.waitFor() (#2047)

340ec90

sofisl mentioned this pull request Nov 17, 2022

migrate code from googleapis/nodejs-compute #3666

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compute: VM.waitFor() #2047

compute: VM.waitFor() #2047

NickZ commented Mar 4, 2017 •

edited by lukesneeringer

Loading

stephenplusplus commented Mar 24, 2017

lukesneeringer left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

NickZ commented Mar 27, 2017

lukesneeringer commented Mar 28, 2017

This comment was marked as spam.

This comment was marked as spam.

NickZ commented Mar 28, 2017

lukesneeringer commented Mar 28, 2017

lukesneeringer commented Apr 4, 2017

stephenplusplus commented Apr 4, 2017

googlebot commented Apr 7, 2017

stephenplusplus commented Apr 7, 2017

coveralls commented Apr 7, 2017

NickZ commented Apr 7, 2017

stephenplusplus commented Apr 7, 2017

compute: VM.waitFor() #2047

compute: VM.waitFor() #2047

Conversation

NickZ commented Mar 4, 2017 • edited by lukesneeringer Loading

Regarding Events

Design Considerations.

Issues

stephenplusplus commented Mar 24, 2017

lukesneeringer left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

NickZ commented Mar 27, 2017

lukesneeringer commented Mar 28, 2017

This comment was marked as spam.

This comment was marked as spam.

NickZ commented Mar 28, 2017

lukesneeringer commented Mar 28, 2017

lukesneeringer commented Apr 4, 2017

stephenplusplus commented Apr 4, 2017

googlebot commented Apr 7, 2017

stephenplusplus commented Apr 7, 2017

coveralls commented Apr 7, 2017

NickZ commented Apr 7, 2017

stephenplusplus commented Apr 7, 2017

NickZ commented Mar 4, 2017 •

edited by lukesneeringer

Loading