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

Add support for guest_accelerators (GPU) to google_compute_instance #330

Merged
merged 7 commits into from
Aug 22, 2017

Conversation

rosbo
Copy link
Contributor

@rosbo rosbo commented Aug 16, 2017

Fixes #212.
Related to #311

guest_accelerator is a beta feature. My first step was to convert google_compute_instance to a beta resource.

I upgraded the compute beta client to the latest version. The beta version we were currently using had fields in the Instance class where pointers for string and bool weren't used to differentiate between unset and default value (false or ""). For instance, some fields in the v1 client had "bool*" and now only had "bool" type. The latest beta version didn't have this problem and lined up with the v1 client.

@paddycarver We will need to add quota for GPU (default quota is 0) to the CI service account.

@danawillow

@rosbo
Copy link
Contributor Author

rosbo commented Aug 16, 2017

make testacc TESTARGS="-run TestAccComputeInstance_guestAccelerator" TEST="./google"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run TestAccComputeInstance_guestAccelerator -timeout 120m
=== RUN   TestAccComputeInstance_guestAccelerator
--- PASS: TestAccComputeInstance_guestAccelerator (207.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	207.833s

make testacc TESTARGS="-run TestAccComputeInstance_basic" TEST="./google"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run TestAccComputeInstance_basic -timeout 120m
=== RUN   TestAccComputeInstance_basic_deprecated_network
--- PASS: TestAccComputeInstance_basic_deprecated_network (79.97s)
=== RUN   TestAccComputeInstance_basic1
--- PASS: TestAccComputeInstance_basic1 (56.00s)
=== RUN   TestAccComputeInstance_basic2
--- PASS: TestAccComputeInstance_basic2 (60.43s)
=== RUN   TestAccComputeInstance_basic3
--- PASS: TestAccComputeInstance_basic3 (58.73s)
=== RUN   TestAccComputeInstance_basic4
--- PASS: TestAccComputeInstance_basic4 (63.90s)
=== RUN   TestAccComputeInstance_basic5
--- PASS: TestAccComputeInstance_basic5 (48.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	367.914s


func flattenBetaScheduling(scheduling *computeBeta.Scheduling) []map[string]interface{} {
result := make([]map[string]interface{}, 0, 1)
schedulingMap := make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can put some of the items in the map initially:

schedulingMap := map[string]interface{}{
  "on_host_maintenanace": scheduling.OnHostMaintenance
  // etc.
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I also fixed the flattenScheduling method.

@danawillow danawillow self-assigned this Aug 17, 2017
Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

It's awesome that you were able to get to google_compute_instance so fast!

I noticed two small things when scanning through quickly, I was curious what the scope of the changes looked like!

d.Set("scheduling", scheduling)

d.Set("scheduling", flattenBetaScheduling(instance.Scheduling))
d.Set("guest_accelerator", flattenGuestAccelerators(instance.Zone, instance.GuestAccelerators))
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Line below) If we don't convert the self link to v1, we will cause diffs in resources that refer to this one; you can use the existing function I was using, or move to a StateFunc.

In this case I'd probably do the former because it is already a big PR, and you can clean up all the old function usages at once in a separate PR when you have time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -260,6 +260,16 @@ The `scheduling` block supports:
* `automatic_restart` - (Optional) Specifies if the instance should be
restarted if it was terminated by Compute Engine (not a user).

---

* `guest_accelerator` - (Optional, Beta) List of the type and count of accelerator cards attached to the instance. Structure documented below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should link to the Beta section of the provider docs whenever we write that a field is Beta; that way, users can easily discover what that means for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rosbo
Copy link
Contributor Author

rosbo commented Aug 17, 2017

Thanks for reviewing this @rileykarson !

@rosbo
Copy link
Contributor Author

rosbo commented Aug 17, 2017

Waiting on @paddycarver to increase quota for the tesla-k80 GPU (default quota is 0) on our CI account before submitting.

@rosbo rosbo force-pushed the beta-compute-instance branch from 9c603d9 to 36150ab Compare August 22, 2017 19:05
@rosbo
Copy link
Contributor Author

rosbo commented Aug 22, 2017

Rebased with master. Re-ran the tests:

make testacc TESTARGS="-run TestAccComputeInstance_guestAccelerator" TEST="./google"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run TestAccComputeInstance_guestAccelerator -timeout 120m
=== RUN   TestAccComputeInstance_guestAccelerator
--- PASS: TestAccComputeInstance_guestAccelerator (77.89s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	78.018s`

@paddycarver
Copy link
Contributor

Ran under the CI account with the quota increased. Passed for me. Feel free to merge at will :)

@rosbo rosbo merged commit 7a216c4 into hashicorp:master Aug 22, 2017
@rosbo rosbo deleted the beta-compute-instance branch August 22, 2017 19:49
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
Add OAuth2 access_token and client config as credential options.
@ghost
Copy link

ghost commented Mar 31, 2020

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Create GPU instances
4 participants