Add Alias IP and Guest Accelerator support to Instance Templates #639

negz · 2017-10-27T19:54:38Z

...by refactoring a whole bunch of instance and instance template code.

Apologies in advance for the size of this PR. I set out to add Alias IP support to instance templates. I decided to implement this by deduplicating a bunch of code that was replicated between resource_compute_instance and resource_compute_instance_template and got a little carried away refactoring.

The changes worth note in this PR:

Per the title instance templates support now Alias IP ranges and Guest Accelerators.
resource_compute_instance and resource_compute_instance_template now use the same code to expand and flatten service accounts and network interfaces.
I switched everything in resource_compute_instance over to the v1 API. It was using the beta API for Min CPU Platform and Guest Accelerators, which are both now GA.
I've deprecated the network_interface.0.network_ip key on resource_compute_instance_template in favour of address, in order to be more consistent with resource_compute_instance.

I've run the following tests:

$ GOOGLE_PROJECT=REDACTED \
  GOOGLE_XPN_HOST_PROJECT=REDACTED \
  GOOGLE_REGION=us-central1 \
  GOOGLE_USE_DEFAULT_CREDENTIALS=1 \
  TF_ACC=1 \
  go test -v -timeout=120m -run "^TestAccComputeInstance(Template|)_.+$" .
...
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 1356.099s

negz · 2017-11-03T20:03:58Z

@rosbo Let me know if there's anything I can do to help you review this - I feel bad about how unwieldy the PR is. Happy to schedule some time to walk through it.

rosbo

Sorry for the long wait, I was out on vacation. Thanks for the cleanup, instance and instance template grew awful... this makes it definitely cleaner :)

rosbo · 2017-11-06T17:24:05Z

website/docs/r/compute_instance_template.html.markdown

+
+* `type` (Required) - The accelerator type resource to expose to this instance. E.g. `nvidia-tesla-k80`.
+
+* `count` (Required) - The number of the guest accelerator cards exposed to this instance.


Nit: Add a new line after

rosbo · 2017-11-06T17:25:28Z

google/resource_compute_instance_template.go

@@ -221,6 +242,33 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 										Computed: true,
 										Optional: true,
 									},
