Add status argument (RUNNING/TERMINATED) to google_compute_instance

[norbjd]

Hello,

I added a status argument on google_compute_instance to be able to
start/stop instances using Terraform, therefore answering :

• this issue : google_compute_instance should allow to specify instance state #1719
• and also this question on StackOverflow : How to stop GCP vm instances using terraform (that made me discover this was not possible yet)

When searching I noticed that there is an open PR on that topic but sadly it looks like it has been abandoned (no update since 2019/03/13), so I've decided to implement it by following the valuable advices written by @rileykarson in this PR.

This is my first PR here so I gladly accept any comment :)

Recap

I added a status field accepting only values RUNNING (default) or TERMINATED.
There are many other existing states : PROVISIONING, STAGING, STOPPING, REPAIRING (from the docs), and I have also seen STOPPED, SUSPENDED or SUSPENDING on Compute Go client (https://godoc.org/google.golang.org/api/compute/v1#Instance, check Status field). Nevertheless, I think that only RUNNING and TERMINATED have sense here, because other are kind of "transition" states.

Instance creation

On instance creation, I have chosen to tolerate only RUNNING status, because this is the default behavior in the current provider, and because I did not have an example that came to my mind where it is needed to create a TERMINATED instance. Also, modifications should have been more important (I think) if the possibiility to create TERMINATED instances were given.

On instance creation (or update), in order to wait for the instance to reach the expected status, I used resource.StateChangeConf and the WaitForState() function to avoid reinventing the wheel, but I don't know if parameters are correct here (Delay, Timeout, and even Pending states). I'm open to any suggestion.

Instance update

On instance update, I have done two things :

• start the instance if status changes from TERMINATED to RUNNING, and stop the instance if status changes from RUNNING to TERMINATED
• if an instance have already TERMINATED status, and when updating fields requiring the instance to be stopped (like changing machine_type, min_cpu_platform, ...), do not try to stop (and after restart) the instance because it is already stopped (TERMINATED status)

Acceptance tests

Finally, I have written acceptance tests covering and passing the following cases :

• a RUNNING instance is created
• an instance is TERMINATED (RUNNING => TERMINATED)
• an instance is started from a TERMINATED status (TERMINATED => RUNNING)
• a TERMINATED instance is updated :
  • on fields that do not require the instance to be stopped (for example labels, metadata, ...)
  • on fields that require the instance to be stopped (machine_type)
• a TERMINATED instance is deleted
• a TERMINATED instance cannot be created (expected to be RUNNING when creating)

I don't think adding this status field will impact the previous behavior of the provider (maybe wait a little more when creating an instance because it needs to wait until the instance is RUNNING, though when testing I've noticed that the instance was always RUNNING when it reached the part of the code waiting for the status). However, I did not run the whole acceptance tests suite (like in other PR I believe), so I'm wondering, is there a process that will run all tests before merging any PR to ensure that the modification did not break something unexpected?

I look forward to reading your comments.

compute: added the ability to manage the status of `google_compute_instance` resources with the `desired_status` field

[norbjd]

Bumping this up.

Is there anything else I can do for this PR?

[tysen]

I'll review this soon.

[norbjd]

Hello @tysen, did you get a chance to look at the updates I made after your first review?

[rileykarson]

Hey @norbjd! Sorry for the delay in getting to this once I was assigned, I'm taking over as reviewer of this PR.

I've left a handful of comments that broadly move us from a joint status field that outputs the current status + can be set to a desired_status field that users can opt-in to using and that avoids some unfortunate interactions with regards to existing infrastructure.

If you'd like to output the current status as status, feel free to do that as well.

I haven't looked at the tests yet, nor the stopping_for_update section of the PR. I'd prefer to try out the change to desired_status first, and then work out the specifics of those parts afterwards.

[norbjd]

Hello @rileykarson, thanks for your review. I made some changes according to your comments. To sum up :

• rename status to desired_status
• add a CustomizeDiff to ensure that instances cannot be created when desired_status = TERMINATED
• update and fix the acceptance tests

[norbjd]

