Refactoring in gke persistent module

Author: vikramvs-gg

modules/compute/gke-job-template/README.md

@@ -114,7 +114,7 @@ No modules.
 | <a name="input_node_count"></a> [node\_count](#input\_node\_count) | How many nodes the job should run in parallel. | `number` | `1` | no |
 | <a name="input_node_pool_names"></a> [node\_pool\_names](#input\_node\_pool\_names) | A list of node pool names on which to run the job. Can be populated via `use` field. | `list(string)` | `[]` | no |
 | <a name="input_node_selectors"></a> [node\_selectors](#input\_node\_selectors) | A list of node selectors to use to place the job. | <pre>list(object({<br/>    key   = string<br/>    value = string<br/>  }))</pre> | `[]` | no |
-| <a name="input_persistent_volume_claims"></a> [persistent\_volume\_claims](#input\_persistent\_volume\_claims) | A list of objects that describes a k8s PVC that is to be used and mounted on the job. Generally supplied by the gke-persistent-volume module. | <pre>list(object({<br/>    name          = string<br/>    mount_path    = string<br/>    mount_options = string<br/>    is_gcs        = bool<br/>  }))</pre> | `[]` | no |
+| <a name="input_persistent_volume_claims"></a> [persistent\_volume\_claims](#input\_persistent\_volume\_claims) | A list of objects that describes a k8s PVC that is to be used and mounted on the job. Generally supplied by the gke-persistent-volume module. | <pre>list(object({<br/>    name          = string<br/>    namespace     = string<br/>    mount_path    = string<br/>    mount_options = string<br/>    storage_type  = string<br/>  }))</pre> | `[]` | no |
 | <a name="input_random_name_sufix"></a> [random\_name\_sufix](#input\_random\_name\_sufix) | Appends a random suffix to the job name to avoid clashes. | `bool` | `true` | no |
 | <a name="input_requested_cpu_per_pod"></a> [requested\_cpu\_per\_pod](#input\_requested\_cpu\_per\_pod) | The requested cpu per pod. If null, allocatable\_cpu\_per\_node will be used to claim whole nodes. If provided will override allocatable\_cpu\_per\_node. | `number` | `-1` | no |
 | <a name="input_requested_gpu_per_pod"></a> [requested\_gpu\_per\_pod](#input\_requested\_gpu\_per\_pod) | The requested gpu per pod. If null, allocatable\_gpu\_per\_node will be used to claim whole nodes. If provided will override allocatable\_gpu\_per\_node. | `number` | `-1` | no |

modules/compute/gke-job-template/main.tf

@@ -110,7 +110,7 @@ locals {
   node_selectors = concat(local.machine_family_node_selector, local.local_ssd_node_selector, var.node_selectors)
 
   any_gcs = anytrue([for pvc in var.persistent_volume_claims :
-    pvc.is_gcs
+    pvc.storage_type == "gcs"
   ])
 
   job_template_contents = templatefile(

modules/compute/gke-job-template/variables.tf

@@ -150,9 +150,10 @@ variable "persistent_volume_claims" {
   description = "A list of objects that describes a k8s PVC that is to be used and mounted on the job. Generally supplied by the gke-persistent-volume module."
   type = list(object({
     name          = string
+    namespace     = string
     mount_path    = string
     mount_options = string
-    is_gcs        = bool
+    storage_type  = string
   }))
   default = []
 }

modules/file-system/gke-persistent-volume/README.md

@@ -123,15 +123,13 @@ limitations under the License.
 | <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.0 |
 | <a name="requirement_google"></a> [google](#requirement\_google) | >= 4.42 |
 | <a name="requirement_kubectl"></a> [kubectl](#requirement\_kubectl) | >= 1.7.0 |
-| <a name="requirement_local"></a> [local](#requirement\_local) | >= 2.0.0 |
 
 ## Providers
 
 | Name | Version |
 |------|---------|
 | <a name="provider_google"></a> [google](#provider\_google) | >= 4.42 |
 | <a name="provider_kubectl"></a> [kubectl](#provider\_kubectl) | >= 1.7.0 |
-| <a name="provider_local"></a> [local](#provider\_local) | >= 2.0.0 |
 
 ## Modules
 
@@ -144,7 +142,6 @@ No modules.
 | [kubectl_manifest.pv](https://registry.terraform.io/providers/gavinbunney/kubectl/latest/docs/resources/manifest) | resource |
 | [kubectl_manifest.pvc](https://registry.terraform.io/providers/gavinbunney/kubectl/latest/docs/resources/manifest) | resource |
 | [kubectl_manifest.pvc_namespace](https://registry.terraform.io/providers/gavinbunney/kubectl/latest/docs/resources/manifest) | resource |
-| [local_file.debug_file](https://registry.terraform.io/providers/hashicorp/local/latest/docs/resources/file) | resource |
 | [google_client_config.default](https://registry.terraform.io/providers/hashicorp/google/latest/docs/data-sources/client_config) | data source |
 | [google_container_cluster.gke_cluster](https://registry.terraform.io/providers/hashicorp/google/latest/docs/data-sources/container_cluster) | data source |
 
@@ -167,6 +164,6 @@ No modules.
 
 | Name | Description |
 |------|-------------|
-| <a name="output_persistent_volume_claims"></a> [persistent\_volume\_claims](#output\_persistent\_volume\_claims) | An object that describes a k8s PVC created by this module. |
-| <a name="output_pvc_name"></a> [pvc\_name](#output\_pvc\_name) | The name for k8s PVC created by this module. |
+| <a name="output_persistent_volume_claims"></a> [persistent\_volume\_claims](#output\_persistent\_volume\_claims) | An object describing the Kubernetes PersistentVolumeClaim created by this module. |
+| <a name="output_pvc_name"></a> [pvc\_name](#output\_pvc\_name) | The name of the Kubernetes PVC created by this module. |
 <!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

modules/file-system/gke-persistent-volume/main.tf

@@ -20,119 +20,102 @@ locals {
 }
 
 locals {
-  is_gcs    = (var.gcs_bucket_name != null)
-  is_lustre = (var.lustre_id != null)
+  # Flags indicating which storage type is active based on input variables.
+  storage_type_active = {
+    gcs       = var.gcs_bucket_name != null
+    lustre    = var.lustre_id != null
+    filestore = var.filestore_id != null
+  }
 
-  filestore_id = (
-    !local.is_gcs && !local.is_lustre ?              # If not using gcs or lustre
-    var.filestore_id :                               # Then filestore_id must be provided
-    "projects/empty/locations/empty/instances/empty" # Otherwise use something arbitrary as it will not be used
-  )
+  # Determine the active storage type name.
+  active_types = [for type, is_active in local.storage_type_active : type if is_active]
 
-  lustre_id = (
-    local.is_lustre ?
-    var.lustre_id :
-    "projects/empty/locations/empty/instances/empty"
-  )
-  location             = local.is_lustre ? split("/", local.lustre_id)[3] : split("/", local.filestore_id)[3]
-  filestore_name       = split("/", local.filestore_id)[5]
-  filestore_share_name = trimprefix(var.network_storage.remote_mount, "/")
-
-  # Determine the base_name based on which storage type is used
-  base_name          = local.is_gcs ? var.gcs_bucket_name : (local.is_lustre ? split("/", local.lustre_id)[5] : local.filestore_name)
-  pv_name            = var.pv_name != null ? var.pv_name : "${local.base_name}-pv"
-  pvc_name           = var.pvc_name != null ? var.pvc_name : "${local.base_name}-pvc"
-  list_mount_options = split(",", var.network_storage.mount_options)
-
-  filestore_pv_contents = templatefile(
-    "${path.module}/templates/filestore-pv.yaml.tftpl",
-    {
-      pv_name        = local.pv_name
-      capacity       = "${var.capacity_gib}Gi"
-      location       = local.location
-      filestore_name = local.filestore_name
-      share_name     = local.filestore_share_name
-      ip_address     = var.network_storage.server_ip
-      labels         = local.labels
-      pvc_name       = local.pvc_name
-      namespace      = var.namespace
-    }
-  )
+  # The precondition in kubectl_manifest.pv ensures exactly one type is active.
+  storage_type = length(local.active_types) > 0 ? local.active_types[0] : "unknown"
 
-  filestore_pvc_contents = templatefile(
-    "${path.module}/templates/filestore-pvc.yaml.tftpl",
-    {
-      pv_name   = local.pv_name
-      capacity  = "${var.capacity_gib}Gi"
-      pvc_name  = local.pvc_name
-      labels    = local.labels
-      namespace = var.namespace
-    }
-  )
+  # Map containing the base name derivation logic for each storage type.
+  base_name_map = {
+    gcs       = var.gcs_bucket_name
+    lustre    = var.lustre_id != null ? split("/", var.lustre_id)[5] : null
+    filestore = var.filestore_id != null ? split("/", var.filestore_id)[5] : null
+  }
+  # Retrieve the base name for the active storage type.
+  base_name = local.base_name_map[local.storage_type]
+
+  # PV and PVC names
+  pv_name  = var.pv_name != null ? var.pv_name : "${local.base_name}-pv"
+  pvc_name = var.pvc_name != null ? var.pvc_name : "${local.base_name}-pvc"
+
+  # Template file paths
+  pv_templates = {
+    gcs       = "${path.module}/templates/gcs-pv.yaml.tftpl"
+    lustre    = "${path.module}/templates/managed-lustre-pv.yaml.tftpl"
+    filestore = "${path.module}/templates/filestore-pv.yaml.tftpl"
+  }
+  pvc_templates = {
+    gcs       = "${path.module}/templates/gcs-pvc.yaml.tftpl"
+    lustre    = "${path.module}/templates/managed-lustre-pvc.yaml.tftpl"
+    filestore = "${path.module}/templates/filestore-pvc.yaml.tftpl"
+  }
 
-  gcs_pv_contents = templatefile(
-    "${path.module}/templates/gcs-pv.yaml.tftpl",
-    {
-      pv_name       = local.pv_name
-      capacity      = "${var.capacity_gib}Gi"
-      labels        = local.labels
-      mount_options = local.is_gcs ? local.list_mount_options : null
-      bucket_name   = local.is_gcs ? var.gcs_bucket_name : ""
-      namespace     = var.namespace
-      pvc_name      = local.pvc_name
-    }
-  )
+  # Common variables for all PVC templates
+  common_pvc_vars = {
+    pv_name   = local.pv_name
+    pvc_name  = local.pvc_name
+    labels    = local.labels
+    capacity  = "${var.capacity_gib}Gi"
+    namespace = var.namespace
+  }
 
-  gcs_pvc_contents = templatefile(
-    "${path.module}/templates/gcs-pvc.yaml.tftpl",
-    {
-      pv_name   = local.pv_name
-      pvc_name  = local.pvc_name
-      labels    = local.labels
-      capacity  = "${var.capacity_gib}Gi"
-      namespace = var.namespace
-    }
-  )
+  # Common variables for all PV templates
+  common_pv_vars = {
+    pv_name  = local.pv_name
+    capacity = "${var.capacity_gib}Gi"
+    labels   = local.labels
+  }
 
-  lustre_pv_contents = templatefile(
-    "${path.module}/templates/managed-lustre-pv.yaml.tftpl",
-    {
-      pv_name         = local.pv_name
-      capacity        = "${var.capacity_gib}Gi"
-      location        = local.location
+  # Variables for PV templates, merging common vars with type-specific ones.
+  pv_template_vars = {
+    gcs = merge(local.common_pv_vars, {
+      mount_options = var.gcs_bucket_name != null ? split(",", var.network_storage.mount_options) : []
+      bucket_name   = var.gcs_bucket_name
+      namespace     = var.namespace
+      pvc_name      = local.pvc_name
+    })
+    lustre = merge(local.common_pv_vars, {
+      location        = var.lustre_id != null ? split("/", var.lustre_id)[3] : null
       project         = split("/", var.cluster_id)[1]
-      labels          = local.labels
       instance_name   = local.base_name
-      server_ip       = split("@", var.network_storage.server_ip)[0]
+      server_ip       = var.lustre_id != null ? split("@", var.network_storage.server_ip)[0] : null
       filesystem_name = var.network_storage.remote_mount
       pvc_name        = local.pvc_name
       namespace       = var.namespace
-    }
-  )
+    })
+    filestore = merge(local.common_pv_vars, {
+      location       = var.filestore_id != null ? split("/", var.filestore_id)[3] : null
+      filestore_name = local.base_name
+      share_name     = trimprefix(var.network_storage.remote_mount, "/")
+      ip_address     = var.network_storage.server_ip
+      pvc_name       = local.pvc_name
+      namespace      = var.namespace
+    })
+  }
 
-  lustre_pvc_contents = templatefile(
-    "${path.module}/templates/managed-lustre-pvc.yaml.tftpl",
-    {
-      pv_name   = local.pv_name
-      pvc_name  = local.pvc_name
-      labels    = local.labels
-      capacity  = "${var.capacity_gib}Gi"
-      namespace = var.namespace
-    }
+  # Rendered YAML contents
+  pv_content = templatefile(
+    local.pv_templates[local.storage_type],
+    local.pv_template_vars[local.storage_type]
+  )
+  pvc_content = templatefile(
+    local.pvc_templates[local.storage_type],
+    local.common_pvc_vars
   )
 
+  # GKE Cluster details
   cluster_name     = split("/", var.cluster_id)[5]
   cluster_location = split("/", var.cluster_id)[3]
 }
 
-resource "local_file" "debug_file" {
-  content  = <<-EOF
-    ${local.filestore_pv_contents}
-    ${local.filestore_pvc_contents}
-    EOF
-  filename = "${path.root}/pv-pvc-debug-file-${local.filestore_name}.yaml"
-}
-
 data "google_container_cluster" "gke_cluster" {
   name     = local.cluster_name
   location = local.cluster_location
@@ -156,17 +139,17 @@ resource "kubectl_manifest" "pvc_namespace" {
 }
 
 resource "kubectl_manifest" "pv" {
-  yaml_body = local.is_gcs ? local.gcs_pv_contents : (local.is_lustre ? local.lustre_pv_contents : local.filestore_pv_contents)
+  yaml_body = local.pv_content
 
   lifecycle {
     precondition {
-      condition     = ((var.gcs_bucket_name != null ? 1 : 0) + (var.filestore_id != null ? 1 : 0) + (var.lustre_id != null ? 1 : 0)) == 1
-      error_message = "Either gcs_bucket_name, filestore_id, or lustre_id must be set."
+      condition     = length(local.active_types) == 1
+      error_message = "Exactly one of gcs_bucket_name, filestore_id, or lustre_id must be set."
     }
   }
 }
 
 resource "kubectl_manifest" "pvc" {
-  yaml_body  = local.is_gcs ? local.gcs_pvc_contents : (local.is_lustre ? local.lustre_pvc_contents : local.filestore_pvc_contents)
+  yaml_body  = local.pvc_content
   depends_on = [kubectl_manifest.pv, kubectl_manifest.pvc_namespace]
 }

modules/file-system/gke-persistent-volume/outputs.tf

@@ -15,17 +15,17 @@
   */
 
 output "persistent_volume_claims" {
-  description = "An object that describes a k8s PVC created by this module."
+  description = "An object describing the Kubernetes PersistentVolumeClaim created by this module."
   value = {
     name          = local.pvc_name
+    namespace     = var.namespace
     mount_path    = var.network_storage.local_mount
     mount_options = var.network_storage.mount_options
-    is_gcs        = local.is_gcs
+    storage_type  = local.storage_type
   }
-  depends_on = [kubectl_manifest.pvc]
 }
 
 output "pvc_name" {
-  description = "The name for k8s PVC created by this module."
+  description = "The name of the Kubernetes PVC created by this module."
   value       = local.pvc_name
 }

modules/file-system/gke-persistent-volume/versions.tf

@@ -23,10 +23,6 @@ terraform {
       source  = "gavinbunney/kubectl"
       version = ">= 1.7.0"
     }
-    local = {
-      source  = "hashicorp/local"
-      version = ">= 2.0.0"
-    }
   }
   provider_meta "google" {
     module_name = "blueprints/terraform/hpc-toolkit:gke-persistent-volume/v1.65.0"