+
+									"assigned_nat_ip": &schema.Schema{


Since an IP is only assigned when an instance is created, it doesn't make sense to have this field in the instance template.

I can't think of a good way to remove this from instance templates while also sharing interface flatten code with instances. I think that might be what sent me down the path of deprecating assigned_nat_ip in favour of just using nat_ip for both explicit and computed NAT IPs.

I believe that:

d.Get("assigned_nat_ip") returns "" or the assigned ip IF the field is present in the
d.Get("assigned_nat_ip") returns nil if the field is not present in the schema.

You might want to use this property to be able to reuse the flatten method. It is a bit hacky. The other option is to have two separate flatten methods. Or your approach, to be honest, your approach might be just fine. If we decide to go ahead and deprecate the assigned_nat_ip in the future, it will all go away.

I will leave it up to you. If you keep it the way it is, can you add a small comment explaining why we have a assigned_nat_ip in the instance template schema.

Part of me prefers your suggested way, but using it would require passing all of d down to flattenAccessConfigs, and I feel a little better about that function currently simply acting on a slice of *compute.AccessConfig (at the expense of having to keep a pointless assigned_nat_ip in the compute instance schema). I've added a comment.

rosbo · 2017-11-06T17:29:42Z

google/resource_compute_instance_template.go

@@ -184,23 +187,41 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 				ForceNew: true,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
-						"network": &schema.Schema{
+						"name": &schema.Schema{


Why did you add a name field here in the network_interface block?

I added it because resource_compute_instance.go has a name field in its network_interface block. I was operating under the assumption the schema had to exactly match in those two resources in order for them to share a flatten/expand function.

I'm happy to remove name if that's not the case, since an instance template pretty obviously won't have an instance name.

I am ok with leaving it so we can reuse the same flatten/expand function.

I managed to work around this - I only try to set the name field if the returned API object has a Name field, which instance template NICs will not.

rosbo · 2017-11-06T17:46:05Z

google/utils.go

-			}
-			network = networkData.SelfLink
-		}
+func getNetworkLink(config *Config, project, network string) (string, error) {


In #527 and #529 I created a helper function to replace this method. I started the work of moving off the getNetworkLink method, my next step was to finish off with instance and instance template. Consider deleting this method and use the method here. The problem with this method is that it makes an extra network call to generate the self link for the network.

The other benefit of using this new method is that you can easily extract the project from the link. No need for the extra methods you added in utils.go to do this.

I replaced getNetworkLink with the helpers you suggested, thanks!

I added some code to parse regional fields, with the intention of using the same helpers for subnetworks. Unfortunately in the context of an instance/instance template the subnetwork's project isn't necessarily the same as the instance's project due to XPN, so I don't think the existing helpers will work.

rosbo · 2017-11-06T17:48:12Z

google/utils.go

+	return acceleratorsSchema
+}
+
+// Instances want a partial URL, but instance templates want the bare


You can use the pattern we are using here: https://github.com/terraform-providers/terraform-provider-google/blob/master/google/field_helpers.go#L20

This way both instance and instance template can accept either the name or the URL. And then when calling the api, you can pass whichever the API requires.

rosbo · 2017-11-06T17:51:42Z

google/resource_compute_instance.go

-										Computed: true,
+										Type:       schema.TypeString,
+										Computed:   true,
+										Deprecated: "Please use nat_ip",


What is the benefit of changing the way people use the nat ip? This PR is already big, I would leave this change out of it for now and open an issue (explaining why it is desirable and how the benefits outweigh having to force users to change their config) if you feel strongly about it.

Note: I don't know why they where split in two fields in the first place, I wasn't there when the decision was taken, maybe @danawillow has more insights.

Nah, I wasn't there either. Maybe it can change due to reasons other than setting the field via the API? There's nothing in the docs that gives me that impression though, it seems like if you set it it'll be what you set it to, and if you don't set it it'll assign something. @evandbrown do you know?

I guessed that perhaps the original implementor was unaware that a field could be both optional and computed - i.e. could be set by a human or determined by GCE - and thus deduplicated them.

I don't feel strongly about making this change though - happy to leave it out of this PR.

I would remove the Deprecated notice from assigned_nat_ip but you can leave the nat_ip field as Computed and set the value from GCE. This way, we don't force user to change their config for no clear benefit and the logic for flatten/expand is simpler.

SGTM - Will do.

rosbo · 2017-11-06T17:53:09Z

google/resource_compute_instance.go

@@ -328,6 +311,14 @@ func resourceComputeInstance() *schema.Resource {
 							Computed: true,
 						},

+						"network_ip": &schema.Schema{


@danawillow what was the plan with address and network_ip?

Looks like @negz put some info about this in the PR description

My bad, it makes total sense to mark it as deprecated.

rosbo · 2017-11-08T23:18:38Z

google/resource_compute_instance_template.go

@@ -184,23 +187,41 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 				ForceNew: true,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
-						"network": &schema.Schema{
+						"name": &schema.Schema{


I am ok with leaving it so we can reuse the same flatten/expand function.

rosbo · 2017-11-08T23:26:56Z

google/resource_compute_instance_template.go

@@ -221,6 +242,33 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 										Computed: true,
 										Optional: true,
 									},
+
+									"assigned_nat_ip": &schema.Schema{


I believe that:

d.Get("assigned_nat_ip") returns "" or the assigned ip IF the field is present in the
d.Get("assigned_nat_ip") returns nil if the field is not present in the schema.

You might want to use this property to be able to reuse the flatten method. It is a bit hacky. The other option is to have two separate flatten methods. Or your approach, to be honest, your approach might be just fine. If we decide to go ahead and deprecate the assigned_nat_ip in the future, it will all go away.

I will leave it up to you. If you keep it the way it is, can you add a small comment explaining why we have a assigned_nat_ip in the instance template schema.

rosbo · 2017-11-09T00:59:44Z

google/resource_compute_instance.go

-										Computed: true,
+										Type:       schema.TypeString,
+										Computed:   true,
+										Deprecated: "Please use nat_ip",


I would remove the Deprecated notice from assigned_nat_ip but you can leave the nat_ip field as Computed and set the value from GCE. This way, we don't force user to change their config for no clear benefit and the logic for flatten/expand is simpler.

negz · 2017-11-15T01:54:02Z

@rosbo I believe I've addressed all your comments as best I can. All tests are passing.

rosbo

Thanks for the great work! Only one small change in the RegionalFieldValue helper and we should be good to merge.

This will also fix #431 :)

rosbo · 2017-11-15T17:19:38Z

google/field_helpers.go

+// - "" (empty string). RelativeLink() returns empty if isEmptyValid is true.
+//
+// If the project is not specified, it first tries to get the project from the `projectSchemaField` and then fallback on the default project.
+// If the region is not specified, it takes the value of `regionSchemaField`.


The Regional case is slightly different from the Zonal case. You can set the default region on the provider. The logic should fallback on the default region if it is not specified in the field value or the schema field.

You can refactor the getRegion method to have a structure similar to getProject. This will introduce a getRegionFromSchema method that you can use here.

negz · 2017-11-15T20:57:01Z

@rosbo Done!

rosbo

@danawillow could you also review this one. This is a pretty big PR on one of the most utilized resource. I would like to have a second pair of eyes reviewing it :)

rosbo · 2017-11-15T21:05:39Z

website/docs/r/compute_instance.html.markdown

@@ -259,7 +259,7 @@ exported:

 * `network_interface.0.address` - The internal ip address of the instance, either manually or dynamically assigned.

-* `network_interface.0.access_config.0.assigned_nat_ip` - If the instance has an access config, either the given external ip (in the `nat_ip` field) or the ephemeral (generated) ip (if you didn't provide one).
+* `network_interface.0.access_config.0.nat_ip` - If the instance has an access config, either the given external ip (in the `nat_ip` field) or the ephemeral (generated) ip (if you didn't provide one).


Revert to network_interface.0.access_config.0.assigned_nat_ip

danawillow

Took a first look- since one of my comments would require a fair number of changes that are reverting things, I'm going to hold off reviewing further until that's sorted out (since I'll be able to do it a lot faster if things do get reverted)

Also, thanks so much for taking this work on! In the future, it would be great if things like this were split across smaller PRs- it might extend the amount of time before the code is fully checked in, but it would definitely make our lives easier to only have to review smaller pieces at a time.

danawillow · 2017-11-16T19:13:44Z

google/resource_compute_instance.go

-		if err != nil {
-			return nil, handleNotFoundError(err, d, fmt.Sprintf("Instance %s", d.Get("name").(string)))
-		}
+	instance, err := config.clientCompute.Instances.Get(project, zone, d.Id()).Do()


I think that when a resource gets transitioned to having beta support, we should keep it that way even if there are no beta features left in it- that way the next time we have a beta feature to add, we don't need to go through the whole process again. What do you think?

I want to think through what that would look like for a resource that supported the beta API but that didn't currently use any beta features.

If I understand correctly, using this resource as an example:

There would be an empty InstanceVersionedFeatures, i.e. var InstanceVersionedFeatures = []Feature{}

We'd keep all the switch computeApiVersion blocks and keep up-converting compute structs to their computeBeta counterparts.

All flatten/expand helper functions (which are potentially shared by multiple resources) would take computeBeta arguments, regardless of whether they contained any beta functionality.

I think that could work. It would be a little confusing that everything took computeBeta arguments regardless of whether it actually contained any beta functionality, and would be especially unfortunate if we needed to keep maintaining two implementations of each helper function.

I'm on the fence and happy to defer to the provider's maintainers. I can put the scaffolding for beta resources back in. I think the code is easier to reason about without the beta resources, but empathise that someone will likely have to add all the boilerplate back next time we want to support a beta feature on this resource.

I was thinking about this more this morning. I'm increasingly feeling that it's more readable to leave out beta support for resources that don't make use of beta features, assuming that re-adding beta support is as simple as:

Re-inserting well-established boilerplate (InstanceVersionedFeatures, switch computeApiVersion)

s/compute/computeBeta/g.

I think it's more readable, but also less maintainable, if that makes sense. Right now, the barrier to entry of using a versioned feature is adding it to the InstanceVersionedFeatures list. If we remove all the current beta scaffolding, then the barrier to entry becomes adding it all back. If we get to a point where we don't have any beta features in any resources, then we would need to go back into the files' histories to find examples of how to convert back and forth. Or, a new contributor might not even realize that we have examples already, and then they'd have to try to figure it all out again. For a user adding something unrelated to beta features, the extra scaffolding is easy to work around, but if the code isn't there it could potentially become very difficult for someone to figure out how to add it back. So I stand by my original opinion that we should leave it in.

@danawillow Fair enough. How do you think I should proceed here?

I believe that if I re-add the beta scaffolding to resource_compute_instance I'll need to switch all of its helper functions to take/produce computeBeta structs.

A big part of this PR's intent was to reuse code between resource_compute_instance and resource_compute_instance_template in order to make the two resources more consistent from a Terraform user's perspective. If I reintroduce the beta scaffolding for resource_compute_instance I'll need to also add said scaffolding to resource_compute_instance_template in order for the two instances to use the same code.

I'm happy to add beta scaffolding to both resources, but I'm also open to abandoning this PR and sending a new one that simply adds what I really want (Alias IP support for instance templates) if you think this is all getting too unwieldy.

Sure, but if you switch it to non-beta and then a beta feature comes along, you still would need to figure out what to do about beta-ifying instance template.

I'm ok with adding the Alias IP support in a new PR so we can keep having the refactoring conversations in this one while that gets in (it'll probably be a quick review since it'll have mostly already been reviewed)

danawillow · 2017-11-16T19:21:45Z

google/resource_compute_instance.go

@@ -948,10 +798,11 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
 	}
 	attachedDisks = append(attachedDisks, extraAttachedDisks...)

+	d.Set("service_account", flattenServiceAccounts(instance.ServiceAccounts))


Hypothetically, if someone were in a weird state where they had a service account configured upstream but not in their config, this now adds it to their state, causing a diff. I don't think this is breaking-enough to need to be in a major release or anything like that (I really hope this doesn't actually affect anyone), but we should add a note in the CHANGELOG just so people are aware.

@danawillow I don't follow why that would be.

Previously we set the service accounts inline: https://github.com/terraform-providers/terraform-provider-google/pull/639/files/eb36a7c615c78b220388b693b779a19216a28c09#diff-6ebc1a840e42235a51a46d537f17ddaaL823

This PR breaks that same code out into a flattenServiceAccounts function: https://github.com/terraform-providers/terraform-provider-google/pull/639/files/eb36a7c615c78b220388b693b779a19216a28c09#diff-d9f99248626432c3fc0b32182bf3a6faR571

However I don't believe there are any changes to what actually happens - i.e. the code does the same thing.

Ah you're right, missed it before- thanks!

negz · 2017-11-17T23:45:42Z

In the future, it would be great if things like this were split across smaller PRs- it might extend the amount of time before the code is fully checked in, but it would definitely make our lives easier to only have to review smaller pieces at a time.

@danawillow Understood - I should know better. I really just wanted Alias IP support for instance templates, but got carried away refactoring. I felt like I'd be contributing to a problem (i.e. two divergent implementations of the same thing) if I were to simply go add Alias IP support to the instance template resource.

negz · 2017-11-22T23:14:04Z

@danawillow I've re-added the beta scaffolding to compute instance and added it to compute instance template. All tests are passing.

danawillow · 2017-11-27T21:53:34Z

google/resource_compute_instance.go

@@ -948,10 +798,11 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
 	}
 	attachedDisks = append(attachedDisks, extraAttachedDisks...)

+	d.Set("service_account", flattenServiceAccounts(instance.ServiceAccounts))


Ah you're right, missed it before- thanks!

danawillow · 2017-11-27T22:46:43Z

google/resource_compute_instance_template_test.go

+		// results in: 
+		// Invalid value for field 'resource.properties.networkInterfaces[0].aliasIpRanges[0].ipCidrRange':
+		// '172.16.0.0/24'. Alias IP CIDR range must be a valid netmask starting with '/' (e.g. '/24')
+    	alias_ip_range {


nit: indentation

danawillow · 2017-11-27T22:49:11Z

google/utils.go

+		}
+
+		subnetworkProject := data["subnetwork_project"].(string)
+		subnetLink, err := getSubnetworkLink(config, project, region, subnetworkProject, subnetwork)


Were you intending on having this use ParseSubnetworkFieldValue? That got added in this PR but isn't used anywhere.

#639 (comment)

I added some code to parse regional fields, with the intention of using the same helpers for subnetworks. Unfortunately in the context of an instance/instance template the subnetwork's project isn't necessarily the same as the instance's project due to XPN, so I don't think the existing helpers will work.

I did originally, but then it was too much effort per the above comment. :) I think it might be worth doing, but it seems like it might be a good candidate for a separate PR given that ParseSubnetworkFieldValue would not work as currently implemented. I'm happy to drop ParseSubnetworkFieldValue and the regional field parsing code from this PR since it's unused if that's preferred.

Ah ok. At this point my goal is to just get this PR in, so don't worry about it

danawillow · 2017-11-27T22:54:48Z

google/utils.go

@@ -408,3 +365,252 @@ func extractFirstMapConfig(m []interface{}) map[string]interface{} {

 	return m[0].(map[string]interface{})
 }
+
+func expandAliasIpRanges(ranges []interface{}) []*computeBeta.AliasIpRange {


Since these are only specific to instances/instance templates, I'd rather them be in resource_compute_instance.go (or a new file) instead of the provider-wide utils

SGTM. I think I'd prefer a new file. _helpers.go and _utils.go patterns both exist, so maybe compute_instance_helpers.go.

danawillow · 2017-11-27T22:56:45Z

google/resource_compute_project_metadata.go

+// compute.metadata rather than computeBeta.metadata as an argument. It should
+// be removed in favour of flattenMetadata if/when this resource gets beta
+// support.
+func flattenCommonInstanceMetadata(metadata *compute.Metadata) map[string]string {


There are a whole bunch of metadata expanders/flatteners floating around (here, resource_compute_instance, utils.go, metadata.go)- is it possible to have just one of each that live in the same file? (my vote is for metadata.go)

SGTM, will change.

To reflect the fact they'll be used by multiple resources.

It's the only thing meta is used for.

compute.Instance.MinCpuPlatform is now GA.

This seemed to be a pre-existing issue, i.e. I could repro it in master. --- FAIL: TestComputeInstanceMigrateState (0.00s) panic: interface conversion: interface {} is nil, not *google.Config [recovered] panic: interface conversion: interface {} is nil, not *google.Config goroutine 85 [running]: testing.tRunner.func1(0xc4205d60f0) /usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:711 +0x2d2 panic(0x203acc0, 0xc4205d2080) /usr/local/Cellar/go/1.9.1/libexec/src/runtime/panic.go:491 +0x283 github.com/terraform-providers/terraform-provider-google/google.migrateStateV3toV4(0xc4205f2000, 0x0, 0x0, 0x0, 0x48, 0xc4205f2000) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate.go:182 +0x2405 github.com/terraform-providers/terraform-provider-google/google.resourceComputeInstanceMigrateState(0x2, 0xc4205f2000, 0x0, 0x0, 0x0, 0x0, 0xe0000000000) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate.go:48 +0x21a github.com/terraform-providers/terraform-provider-google/google.runInstanceMigrateTest(0xc4205d60f0, 0x2260816, 0x8, 0x227d23a, 0x20, 0x2, 0xc4205ec0f0, 0xc4205ec120, 0x0, 0x0) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate_test.go:803 +0xc1 github.com/terraform-providers/terraform-provider-google/google.TestComputeInstanceMigrateState(0xc4205d60f0) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate_test.go:71 +0xc84 testing.tRunner(0xc4205d60f0, 0x22d81c0) /usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:746 +0xd0 created by testing.(*T).Run /usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:789 +0x2de FAIL github.com/terraform-providers/terraform-provider-google/google 0.035s

Alias IP ranges, Accelerators, and min CPU platform are now GA.

Methods used by both resource_compute_instance and resource_compute_instance_template are currently spread between their respective files, and utils.go. This commit moves them all into utils.go for the sake of consistency. It may be worth considering an instance_common.go file or similar.

…e and service_account code This has the side effect of enabling Alias IP range support for compute_instance_templates.

We compute it from the subnet its network interfaces are in. Note this is not new behaviour - I believe it was erroneously missing the computed flag.

Since most of the code is already there.

…templates

…mpute instance template

Currently unused because subnetworks may have a separate project from that of the instance using them, which complicates looking up the project field.

The API calls to Google to create guest accelerators take different values for instances and instance templates. Instance templates don't have a zone and can thus *only* be passed a guest accelerator name.

Also slightly refactors getXFromSchema field helper functions for readability.

Note these resources don't currently use beta features - this is futureproofing.

negz · 2017-11-28T01:48:10Z

@danawillow Think I've got all your comments addressed. It was mostly just shuffling functions around to more sensible places, but I confirmed the tests are still passing too.

danawillow · 2017-11-28T18:00:56Z

Thansk @negz, and sorry for the drawn-out review cycle! Merging now.

negz · 2017-11-29T01:18:42Z

No problem @danawillow ! Thank you and @rosbo for all the help getting this merged.

…hicorp#639) * Move AliasIpRange helpers into utils To reflect the fact they'll be used by multiple resources. * Pass Config to build helpers, not meta It's the only thing meta is used for. * Refactor getNetwork util methods to return early for the happy path. * Update compute APIs compute.Instance.MinCpuPlatform is now GA. * Fix panic in TestComputeInstanceMigrateState This seemed to be a pre-existing issue, i.e. I could repro it in master. --- FAIL: TestComputeInstanceMigrateState (0.00s) panic: interface conversion: interface {} is nil, not *google.Config [recovered] panic: interface conversion: interface {} is nil, not *google.Config goroutine 85 [running]: testing.tRunner.func1(0xc4205d60f0) /usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:711 +0x2d2 panic(0x203acc0, 0xc4205d2080) /usr/local/Cellar/go/1.9.1/libexec/src/runtime/panic.go:491 +0x283 github.com/terraform-providers/terraform-provider-google/google.migrateStateV3toV4(0xc4205f2000, 0x0, 0x0, 0x0, 0x48, 0xc4205f2000) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate.go:182 +0x2405 github.com/terraform-providers/terraform-provider-google/google.resourceComputeInstanceMigrateState(0x2, 0xc4205f2000, 0x0, 0x0, 0x0, 0x0, 0xe0000000000) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate.go:48 +0x21a github.com/terraform-providers/terraform-provider-google/google.runInstanceMigrateTest(0xc4205d60f0, 0x2260816, 0x8, 0x227d23a, 0x20, 0x2, 0xc4205ec0f0, 0xc4205ec120, 0x0, 0x0) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate_test.go:803 +0xc1 github.com/terraform-providers/terraform-provider-google/google.TestComputeInstanceMigrateState(0xc4205d60f0) /Users/negz/control/go/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_instance_migrate_test.go:71 +0xc84 testing.tRunner(0xc4205d60f0, 0x22d81c0) /usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:746 +0xd0 created by testing.(*T).Run /usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:789 +0x2de FAIL github.com/terraform-providers/terraform-provider-google/google 0.035s * Use only the v1 API for resource_compute_instance Alias IP ranges, Accelerators, and min CPU platform are now GA. * Move common instance code into utils.go Methods used by both resource_compute_instance and resource_compute_instance_template are currently spread between their respective files, and utils.go. This commit moves them all into utils.go for the sake of consistency. It may be worth considering an instance_common.go file or similar. * Unify compute_instance and compute_instance_template network_interface and service_account code This has the side effect of enabling Alias IP range support for compute_instance_templates. * Add tests for compute instance template Alias IP ranges * Mark instance template region as computed We compute it from the subnet its network interfaces are in. Note this is not new behaviour - I believe it was erroneously missing the computed flag. * Support guest accelerators for instance templates Since most of the code is already there. * Add a test for using 'address' rather than 'network_ip' for instance templates * Don't mark assigned_nat_ip as deprecated * Remove network_interface schema fields that don't make sense for a compute instance template * Add newline after count in instance template docs * Don't try to dedupe guest accelerator expansion code The API calls to Google to create guest accelerators take different values for instances and instance templates. Instance templates don't have a zone and can thus *only* be passed a guest accelerator name. * Use ParseNetworkFieldValue instead of getNetworkLink * Add support for parsing regional fields, and subnetworks specifically Currently unused because subnetworks may have a separate project from that of the instance using them, which complicates looking up the project field. * Fall back to provider region when parsing regional field values Also slightly refactors getXFromSchema field helper functions for readability. * Revert to assigned_nat_ip in compute instance docs * Add beta scaffolding to compute instance and compute instance template Note these resources don't currently use beta features - this is futureproofing. * Fix indentation in comment about instance template alias IP ranges * Consolidate metadata helper functions in metadata.go * Move compute instance (and template) related helpers into their own file

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost · 2020-03-30T14:10:13Z

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!

rosbo self-requested a review October 30, 2017 18:29

negz force-pushed the unifyinstancecode branch from 4e989c1 to fcf2fc3 Compare November 3, 2017 20:01

rosbo suggested changes Nov 6, 2017

View reviewed changes

paddycarver added the waiting-response label Nov 7, 2017

rosbo suggested changes Nov 9, 2017

View reviewed changes

negz force-pushed the unifyinstancecode branch from fcf2fc3 to 5ff4880 Compare November 15, 2017 01:20

rosbo suggested changes Nov 15, 2017

View reviewed changes

rosbo mentioned this pull request Nov 15, 2017

Add support for min_cpu_platform in google_compute_instance. #349

Merged

rosbo self-assigned this Nov 15, 2017

rosbo approved these changes Nov 15, 2017

View reviewed changes

danawillow reviewed Nov 17, 2017

View reviewed changes

rosbo removed the waiting-response label Nov 20, 2017

negz force-pushed the unifyinstancecode branch 2 times, most recently from 8828595 to 701ac6f Compare November 22, 2017 02:41

danawillow reviewed Nov 27, 2017

View reviewed changes

Nic Cope added 10 commits November 27, 2017 17:10

Move AliasIpRange helpers into utils

b12d5e2

To reflect the fact they'll be used by multiple resources.

Pass Config to build helpers, not meta

f6203af

It's the only thing meta is used for.

Refactor getNetwork util methods to return early for the happy path.

8fb203b

Update compute APIs

12d0004

compute.Instance.MinCpuPlatform is now GA.

Use only the v1 API for resource_compute_instance

6852821

Alias IP ranges, Accelerators, and min CPU platform are now GA.

Unify compute_instance and compute_instance_template network_interfac…

6f3bfd6

…e and service_account code This has the side effect of enabling Alias IP range support for compute_instance_templates.

Add tests for compute instance template Alias IP ranges

381ce2d

Mark instance template region as computed

a30acaa

We compute it from the subnet its network interfaces are in. Note this is not new behaviour - I believe it was erroneously missing the computed flag.

Nic Cope added 14 commits November 27, 2017 17:15

Support guest accelerators for instance templates

d28073f

Since most of the code is already there.

Add a test for using 'address' rather than 'network_ip' for instance …

ee44d7a

…templates

Don't mark assigned_nat_ip as deprecated

36c6b06

Remove network_interface schema fields that don't make sense for a co…

d3318df

…mpute instance template

Add newline after count in instance template docs

464b7e7

Use ParseNetworkFieldValue instead of getNetworkLink

1a2f76a

Add support for parsing regional fields, and subnetworks specifically

ace2c8a

Currently unused because subnetworks may have a separate project from that of the instance using them, which complicates looking up the project field.

Don't try to dedupe guest accelerator expansion code

fa5dd69

The API calls to Google to create guest accelerators take different values for instances and instance templates. Instance templates don't have a zone and can thus *only* be passed a guest accelerator name.

Fall back to provider region when parsing regional field values

73cb2b5

Also slightly refactors getXFromSchema field helper functions for readability.

Revert to assigned_nat_ip in compute instance docs

2a61a0f

Add beta scaffolding to compute instance and compute instance template

3c0f9bb

Note these resources don't currently use beta features - this is futureproofing.

Fix indentation in comment about instance template alias IP ranges

e8f8442

Consolidate metadata helper functions in metadata.go

2f8d7ad

Move compute instance (and template) related helpers into their own file

d40a8b5

negz force-pushed the unifyinstancecode branch from 701ac6f to d40a8b5 Compare November 28, 2017 01:46

danawillow merged commit 94a405d into hashicorp:master Nov 28, 2017

danawillow mentioned this pull request Nov 28, 2017

compute instance helpers / metadata cleanup #797

Merged

paddycarver mentioned this pull request Oct 2, 2018

Deprecate assigned_nat_ip in compute_instance family #2155

Merged

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Add compute target instances (TF-only) (hashicorp#639)

31f59ad

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost unassigned rosbo Mar 30, 2020

ghost locked and limited conversation to collaborators Mar 30, 2020


		* `type` (Required) - The accelerator type resource to expose to this instance. E.g. `nvidia-tesla-k80`.

		* `count` (Required) - The number of the guest accelerator cards exposed to this instance.

Add Alias IP and Guest Accelerator support to Instance Templates #639

Add Alias IP and Guest Accelerator support to Instance Templates #639

Conversation

negz commented Oct 27, 2017

negz commented Nov 3, 2017

rosbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz commented Nov 15, 2017

rosbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz commented Nov 15, 2017

rosbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz commented Nov 17, 2017

negz commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz commented Nov 28, 2017

danawillow commented Nov 28, 2017

negz commented Nov 29, 2017

ghost commented Mar 30, 2020

negz Nov 20, 2017 •

edited

Loading