Hello @rileykarson, just updated the PR with your helpful comments 😄

To sum up, the biggest update was to handle all cases that requires stopping the instance before updating some of the attributes. Introducing desired_status created some conflicts, as for example there is no need to stop an instance already TERMINATED. I've made a table based on the different cases you provided and implement an acceptance test for each case :

+----------------------------------------------------------------------+---------+-----------------------+-------------------------------------------------------------------------------------------+
| Input                                                                |         | Actions (ordered)     |                                                                                           |
+----------------------------------------------------------------------+         +-----------------------+                                                                                           |
| Current instance status | desired_status | allow_stopping_for_update | Success | Stop | Update | Start | Acceptance test                                                                           |
+-------------------------+----------------+---------------------------+---------+------+--------+-------+-------------------------------------------------------------------------------------------+
| RUNNING                 | ""             | TRUE                      | X       | X    | X      | X     | TestAccComputeInstance_updateRunning_desiredStatusNotSet_allowStoppingForUpdate           |
| RUNNING                 | RUNNING        | TRUE                      | X       | X    | X      | X     | TestAccComputeInstance_updateRunning_desiredStatusRunning_allowStoppingForUpdate          |
| RUNNING                 | ""             | FALSE                     |         |      |        |       | TestAccComputeInstance_updateRunning_desiredStatusNotSet_notAllowStoppingForUpdate        |
| RUNNING                 | RUNNING        | FALSE                     |         |      |        |       | TestAccComputeInstance_updateRunning_desiredStatusRunning_notAllowStoppingForUpdate       |
| RUNNING                 | TERMINATED     | TRUE                      | X       | X    | X      |       | TestAccComputeInstance_updateRunning_desiredStatusTerminated_allowStoppingForUpdate       |
| RUNNING                 | TERMINATED     | FALSE                     | X       | X    | X      |       | TestAccComputeInstance_updateRunning_desiredStatusTerminated_notAllowStoppingForUpdate    |
+-------------------------+----------------+---------------------------+---------+------+--------+-------+-------------------------------------------------------------------------------------------+
| TERMINATED              | ""             | TRUE                      | X       |      | X      |       | TestAccComputeInstance_updateTerminated_desiredStatusNotSet_allowStoppingForUpdate        |
| TERMINATED              | TERMINATED     | TRUE                      | X       |      | X      |       | TestAccComputeInstance_updateTerminated_desiredStatusTerminated_allowStoppingForUpdate    |
| TERMINATED              | ""             | FALSE                     | X       |      | X      |       | TestAccComputeInstance_updateTerminated_desiredStatusNotSet_notAllowStoppingForUpdate     |
| TERMINATED              | TERMINATED     | FALSE                     | X       |      | X      |       | TestAccComputeInstance_updateTerminated_desiredStatusTerminated_notAllowStoppingForUpdate |
| TERMINATED              | RUNNING        | TRUE                      | X       |      | X      | X     | TestAccComputeInstance_updateTerminated_desiredStatusRunning_allowStoppingForUpdate       |
| TERMINATED              | RUNNING        | FALSE                     | X       |      | X      | X     | TestAccComputeInstance_updateTerminated_desiredStatusRunning_notAllowStoppingForUpdate    |
+-------------------------+----------------+---------------------------+---------+------+--------+-------+-------------------------------------------------------------------------------------------+

[rileykarson]

Looking great, and thanks for the thorough enumeration + testing of all those cases. I have a couple small thoughts, as well as some comments that can help reduce the amount of redundant test code a little.

[rileykarson]

LGTM. Thanks for working through this @norbjd! Configuration of states on a resource has had no precedents in Terraform that I'm aware of. The best one is the issue for AWS instances (/ the original issue), and that's as-of-yet unresolved. Great work!

I'm going to upstream this to Magic Modules so these changes make it to the google-beta provider as well, and merge this PR after the MM PR has been approved. I'm going to need a reviewer there, so if they have any suggestions I'll apply them upstream during the merge process, they'll be applied to this repo as well by our syncing tool.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!