-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow specifying accelerators in cluster node_config (#1067) #1115
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,28 @@ var schemaNodeConfig = &schema.Schema{ | |
ValidateFunc: validation.IntAtLeast(10), | ||
}, | ||
|
||
"guest_accelerator": &schema.Schema{ | ||
Type: schema.TypeList, | ||
Optional: true, | ||
Computed: true, | ||
ForceNew: true, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"count": &schema.Schema{ | ||
Type: schema.TypeInt, | ||
Required: true, | ||
ForceNew: true, | ||
}, | ||
"type": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
DiffSuppressFunc: linkDiffSuppress, | ||
}, | ||
}, | ||
}, | ||
}, | ||
|
||
"image_type": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
|
@@ -128,6 +150,22 @@ func expandNodeConfig(v interface{}) *container.NodeConfig { | |
nc.MachineType = v.(string) | ||
} | ||
|
||
if v, ok := nodeConfig["guest_accelerator"]; ok { | ||
accels := v.([]interface{}) | ||
guestAccelerators := make([]*container.AcceleratorConfig, 0, len(accels)) | ||
for _, raw := range accels { | ||
data := raw.(map[string]interface{}) | ||
if data["count"].(int) == 0 { | ||
continue | ||
} | ||
guestAccelerators = append(guestAccelerators, &container.AcceleratorConfig{ | ||
AcceleratorCount: int64(data["count"].(int)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be able to do data["count"].(int64) - why do it this way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Side note: line 162 is basically copied from https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_instance_template.go#L512) |
||
AcceleratorType: data["type"].(string), | ||
}) | ||
} | ||
nc.Accelerators = guestAccelerators | ||
} | ||
|
||
if v, ok := nodeConfig["disk_size_gb"]; ok { | ||
nc.DiskSizeGb = int64(v.(int)) | ||
} | ||
|
@@ -196,16 +234,17 @@ func flattenNodeConfig(c *container.NodeConfig) []map[string]interface{} { | |
} | ||
|
||
config = append(config, map[string]interface{}{ | ||
"machine_type": c.MachineType, | ||
"disk_size_gb": c.DiskSizeGb, | ||
"local_ssd_count": c.LocalSsdCount, | ||
"service_account": c.ServiceAccount, | ||
"metadata": c.Metadata, | ||
"image_type": c.ImageType, | ||
"labels": c.Labels, | ||
"tags": c.Tags, | ||
"preemptible": c.Preemptible, | ||
"min_cpu_platform": c.MinCpuPlatform, | ||
"machine_type": c.MachineType, | ||
"disk_size_gb": c.DiskSizeGb, | ||
"guest_accelerator": c.Accelerators, | ||
"local_ssd_count": c.LocalSsdCount, | ||
"service_account": c.ServiceAccount, | ||
"metadata": c.Metadata, | ||
"image_type": c.ImageType, | ||
"labels": c.Labels, | ||
"tags": c.Tags, | ||
"preemptible": c.Preemptible, | ||
"min_cpu_platform": c.MinCpuPlatform, | ||
}) | ||
|
||
if len(c.OauthScopes) > 0 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,28 @@ func TestAccContainerNodePool_withNodeConfig(t *testing.T) { | |
}) | ||
} | ||
|
||
func TestAccContainerNodePool_withGPU(t *testing.T) { | ||
t.Parallel() | ||
|
||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckContainerNodePoolDestroy, | ||
Steps: []resource.TestStep{ | ||
resource.TestStep{ | ||
Config: testAccContainerNodePool_withGPU(), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckContainerNodePoolMatches("google_container_node_pool.np_with_gpu"), | ||
), | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks great - can you stick an ImportState step on there so we know that accelerated node pools can be imported? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be the only ImportState in this file - why introduce it for just this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've talked to Hashicorp about best practices recently, and so it's a new thing we're doing with new tests. |
||
resource.TestStep{ | ||
ResourceName: "google_container_node_pool.np_with_gpu", | ||
ImportState: true, | ||
}, | ||
}, | ||
}) | ||
} | ||
|
||
func TestAccContainerNodePool_withManagement(t *testing.T) { | ||
t.Parallel() | ||
|
||
|
@@ -315,6 +337,18 @@ func testAccCheckContainerNodePoolMatches(n string) resource.TestCheckFunc { | |
|
||
} | ||
|
||
tfGA := attributes["node_config.0.guest_accelerator.#"] == "1" | ||
if gcpGA := nodepool.Config.Accelerators != nil && len(nodepool.Config.Accelerators) == 1; tfGA != gcpGA { | ||
if tf := attributes["node_config.0.guest_accelerator.0.type"]; nodepool.Config.Accelerators[0].AcceleratorType != tf { | ||
return fmt.Errorf("Mismatched NodeConfig.Accelerators type. TF State: %s. GCP State: %s", | ||
tf, nodepool.Config.Accelerators[0].AcceleratorType) | ||
} | ||
if tf := attributes["node_config.0.guest_accelerator.0.count"]; strconv.FormatInt(nodepool.Config.Accelerators[0].AcceleratorCount, 10) != tf { | ||
return fmt.Errorf("Mismatched NodeConfig.Accelerators count. TF State: %s. GCP State: %d", | ||
tf, nodepool.Config.Accelerators[0].AcceleratorCount) | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
} | ||
|
@@ -583,6 +617,44 @@ resource "google_container_node_pool" "np_with_node_config" { | |
}`, acctest.RandString(10), acctest.RandString(10)) | ||
} | ||
|
||
func testAccContainerNodePool_withGPU() string { | ||
return fmt.Sprintf(` | ||
resource "google_container_cluster" "cluster" { | ||
name = "tf-cluster-nodepool-test-%s" | ||
zone = "us-central1-c" | ||
initial_node_count = 1 | ||
node_version = "1.9.2-gke.1" | ||
min_master_version = "1.9.2-gke.1" | ||
} | ||
resource "google_container_node_pool" "np_with_gpu" { | ||
name = "tf-nodepool-test-%s" | ||
zone = "us-central1-c" | ||
cluster = "${google_container_cluster.cluster.name}" | ||
initial_node_count = 1 | ||
node_config { | ||
machine_type = "n1-standard-1" | ||
disk_size_gb = 10 | ||
oauth_scopes = [ | ||
"https://www.googleapis.com/auth/devstorage.read_only", | ||
"https://www.googleapis.com/auth/logging.write", | ||
"https://www.googleapis.com/auth/monitoring", | ||
"https://www.googleapis.com/auth/service.management.readonly", | ||
"https://www.googleapis.com/auth/servicecontrol", | ||
"https://www.googleapis.com/auth/trace.append" | ||
] | ||
preemptible = true | ||
service_account = "default" | ||
image_type = "COS" | ||
guest_accelerator = [ | ||
{ | ||
type = "nvidia-tesla-k80" | ||
count = 1 | ||
} | ||
] | ||
} | ||
}`, acctest.RandString(10), acctest.RandString(10)) | ||
} | ||
|
||
func testAccContainerNodePool_withNodeConfigScopeAlias() string { | ||
return fmt.Sprintf(` | ||
resource "google_container_cluster" "cluster" { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,10 @@ resource "google_container_cluster" "primary" { | |
"https://www.googleapis.com/auth/logging.write", | ||
"https://www.googleapis.com/auth/monitoring", | ||
] | ||
guest_accelerator = [{ | ||
type="nvidia-tesla-k80" | ||
count=1 | ||
}] | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'll need to add the details of this structure to the documentation below here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you could make accels a []map[string]interface{}, which would mean you'd only have to do one of these casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making accels a
[]map[string]interface{}
leads to an error:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Side note: Line 154 is basically copied from https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_instance_template.go#L504)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, all right, that makes sense.