Add support for interface on disk in google_compute_instance

[helielson]

Interface

Optional. The kind of disk interface exposed to the VM for this SSD. Valid values are SCSI and NVME. SCSI is the default and is supported by more guest operating systems. NVME may provide higher performance.
From https://cloud.google.com/sdk/gcloud/reference/compute/instances/create

#5598

[danawillow]

There are a lot of unrelated files in this PR- mind taking them out?

[danawillow]

Instead of a boolean, consider making this a string for the interface name. That way if there's ever a third option the code will just work.

[helielson]

Hi.
I followed the same pattern used for definition of scratch disk type that the possible values for are either "PERSISTENT" or "SCRATCH" and it was defined as boolean with PERSISTENT as default value.
Should I change both?

About the unrelated files I removed them.

[helielson]

@danawillow Any thoughts about how to test it?
I have tried but the the TestAcc* tests always fails on my machine

[danawillow]

Ah, I see. So you can't change the scratch field because that would cause a backwards-incompatible change with people that are already using the boolean. Since that field does have just the two values, I think leaving it be is just fine- we'll fix it when it becomes a problem. Note that the pattern I suggested is used plenty of places elsewhere in the provider, so this isn't going entirely against convention.

I can help you debug your test failures if you think it's a configuration problem. I'm also happy to run them myself to verify. However, I don't actually see any test changes in your PR. Mind adding one?

[matschaffer]

fwiw, I would have also expected interface = "nvme" rather than nvme = true.

[jbarbuto]

Hi @helielson, could I assist with adding tests to help get this merged?

[helielson]

@jbarbuto. I'll change the implementation from boolean to string.

I've tried to run the acceptance tests and it failure.
I'll update this branch and try again. I would appreciate if you assist me with adding tests.

[jbarbuto]

How are the acceptance tests failing? They're working for me after setting the requested environment variables:

$  GCLOUD_KEYFILE_JSON="$(cat <your_keyfile_path>)" GCLOUD_PROJECT=<your_project_id> GCLOUD_REGION=us-central1 GOOGLE_XPN_HOST_PROJECT=<your_project_id> make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeInstance_local_ssd'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/12 17:21:52 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeInstance_local_ssd -timeout 120m
=== RUN   TestAccComputeInstance_local_ssd
--- PASS: TestAccComputeInstance_local_ssd (46.26s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	46.278s

[jbarbuto]

I've included a patch with a working acceptance test against your fork, rebased with master.

Code Diff

diff --git a/builtin/providers/google/resource_compute_instance.go b/builtin/providers/google/resource_compute_instance.go
index f7feeb2d6..476e82bae 100644
--- a/builtin/providers/google/resource_compute_instance.go
+++ b/builtin/providers/google/resource_compute_instance.go
@@ -880,6 +880,7 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
 				"image":                   d.Get(fmt.Sprintf("disk.%d.image", dIndex)),
 				"type":                    d.Get(fmt.Sprintf("disk.%d.type", dIndex)),
 				"scratch":                 d.Get(fmt.Sprintf("disk.%d.scratch", dIndex)),
+				"nvme":                    d.Get(fmt.Sprintf("disk.%d.nvme", dIndex)),
 				"auto_delete":             d.Get(fmt.Sprintf("disk.%d.auto_delete", dIndex)),
 				"size":                    d.Get(fmt.Sprintf("disk.%d.size", dIndex)),
 				"device_name":             d.Get(fmt.Sprintf("disk.%d.device_name", dIndex)),
diff --git a/builtin/providers/google/resource_compute_instance_test.go b/builtin/providers/google/resource_compute_instance_test.go
index e91368e24..333f7e789 100644
--- a/builtin/providers/google/resource_compute_instance_test.go
+++ b/builtin/providers/google/resource_compute_instance_test.go
@@ -303,6 +303,27 @@ func TestAccComputeInstance_local_ssd(t *testing.T) {
 	})
 }
 
+func TestAccComputeInstance_local_ssd_nvme(t *testing.T) {
+	var instance compute.Instance
+	var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10))
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckComputeInstanceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccComputeInstance_local_ssd_nvme(instanceName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckComputeInstanceExists(
+						"google_compute_instance.local-ssd-nvme", &instance),
+					testAccCheckComputeInstanceDisk(&instance, instanceName, true, true),
+				),
+			},
+		},
+	})
+}
+
 func TestAccComputeInstance_update_deprecated_network(t *testing.T) {
 	var instance compute.Instance
 	var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10))
@@ -1227,6 +1248,30 @@ func testAccComputeInstance_local_ssd(instance string) string {
 	}`, instance)
 }
 
+func testAccComputeInstance_local_ssd_nvme(instance string) string {
+	return fmt.Sprintf(`
+	resource "google_compute_instance" "local-ssd-nvme" {
+		name = "%s"
+		machine_type = "n1-standard-1"
+		zone = "us-central1-a"
+
+		disk {
+			image = "debian-8-jessie-v20160803"
+		}
+
+		disk {
+			type = "local-ssd"
+			scratch = true
+			nvme = true
+		}
+
+		network_interface {
+			network = "default"
+		}
+
+	}`, instance)
+}
+
 func testAccComputeInstance_service_account(instance string) string {
 	return fmt.Sprintf(`
 	resource "google_compute_instance" "foobar" {

Test output

$ make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeInstance_local_ssd_nvme'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/12 19:53:38 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeInstance_local_ssd_nvme -timeout 120m
=== RUN   TestAccComputeInstance_local_ssd_nvme
--- PASS: TestAccComputeInstance_local_ssd_nvme (61.76s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	61.785s

[jbarbuto]

@helielson are you having any more luck with tests?

[helielson]

Yes, All tests are passing, included the test you'd added. Thank you.
@jbarbuto. I'll incorporate the requested changes soon.

[helielson]

Closed due hashicorp/terraform-provider-google#123

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.