Fix: Instance level stateful disk config (#1578)

* update doco * fix bug in TF code * change instance name in README to fix test * revert disk name * Update stateful.yaml * fix examples and tests --------- Co-authored-by: Julio Castillo <juliocc@gmail.com> Co-authored-by: Ludovico Magnocavallo <ludomagno@google.com>
GoogleCloudPlatform · Aug 11, 2023 · b1679ad · b1679ad
1 parent 5a12e2a
commit b1679ad
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 85 deletions.
diff --git a/modules/compute-mig/README.md b/modules/compute-mig/README.md
@@ -6,8 +6,23 @@ This module can be coupled with the [`compute-vm`](../compute-vm) module which c
 
 Stateful disks can be created directly, as shown in the last example below.
 
+<!-- BEGIN TOC -->
+- [Examples](#examples)
+  - [Simple Example](#simple-example)
+  - [Multiple Versions](#multiple-versions)
+  - [Health Check and Autohealing Policies](#health-check-and-autohealing-policies)
+  - [Autoscaling](#autoscaling)
+  - [Update Policy](#update-policy)
+  - [Stateful MIGs - MIG Config](#stateful-migs-mig-config)
+  - [Stateful MIGs - Instance Config](#stateful-migs-instance-config)
+- [Variables](#variables)
+- [Outputs](#outputs)
+<!-- END TOC -->
+
 ## Examples
 
+### Simple Example
+
 This example shows how to manage a simple MIG that leverages the `compute-vm` module to manage the underlying instance template. The following sub-examples will only show how to enable specific features of this module, and won't replicate the combined setup.
 
 ```hcl
@@ -49,7 +64,7 @@ module "nginx-mig" {
 # tftest modules=2 resources=2 inventory=simple.yaml
 ```
 
-### Multiple versions
+### Multiple Versions
 
 If multiple versions are desired, use more `compute-vm` instances for the additional templates used in each version (not shown here), and reference them like this:
 
@@ -100,7 +115,7 @@ module "nginx-mig" {
 # tftest modules=2 resources=2
 ```
 
-### Health check and autohealing policies
+### Health Check and Autohealing Policies
 
 Autohealing policies can use an externally defined health check, or have this module auto-create one:
 
@@ -205,7 +220,7 @@ module "nginx-mig" {
 # tftest modules=2 resources=3 inventory=autoscaling.yaml
 ```
 
-### Update policy
+### Update Policy
 
 ```hcl
 module "cos-nginx" {
@@ -262,40 +277,34 @@ You can configure a disk defined in the instance template to be stateful  for al
 
 An example using only the configuration at the MIG level can be seen below.
 
-Note that when referencing the stateful disk, you use `device_name` and not `disk_name`.
+Note that when referencing the stateful disk, you use `device_name` and not `disk_name`. Specifying an existing disk in the template (and stateful config) only allows a single instance to be managed by the MIG, typically coupled with an autohealing policy (shown in the examples above).
 
 ```hcl
 module "cos-nginx" {
   source = "./fabric/modules/cloud-config-container/nginx"
 }
 
 module "nginx-template" {
-  source     = "./fabric/modules/compute-vm"
-  project_id = var.project_id
-  name       = "nginx-template"
-  zone       = "europe-west1-b"
-  tags       = ["http-server", "ssh"]
+  source        = "./fabric/modules/compute-vm"
+  project_id    = "my-prj"
+  name          = "nginx-template"
+  zone          = "europe-west8-b"
+  tags          = ["http-server", "ssh"]
+  instance_type = "e2-small"
   network_interfaces = [{
     network    = var.vpc.self_link
     subnetwork = var.subnet.self_link
-    nat        = false
-    addresses  = null
   }]
   boot_disk = {
     initialize_params = {
       image = "projects/cos-cloud/global/images/family/cos-stable"
     }
   }
   attached_disks = [{
-    name        = "repd-1"
-    size        = null
     source_type = "attach"
-    source      = "regions/${var.region}/disks/repd-test-1"
-    options = {
-      mode         = "READ_ONLY"
-      replica_zone = "${var.region}-c"
-      type         = "PERSISTENT"
-    }
+    name        = "data-1"
+    size        = 10
+    source      = "test-data-1"
   }]
   create_template = true
   metadata = {
@@ -305,67 +314,48 @@ module "nginx-template" {
 
 module "nginx-mig" {
   source            = "./fabric/modules/compute-mig"
-  project_id        = "my-project"
-  location          = "europe-west1-b"
-  name              = "mig-test"
-  target_size       = 3
+  project_id        = "my-prj"
+  location          = "europe-west8-b"
+  name              = "mig-test-2"
+  target_size       = 1
   instance_template = module.nginx-template.template.self_link
-  autoscaler_config = {
-    max_replicas    = 3
-    min_replicas    = 1
-    cooldown_period = 30
-    scaling_signals = {
-      cpu_utilization = {
-        target = 0.65
-      }
-    }
-  }
   stateful_disks = {
-    repd-1 = false
+    data-1 = false
   }
 }
-# tftest modules=2 resources=3
-
+# tftest modules=2 resources=2
 ```
 
 ### Stateful MIGs - Instance Config
 
-Here is an example defining the stateful config at the instance level.
-
-Note that you will need to know the instance name in order to use this configuration.
+Here is an example defining the stateful config at the instance level. As in the example above, specifying an existing disk in the template (and stateful config) only allows a single instance to be managed by the MIG, typically coupled with an autohealing policy (shown in the examples above).
 
 ```hcl
 module "cos-nginx" {
   source = "./fabric/modules/cloud-config-container/nginx"
 }
 
 module "nginx-template" {
-  source     = "./fabric/modules/compute-vm"
-  project_id = var.project_id
-  name       = "nginx-template"
-  zone       = "europe-west1-b"
-  tags       = ["http-server", "ssh"]
+  source        = "./fabric/modules/compute-vm"
+  project_id    = "my-prj"
+  name          = "nginx-template"
+  zone          = "europe-west8-b"
+  tags          = ["http-server", "ssh"]
+  instance_type = "e2-small"
   network_interfaces = [{
     network    = var.vpc.self_link
     subnetwork = var.subnet.self_link
-    nat        = false
-    addresses  = null
   }]
   boot_disk = {
     initialize_params = {
       image = "projects/cos-cloud/global/images/family/cos-stable"
     }
   }
   attached_disks = [{
-    name        = "repd-1"
-    size        = null
     source_type = "attach"
-    source      = "regions/${var.region}/disks/repd-test-1"
-    options = {
-      mode         = "READ_ONLY"
-      replica_zone = "${var.region}-c"
-      type         = "PERSISTENT"
-    }
+    name        = "data-1"
+    size        = 10
+    source      = "test-data-1"
   }]
   create_template = true
   metadata = {
@@ -375,30 +365,18 @@ module "nginx-template" {
 
 module "nginx-mig" {
   source            = "./fabric/modules/compute-mig"
-  project_id        = "my-project"
-  location          = "europe-west1-b"
+  project_id        = "my-prj"
+  location          = "europe-west8-b"
   name              = "mig-test"
-  target_size       = 3
   instance_template = module.nginx-template.template.self_link
-  autoscaler_config = {
-    max_replicas    = 3
-    min_replicas    = 1
-    cooldown_period = 30
-    scaling_signals = {
-      cpu_utilization = {
-        target = 0.65
-      }
-    }
-  }
   stateful_config = {
-    # name needs to match a MIG instance name
     instance-1 = {
       minimal_action                 = "NONE",
       most_disruptive_allowed_action = "REPLACE"
       preserved_state = {
         disks = {
-          persistent-disk-1 = {
-            source = "test-disk",
+          data-1 = {
+            source = "projects/my-prj/zones/europe-west8-b/disks/test-data-1"
           }
         }
         metadata = {
@@ -408,8 +386,7 @@ module "nginx-mig" {
     }
   }
 }
-# tftest modules=2 resources=4 inventory=stateful.yaml
-
+# tftest modules=2 resources=3 inventory=stateful.yaml
 ```
 <!-- BEGIN TFDOC -->
 

diff --git a/modules/compute-mig/stateful-config.tf b/modules/compute-mig/stateful-config.tf
@@ -22,7 +22,7 @@ resource "google_compute_per_instance_config" "default" {
   zone     = var.location
   name     = each.key
   instance_group_manager = try(
-    google_compute_instance_group_manager.default.0.id, null
+    google_compute_instance_group_manager.default.0.name, null
   )
   minimal_action                   = each.value.minimal_action
   most_disruptive_allowed_action   = each.value.most_disruptive_action
@@ -59,7 +59,7 @@ resource "google_compute_region_per_instance_config" "default" {
   region   = var.location
   name     = each.key
   region_instance_group_manager = try(
-    google_compute_region_instance_group_manager.default.0.id, null
+    google_compute_region_instance_group_manager.default.0.name, null
   )
   minimal_action                   = each.value.minimal_action
   most_disruptive_allowed_action   = each.value.most_disruptive_action

diff --git a/modules/compute-vm/README.md b/modules/compute-vm/README.md
@@ -626,7 +626,6 @@ module "instance" {
 # tftest modules=1 resources=5 inventory=snapshot-schedule-create.yaml
 ```
 <!-- BEGIN TFDOC -->
-
 ## Variables
 
 | name | description | type | required | default |
@@ -636,7 +635,7 @@ module "instance" {
 | [project_id](variables.tf#L277) | Project id. | <code>string</code> | ✓ |  |
 | [zone](variables.tf#L379) | Compute zone. | <code>string</code> | ✓ |  |
 | [attached_disk_defaults](variables.tf#L17) | Defaults for attached disks options. | <code title="object&#40;&#123;&#10;  auto_delete  &#61; optional&#40;bool, false&#41;&#10;  mode         &#61; string&#10;  replica_zone &#61; string&#10;  type         &#61; string&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> |  | <code title="&#123;&#10;  auto_delete  &#61; true&#10;  mode         &#61; &#34;READ_WRITE&#34;&#10;  replica_zone &#61; null&#10;  type         &#61; &#34;pd-balanced&#34;&#10;&#125;">&#123;&#8230;&#125;</code> |
-| [attached_disks](variables.tf#L38) | Additional disks, if options is null defaults will be used in its place. Source type is one of 'image' (zonal disks in vms and template), 'snapshot' (vm), 'existing', and null. | <code title="list&#40;object&#40;&#123;&#10;  name              &#61; string&#10;  device_name       &#61; optional&#40;string&#41;&#10;  size              &#61; string&#10;  snapshot_schedule &#61; optional&#40;string&#41;&#10;  source            &#61; optional&#40;string&#41;&#10;  source_type       &#61; optional&#40;string&#41;&#10;  options &#61; optional&#40;&#10;    object&#40;&#123;&#10;      auto_delete  &#61; optional&#40;bool, false&#41;&#10;      mode         &#61; optional&#40;string, &#34;READ_WRITE&#34;&#41;&#10;      replica_zone &#61; optional&#40;string&#41;&#10;      type         &#61; optional&#40;string, &#34;pd-balanced&#34;&#41;&#10;    &#125;&#41;,&#10;    &#123;&#10;      auto_delete  &#61; true&#10;      mode         &#61; &#34;READ_WRITE&#34;&#10;      replica_zone &#61; null&#10;      type         &#61; &#34;pd-balanced&#34;&#10;    &#125;&#10;  &#41;&#10;&#125;&#41;&#41;">list&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> |  | <code>&#91;&#93;</code> |
+| [attached_disks](variables.tf#L37) | Additional disks, if options is null defaults will be used in its place. Source type is one of 'image' (zonal disks in vms and template), 'snapshot' (vm), 'existing', and null. | <code title="list&#40;object&#40;&#123;&#10;  name        &#61; string&#10;  device_name &#61; optional&#40;string&#41;&#10;  size              &#61; string&#10;  snapshot_schedule &#61; optional&#40;string&#41;&#10;  source            &#61; optional&#40;string&#41;&#10;  source_type       &#61; optional&#40;string&#41;&#10;  options &#61; optional&#40;&#10;    object&#40;&#123;&#10;      auto_delete  &#61; optional&#40;bool, false&#41;&#10;      mode         &#61; optional&#40;string, &#34;READ_WRITE&#34;&#41;&#10;      replica_zone &#61; optional&#40;string&#41;&#10;      type         &#61; optional&#40;string, &#34;pd-balanced&#34;&#41;&#10;    &#125;&#41;,&#10;    &#123;&#10;      auto_delete  &#61; true&#10;      mode         &#61; &#34;READ_WRITE&#34;&#10;      replica_zone &#61; null&#10;      type         &#61; &#34;pd-balanced&#34;&#10;    &#125;&#10;  &#41;&#10;&#125;&#41;&#41;">list&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> |  | <code>&#91;&#93;</code> |
 | [boot_disk](variables.tf#L83) | Boot disk properties. | <code title="object&#40;&#123;&#10;  auto_delete       &#61; optional&#40;bool, true&#41;&#10;  snapshot_schedule &#61; optional&#40;string&#41;&#10;  source            &#61; optional&#40;string&#41;&#10;  initialize_params &#61; optional&#40;object&#40;&#123;&#10;    image &#61; optional&#40;string, &#34;projects&#47;debian-cloud&#47;global&#47;images&#47;family&#47;debian-11&#34;&#41;&#10;    size  &#61; optional&#40;number, 10&#41;&#10;    type  &#61; optional&#40;string, &#34;pd-balanced&#34;&#41;&#10;  &#125;&#41;&#41;&#10;  use_independent_disk &#61; optional&#40;bool, false&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> |  | <code title="&#123;&#10;  initialize_params &#61; &#123;&#125;&#10;&#125;">&#123;&#8230;&#125;</code> |
 | [can_ip_forward](variables.tf#L117) | Enable IP forwarding. | <code>bool</code> |  | <code>false</code> |
 | [confidential_compute](variables.tf#L123) | Enable Confidential Compute for these instances. | <code>bool</code> |  | <code>false</code> |
@@ -678,7 +677,6 @@ module "instance" {
 | [service_account_iam_email](outputs.tf#L74) | Service account email. |  |
 | [template](outputs.tf#L82) | Template resource. |  |
 | [template_name](outputs.tf#L87) | Template name. |  |
-
 <!-- END TFDOC -->
 ## TODO
 

diff --git a/modules/compute-vm/variables.tf b/modules/compute-vm/variables.tf
@@ -28,7 +28,6 @@ variable "attached_disk_defaults" {
     replica_zone = null
     type         = "pd-balanced"
   }
-
   validation {
     condition     = var.attached_disk_defaults.mode == "READ_WRITE" || !var.attached_disk_defaults.auto_delete
     error_message = "auto_delete can only be specified on READ_WRITE disks."
@@ -38,8 +37,9 @@ variable "attached_disk_defaults" {
 variable "attached_disks" {
   description = "Additional disks, if options is null defaults will be used in its place. Source type is one of 'image' (zonal disks in vms and template), 'snapshot' (vm), 'existing', and null."
   type = list(object({
-    name              = string
-    device_name       = optional(string)
+    name        = string
+    device_name = optional(string)
+    # TODO: size can be null when source_type is attach
     size              = string
     snapshot_schedule = optional(string)
     source            = optional(string)

diff --git a/tests/modules/compute_mig/examples/stateful.yaml b/tests/modules/compute_mig/examples/stateful.yaml
@@ -13,25 +13,49 @@
 # limitations under the License.
 
 values:
+  module.nginx-mig.google_compute_instance_group_manager.default[0]:
+    all_instances_config: []
+    auto_healing_policies: []
+    base_instance_name: mig-test
+    description: Terraform managed.
+    list_managed_instances_results: PAGELESS
+    name: mig-test
+    named_port: []
+    project: my-prj
+    stateful_disk: []
+    stateful_external_ip: []
+    stateful_internal_ip: []
+    target_pools: null
+    timeouts: null
+    version:
+    - name: default
+      target_size: []
+    wait_for_instances: false
+    wait_for_instances_status: STABLE
+    zone: europe-west8-b
   module.nginx-mig.google_compute_per_instance_config.default["instance-1"]:
+    instance_group_manager: mig-test
     minimal_action: NONE
     most_disruptive_allowed_action: REPLACE
     name: instance-1
     preserved_state:
     - disk:
       - delete_rule: NEVER
-        device_name: persistent-disk-1
+        device_name: data-1
         mode: READ_WRITE
-        source: test-disk
+        source: projects/my-prj/zones/europe-west8-b/disks/test-data-1
       metadata:
         foo: bar
-    project: my-project
+    project: my-prj
     remove_instance_state_on_destroy: false
     timeouts: null
-    zone: europe-west1-b
+    zone: europe-west8-b
 
 counts:
-  google_compute_autoscaler: 1
   google_compute_instance_group_manager: 1
   google_compute_instance_template: 1
   google_compute_per_instance_config: 1
+  modules: 2
+  resources: 3
+
+outputs: {}