[siddhartha-quad]

Develop to main test PR

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces significant enhancements to the GKE orchestrator, focusing on storage management and resource provisioning. It adds robust pre-flight checks for GCSFuse storage profiles to prevent deployment failures, implements automated lifecycle management for storage gateways to prevent resource leakage, and enables instance flexibility policies for MIG NodeSets to improve scheduling success rates. Additionally, it includes dependency updates and refined error handling for telemetry.

Highlights

• GKE Storage Profiles: Added pre-flight validation for GCSFuse storage profiles, ensuring cluster compatibility and bucket region co-location requirements are met before job submission.
• Storage Gateway Management: Implemented automatic cleanup of unused storage gateways (PVCs/PVs) after job cancellation or completion, with logic to protect gateways during concurrent submissions.
• Instance Flexibility Policy: Introduced support for machine-type fallback in MIG NodeSets, allowing the orchestrator to automatically select from a prioritized list of machine types based on capacity.
• Dependency Updates: Updated various Go and Python dependencies, including Kubernetes client libraries and Google Cloud SDKs, to ensure compatibility and security.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces an internal application load balancer module (int-app-lb) and adds a high-availability Slurm cluster example (hpc-slurm-ha-lb.yaml). It also implements an instance_flexibility_policy for machine-type fallback in MIG NodeSets, alongside GKE storage profile preflight checks and gateway cleanup. The code review feedback highlights several areas for improvement: the new example blueprint must be indexed in examples/README.md per the style guide; several modules in the new blueprint should utilize the use block rather than explicit variable linking; a potential evaluation error in the nodeset's GPU signature logic should be guarded with try(); and the load balancer's proxy target selection should use the one() function instead of direct indexing to prevent out-of-bounds errors.

[gemini-code-assist]

The new example blueprint community/examples/hpc-slurm-ha-lb.yaml has been added to the repository but is missing from the index in examples/README.md. Please add it to the index to ensure it is discoverable.

References

1. If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

In Terraform, the && operator does not short-circuit to prevent evaluation of the right-hand side. If var.machine_type is not present in local.flex_machine_types, accessing module.fallback_gpu[mt] will fail with a 'key not found' error during evaluation. Use try() to safely handle potentially missing keys.

      length(try(module.fallback_gpu[mt].guest_accelerator, [])) > 0 ? "${replace(module.fallback_gpu[mt].guest_accelerator[0].type, "/^.*\\//", "")}:${module.fallback_gpu[mt].guest_accelerator[0].count}" :

[gemini-code-assist]

According to the Cluster Toolkit style guide, the use block is preferred for module dependencies within blueprints to allow automatic propagation of settings (like subnetwork_self_link) and prevent deployment race conditions. Explicit variable linking should only be used when necessary to resolve ambiguity.

  - id: login_nodes
    source: community/modules/scheduler/schedmd-slurm-gcp-v6-login
    use: [network]
    settings:
      name_prefix: login
      num_instances: 1
      machine_type: n1-standard-2
      service_account_email: $(login_sa.service_account_email)
      service_account_scopes: ["https://www.googleapis.com/auth/cloud-platform"]
      enable_oslogin: true
      enable_shielded_vm: true
      enable_login_public_ips: false # NO EXTERNAL IP

References

1. Ensure the use block is preferred for module dependencies within blueprints. Explicit variable linking should only be used when necessary to resolve ambiguity. ^(link)

[gemini-code-assist]

According to the Cluster Toolkit style guide, the use block is preferred for module dependencies within blueprints to allow automatic propagation of settings (like subnetwork_self_link) and prevent deployment race conditions. Explicit variable linking should only be used when necessary to resolve ambiguity.

  - id: compute_nodes
    source: community/modules/compute/schedmd-slurm-gcp-v6-nodeset
    use: [network]
    settings:
      name: a2-nodes
      node_count_dynamic_max: 4
      node_count_static: 0
      machine_type: n1-standard-2
      service_account_email: $(compute_sa.service_account_email)
      service_account_scopes: ["https://www.googleapis.com/auth/cloud-platform"]
      enable_oslogin: true
      enable_shielded_vm: true
      enable_placement: false

References

1. Ensure the use block is preferred for module dependencies within blueprints. Explicit variable linking should only be used when necessary to resolve ambiguity. ^(link)

[gemini-code-assist]

According to the Cluster Toolkit style guide, the use block is preferred for module dependencies within blueprints to allow automatic propagation of settings (like subnetwork_self_link and network_id) and prevent deployment race conditions. Explicit variable linking should only be used when necessary to resolve ambiguity.

  - id: slurm_db
    source: community/modules/database/slurm-cloudsql-federation
    use: [network]
    settings:
      deployment_name: $(vars.deployment_name)
      project_id: $(vars.project_id)
      region: $(vars.region)
      tier: db-f1-micro
      sql_instance_name: $(vars.deployment_name)-db
      use_psc_connection: true

References

1. Ensure the use block is preferred for module dependencies within blueprints. Explicit variable linking should only be used when necessary to resolve ambiguity. ^(link)

[gemini-code-assist]

According to the Cluster Toolkit style guide, the use block is preferred for module dependencies within blueprints to allow automatic propagation of settings (like subnetwork_self_link) and prevent deployment race conditions. Explicit variable linking should only be used when necessary to resolve ambiguity.

  - id: slurm_controller
    source: community/modules/scheduler/schedmd-slurm-gcp-v6-controller
    use: [debug_partition, network]
    settings:
      enable_backup_controller: true
      controller_ha_type: regional
      slurm_cluster_name: $(vars.deployment_name)
      backup_zone: us-central1-f
      # Disable PD state disk, rely on /var/spool/slurm filestore share
      controller_state_disk: null
      cloudsql:
        server_ip: $(slurm_db.cloudsql.server_ip)
        user: $(slurm_db.cloudsql.user)
        password: $(slurm_db.cloudsql.password)
        db_name: $(slurm_db.cloudsql.db_name)
      enable_controller_public_ips: false # NO EXTERNAL IP
      machine_type: n1-standard-2
      zone: $(vars.zone)
      service_account_email: $(controller_sa.service_account_email)
      service_account_scopes: ["https://www.googleapis.com/auth/cloud-platform"]
      enable_oslogin: true
      enable_shielded_vm: true
      tags: ["$(vars.deployment_name)-controller"]
      named_ports:
      - name: slurm-restapi
        port: 6842

References

1. Ensure the use block is preferred for module dependencies within blueprints. Explicit variable linking should only be used when necessary to resolve ambiguity. ^(link)

[gemini-code-assist]

To prevent potential index-out-of-bounds errors when resources have a conditional count of 0 or 1, use the built-in one() function instead of direct [0] indexing. This is safer and more idiomatic in Terraform.

  target                = local.is_https ? one(google_compute_region_target_https_proxy.default[*].id) : one(google_compute_region_target_http_proxy.default[*].id)

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a machine-type fallback feature (instance_flexibility_policy) for MIG NodeSets in Slurm, adds a new community module for an Internal Application Load Balancer (int-app-lb) with an accompanying HA Slurm example, and implements Multi-NIC client configuration for Managed Lustre. Additionally, GKE job cancellation now reclaims unused storage gateways, and pre-flight checks have been added for GCSFuse storage profiles. Feedback on these changes includes indexing the new example and module in their respective READMEs, fixing potential Terraform evaluation errors in the nodeset and load balancer modules by using try() and one(concat(...)), and correcting invalid variable syntax (${vars.var}) to the standard $(vars.var) format in the machine learning blueprints.

[gemini-code-assist]

The new example blueprint hpc-slurm-ha-lb.yaml has been added but is not indexed in examples/README.md. Please add it to the index to comply with the repository style guide.

References

1. If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

The new module int-app-lb has been added but is not indexed in modules/README.md. Please add it to the index to comply with the repository style guide.

References

1. If new modules (core or community) are added, ensure they are added to the index in modules/README.md. ^(link)

[gemini-code-assist]

Terraform does not short-circuit logical operators like && to prevent evaluation of the right-hand side. If mt (e.g., var.machine_type) is not present in local.flex_machine_types, referencing module.fallback_gpu[mt] will throw a "Missing map element" error during planning. Use try() to safely handle potentially missing keys.

      try(length(module.fallback_gpu[mt].guest_accelerator) > 0, false) ? "${replace(module.fallback_gpu[mt].guest_accelerator[0].type, "/^.*\\//", "")}:${module.fallback_gpu[mt].guest_accelerator[0].count}" :

[gemini-code-assist]

Terraform evaluates both branches of a ternary operator during expression evaluation. If local.is_https is true, google_compute_region_target_http_proxy.default has a count of 0, and referencing index [0] will fail with an "index out of range" error. Conversely, if it is false, referencing google_compute_region_target_https_proxy.default[0] will fail. Use one(concat(...)) to safely reference conditional resources.

  target                = one(concat(google_compute_region_target_https_proxy.default[*].id, google_compute_region_target_http_proxy.default[*].id))

[gemini-code-assist]

The syntax ${vars.region} and ${vars.base_network_name} is invalid. In Cluster Toolkit blueprints, variables must be referenced using the $(vars.variable_name) syntax. Using ${vars.region} will cause a parser or deployment failure.

            subnetwork=a3ultra-slurm-net-0.subnetworks["$(vars.region)/$(vars.base_network_name)-sub-1"].self_link,

[gemini-code-assist]

The syntax ${vars.region} and ${vars.base_network_name} is invalid. In Cluster Toolkit blueprints, variables must be referenced using the $(vars.variable_name) syntax. Using ${vars.region} will cause a parser or deployment failure.

            subnetwork=a4high-slurm-net-0.subnetworks["$(vars.region)/$(vars.base_network_name)-sub-1"].self_link,

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new internal application load balancer module (int-app-lb) and an associated high-availability Slurm example blueprint, alongside a new machine-type fallback policy (instance flexibility) for MIG nodesets. The code review identified several critical issues that need to be addressed: a command substitution syntax issue in hcls-blueprint.yaml that can cause parsing errors, bypassed disk compatibility checks and potential plan-time crashes in the nodeset output preconditions, an invalid metadata projection in the compute cleanup script, and fragile string splitting on network_id in the Managed Lustre module. Additionally, the new example and module must be indexed in examples/README.md and modules/README.md respectively, to comply with the repository style guide.

[gemini-code-assist]

When embedding shell scripts in YAML multiline strings, the $(...) syntax for command substitution can cause parsing errors in the Cluster Toolkit. Consider using the legacy backtick syntax (`...`) as a workaround to avoid these errors.

        vmd_tarball=`find /user_provided_software -name "vmd-1.9.*.bin.LINUXAMD64*.tar.gz" -print -quit 2>/dev/null`
        if [ -z "${vmd_tarball}" ]; then
          echo "VMD tarball not found in /user_provided_software; skipping VMD installation."
          exit 0
        fi
        set -e
        echo "Installing VMD from ${vmd_tarball}"
        build_dir=`mktemp -d`

References

1. When embedding shell scripts in YAML multiline strings, the $(...) syntax for command substitution can cause parsing errors. Consider using the legacy backtick syntax as a workaround if such errors occur.

[gemini-code-assist]

The precondition uses !local.has_flex_policy || alltrue(...), which means the new disk compatibility checks (for c3, h3, c4, etc.) are completely bypassed for standard nodesets that do not use an instance_flexibility_policy.

Since local.flex_machine_types defaults to an empty set when no flexibility policy is defined, setunion([var.machine_type], local.flex_machine_types) will safely evaluate to just [var.machine_type]. Removing the !local.has_flex_policy || guard ensures these important compatibility checks are always enforced for the primary machine_type as well.

    condition = alltrue([
      for mt in setunion([var.machine_type], local.flex_machine_types) : !(
        (can(regex("^c2-", mt)) && startswith(var.disk_type, "hyperdisk-")) ||
        (can(regex("^(c3|c3d)-", mt)) && var.disk_type == "pd-standard") ||
        (can(regex("^h3-", mt)) && contains(["pd-standard", "pd-ssd"], var.disk_type)) ||
        (can(regex("^(c4|c4a|c4d|h4d|n4|n4a|n4d)-", mt)) && startswith(var.disk_type, "pd-"))
      )
    ])
    error_message = "A disk_type=${var.disk_type} cannot be used with machine_type=${var.machine_type} or one of its instance_flexibility_policy fallback machine types."
  }

References

1. In Terraform, use the contains function for validating if a variable's value is within a list of allowed values to improve readability and maintainability.

[gemini-code-assist]

If var.instance_flexibility_policy is null, local.has_flex_policy is false. However, Terraform evaluates all parts of the precondition expression, and accessing local.instance_flexibility_policy.instance_selections and var.instance_flexibility_policy.instance_selections directly will result in a "Can't access attributes on a primitive-typed value" or null-pointer evaluation error.

To prevent this, use try() to safely fallback to an empty list when the policy is null.

    condition = !local.has_flex_policy || (
      length(setunion([var.machine_type], local.flex_machine_types)) <= 10 &&
      length(distinct([for s in try(local.instance_flexibility_policy.instance_selections, []) : s.name])) == length(try(local.instance_flexibility_policy.instance_selections, [])) &&
      (contains(local.flex_machine_types, var.machine_type) || alltrue([for s in try(var.instance_flexibility_policy.instance_selections, []) : s.rank > 0]))
    )
    error_message = "instance_flexibility_policy may reference at most 10 distinct machine types in total (including machine_type=${var.machine_type}), all selection names must be unique, and fallback selections must use rank >= 1 when machine_type=${var.machine_type} is not explicitly included in instance_selections."
  }

References

1. To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.

[gemini-code-assist]

In gcloud, metadata.items is a list of key-value maps, so metadata.items.created-by is not a valid projection and will return an empty value. This causes the awk filter to always evaluate to true, meaning all instances (including those managed by Terraform) will be deleted, which defeats the purpose of this change.

To correctly extract the created-by metadata value, use the .filter() projection method:
metadata.items.filter(key:created-by).value.basename()

Suggested change

	gcloud compute instances list --format="value(selfLink,metadata.items.created-by.basename())" --filter="${running_nodes_filter}" |
	awk -F'\t' -v re="${tf_mig_regex}" '$2 !~ re {print $1}' >"$tmpfile"
	gcloud compute instances list --format="value(selfLink,metadata.items.filter(key:created-by).value.basename())" --filter="${running_nodes_filter}" |
	awk -F'\t' -v re="${tf_mig_regex}" '$2 !~ re {print $1}' >"$tmpfile"

[gemini-code-assist]

The new example blueprint hpc-slurm-ha-lb.yaml is not indexed in examples/README.md. Please add it to the index as required by the repository style guide.

References

1. If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

The new community module int-app-lb is not indexed in modules/README.md. Please add it to the index as required by the repository style guide. Note that updates to the module index can be deferred to a separate, dedicated pull request if you prefer to maintain the focus of the current PR.

References

1. All new modules must be registered in the main index in modules/README.md to ensure they are discoverable, as per the repository style guide. ^(link)
2. Updates to the module index in modules/README.md for new modules can be deferred to a separate, dedicated pull request, particularly when multiple modules are being added or to maintain the focus of the current PR.

[gemini-code-assist]

Using split("/", var.network_id)[4] is fragile and will cause a Terraform evaluation error if var.network_id is provided as a short name (e.g., "default") or a self-link with a different number of path segments.

Using the basename() function is much more robust as it safely extracts the last segment (the network name) regardless of whether a full resource ID, self-link, or short name is provided.

  network = basename(var.network_id)

[siddhartha-quad]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new internal application load balancer module and an associated high-availability Slurm example blueprint, alongside support for machine-type fallback policies (instance flexibility) in MIG NodeSets. It also enhances GKE workload orchestration with Cloud Storage FUSE storage profiles, pre-flight validation checks, and improved signal handling for graceful termination. The review feedback highlights several critical issues in the Terraform code, including potential evaluation errors when referencing count-zero resources in ternary operators and when calling trimspace on null values. Additionally, the new example blueprint should use explicit subnetwork map references instead of the ambiguous subnetwork_self_link output, and it must be added to the index in examples/README.md per the style guide. Finally, a defensive check should be added in the Go configuration parser to prevent potential panics on null or unknown collections.

[gemini-code-assist]

In Terraform, referencing index 0 of a resource with count = 0 (such as google_compute_region_target_https_proxy.default or google_compute_region_target_http_proxy.default) will cause an evaluation error during plan/apply, even if it is inside a non-selected branch of a ternary operator. Use the one() function to safely handle empty lists.

  target                = local.is_https ? one(google_compute_region_target_https_proxy.default[*].id) : one(google_compute_region_target_http_proxy.default[*].id)

[gemini-code-assist]

trimspace(s.name) will fail with an evaluation error if s.name is null (which is allowed since name is optional(string)). Use try(trimspace(s.name), "") to safely handle null values.

  validation {
    condition = var.instance_flexibility_policy == null || alltrue([
      for s in var.instance_flexibility_policy.instance_selections :
      s.name == null || try(trimspace(s.name), "") == "" || can(regex("^[a-z]([-a-z0-9]{0,61}[a-z0-9])?$", try(trimspace(s.name), "")))
    ])

[gemini-code-assist]

trimspace(s.name) will fail with an evaluation error if s.name is null. Use try(trimspace(s.name), "") to safely handle null values.

  validation {
    condition = var.instance_flexibility_policy == null || length(distinct([
      for idx, s in var.instance_flexibility_policy.instance_selections :
      try(trimspace(s.name), "") != "" ? trimspace(s.name) : "selection-${idx + 1}"
    ])) == length(var.instance_flexibility_policy.instance_selections)

[gemini-code-assist]

trimspace(instance_selections.value.name) will fail with an evaluation error if name is null. Use try(trimspace(instance_selections.value.name), "") to safely handle null values.

          name = try(trimspace(instance_selections.value.name), "") != "" ? trimspace(instance_selections.value.name) : "selection-${instance_selections.key + 1}"

[gemini-code-assist]

Since the network module defines multiple subnetworks, the subnetwork_self_link output is ambiguous or invalid. Use the explicit subnetwork map output instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Since the network module defines multiple subnetworks, the subnetwork_self_link output is ambiguous or invalid. Use the explicit subnetwork map output instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Since the network module defines multiple subnetworks, the subnetwork_self_link output is ambiguous or invalid. Use the explicit subnetwork map output instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Since the network module defines multiple subnetworks, the subnetwork_self_link output is ambiguous or invalid. Use the explicit subnetwork map output instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Style guide violation (Repository Style Guide line 33): New examples must be added to the index in examples/README.md. Please add hpc-slurm-ha-lb.yaml to the index.

Suggested change

	* [slurm-gke.yaml](#slurm-gkeyaml--) ![community-badge] ![experimental-badge]
	* [sycomp](#sycomp--) ![community-badge] ![experimental-badge]
	* [slurm-gke.yaml](#slurm-gkeyaml--) ![community-badge] ![experimental-badge]
	* [hpc-slurm-ha-lb.yaml](#hpc-slurm-ha-lbyaml--) ![community-badge]
	* [sycomp](#sycomp--) ![community-badge] ![experimental-badge]

References

1. CRITICAL: If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

Enforce defensive programming: coll.ElementIterator() will panic if coll is null or unknown. Check coll.IsNull() || !coll.IsKnown() before processing.

Suggested change

	func extractStringElements(coll cty.Value) []string {
	if !coll.Type().IsTupleType() && !coll.Type().IsListType() && !coll.Type().IsSetType() {
	return nil
	}
	func extractStringElements(coll cty.Value) []string {
	if coll.IsNull() || !coll.IsKnown() || (!coll.Type().IsTupleType() && !coll.Type().IsListType() && !coll.Type().IsSetType()) {
	return nil
	}

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new community network module int-app-lb and an accompanying Slurm HA load balancer example blueprint, alongside a new instance_flexibility_policy for machine-type fallback in MIG nodesets. It also adds pre-flight checks for GCSFuse storage profiles in GKE and wraps workload container commands for graceful SIGTERM handling. Feedback on these changes highlights several critical issues: the new example blueprint incorrectly references a non-existent subnetwork_self_link output from the multi-subnet network module, and ML blueprints use invalid ${vars.name} syntax instead of $(vars.name). Additionally, potential Terraform evaluation errors were identified in the nodeset and controller modules where trimspace is called on potentially null name values, and an invalid metadata projection was found in the compute cleanup script. Finally, the new example and module must be added to their respective README indexes to comply with the repository style guide.

[gemini-code-assist]

Use the explicit subnetwork reference from the subnetworks map instead of the non-existent subnetwork_self_link output.

      subnetwork_self_link: $(network.subnetworks["$(vars.region)/$(vars.deployment_name)-primary-subnet"].self_link)

[gemini-code-assist]

Avoid calling trimspace on instance_selections.value.name to prevent Terraform evaluation errors when the name is null.

          name = (
            instance_selections.value.name != null && instance_selections.value.name != ""
          ) ? instance_selections.value.name : "selection-${instance_selections.key + 1}"

[gemini-code-assist]

Blueprint variables must be referenced using the $(vars.name) syntax in Cluster Toolkit blueprints. Using ${vars.name} is invalid and will not be expanded by the blueprint compiler, leading to deployment failures.

            subnetwork=a4high-slurm-net-0.subnetworks["$(vars.region)/$(vars.base_network_name)-sub-1"].self_link,

[gemini-code-assist]

Since the network module defines multiple subnetworks, it does not output a single subnetwork_self_link. To reference the primary subnetwork, use the subnetworks map output with the region and subnet name:
$(network.subnetworks["$(vars.region)/$(vars.deployment_name)-primary-subnet"].self_link)

      subnetwork_self_link: $(network.subnetworks["$(vars.region)/$(vars.deployment_name)-primary-subnet"].self_link)

[gemini-code-assist]

Use the explicit subnetwork reference from the subnetworks map instead of the non-existent subnetwork_self_link output.

      subnetwork_self_link: $(network.subnetworks["$(vars.region)/$(vars.deployment_name)-primary-subnet"].self_link)

[gemini-code-assist]

The int-app-lb module requires subnetwork_self_link as a required input, but it is not automatically bound by use: [network] because the network module defines multiple subnetworks and does not output a single subnetwork_self_link. You must explicitly specify subnetwork_self_link in the settings.

    settings:
      project_id: $(vars.project_id)
      region: $(vars.region)
      subnetwork_self_link: $(network.subnetworks["$(vars.region)/$(vars.deployment_name)-primary-subnet"].self_link)
      backends:

[gemini-code-assist]

Calling trimspace(s.name) when s.name is null (which is the default/optional value) will cause a Terraform evaluation error because trimspace does not accept null arguments. Since any name with leading/trailing spaces is already invalid under RFC 1035, you can safely remove trimspace and validate s.name directly.

    condition = var.instance_flexibility_policy == null || alltrue([
      for s in var.instance_flexibility_policy.instance_selections :
      s.name == null || s.name == "" || can(regex("^[a-z]([-a-z0-9]{0,61}[a-z0-9])?$", s.name))
    ])

[gemini-code-assist]

Similarly, avoid calling trimspace(s.name) here to prevent potential Terraform evaluation errors when s.name is null. You can check and use s.name directly.

    condition = var.instance_flexibility_policy == null || length(distinct([
      for idx, s in var.instance_flexibility_policy.instance_selections :
      (s.name != null && s.name != "") ? s.name : "selection-${idx + 1}"
    ])) == length(var.instance_flexibility_policy.instance_selections)

[gemini-code-assist]

According to the Repository Style Guide (line 33), new examples must be added to the index in examples/README.md. Please add the new hpc-slurm-ha-lb.yaml example to this index.

References

1. If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

According to the Repository Style Guide (line 32), new modules must be added to the index in modules/README.md. Please add the new int-app-lb community module to this index under the Network section.

References

1. If new modules (core or community) are added, ensure they are added to the index in modules/README.md. ^(link)

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces machine-type fallback support (instance_flexibility_policy) for MIG NodeSets, adds an Internal Application Load Balancer module for the Slurm REST API, and improves GKE workload signal handling and storage profile pre-flight checks. The code review identified several critical issues: a bug in cleanup_compute.sh that prevents correct identification of creator MIGs; potential Terraform evaluation crashes in the nodeset variables and outputs when the flexibility policy is null; an index out of bounds error in the load balancer proxy target selection; and an undefined variable referenced in the nodeset metadata validators. Additionally, the new example blueprint needs to be indexed in examples/README.md and should use explicit subnetwork map references instead of ambiguous self-links.

[gemini-code-assist]

The gcloud compute instances list command uses --format="value(selfLink,metadata.items.created-by.basename())". However, metadata.items is a list of key-value maps, and you cannot query a specific key using metadata.items.created-by. This will return an empty value, causing the subsequent awk command to match all instances (including those owned by static MIGs) and attempt to delete them, which will trigger race conditions and fail terraform destroy. Use metadata.items.filter(key='created-by').value.basename() to correctly extract the creator MIG name.

Suggested change

	gcloud compute instances list --format="value(selfLink,metadata.items.created-by.basename())" --filter="${running_nodes_filter}" |
	awk -F'\t' -v re="${tf_mig_regex}" '$2 !~ re {print $1}' >"$tmpfile"
	gcloud compute instances list --format="value(selfLink,metadata.items.filter(key='created-by').value.basename())" --filter="${running_nodes_filter}" |
	awk -F'\t' -v re="${tf_mig_regex}" '$2 !~ re {print $1}' >"$tmpfile"

[gemini-code-assist]

The newly added example blueprint hpc-slurm-ha-lb.yaml is not indexed in examples/README.md. According to the Repository Style Guide, all new examples must be added to the index in examples/README.md to maintain documentation completeness.

References

1. If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

Using $(network.subnetwork_self_link) is ambiguous and can lead to deployment failures or incorrect subnetwork associations when the VPC module defines multiple subnetworks (such as primary-subnet and proxy-subnet here). Instead, use the explicit subnetwork from the exported subnetworks map to ensure the correct subnetwork is targeted. This also applies to lines 145, 182, and 211.

      subnetwork_self_link: $(network.subnetworks[vars.region + "/" + vars.deployment_name + "-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

The conditional_regex validator triggers on enable_multinic: true, but enable_multinic is not a variable defined in the schedmd-slurm-gcp-v6-nodeset module (it is not present in variables.tf or README.md). This will cause a validation error or crash when parsing the module metadata. Please verify if this validator belongs to a different module or if the variable needs to be added to this module.

[gemini-code-assist]

In this precondition, local.instance_flexibility_policy.instance_selections is accessed directly. However, if local.has_flex_policy is false, local.instance_flexibility_policy is null, and accessing .instance_selections on it will throw an 'Unsupported attribute' error during Terraform evaluation, blocking plan/apply even for users who do not use the flexibility policy. Wrap the access in try() to handle the null case safely.

  precondition {
    condition = !local.has_flex_policy || (
      length(setunion([var.machine_type], local.flex_machine_types)) <= 10 &&
      length(distinct([for s in try(local.instance_flexibility_policy.instance_selections, []) : s.name])) == length(try(local.instance_flexibility_policy.instance_selections, [])) &&
      (contains(local.flex_machine_types, var.machine_type) || alltrue([for s in try(local.instance_flexibility_policy.instance_selections, []) : s.rank > 0]))
    )
    error_message = "instance_flexibility_policy may reference at most 10 distinct machine types in total (including machine_type=${var.machine_type}), all selection names must be unique, and fallback selections must use rank >= 1 when machine_type=${var.machine_type} is not explicitly included in instance_selections."
  }

References

1. To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.

[gemini-code-assist]

This validation block accesses var.instance_flexibility_policy.instance_selections directly. If var.instance_flexibility_policy is null (the default), this will throw an 'Unsupported attribute' error and block Terraform execution. Wrap the access in try() to handle the null case safely. This also applies to the other validation blocks below (lines 755-793).

  validation {
    condition = var.instance_flexibility_policy == null || alltrue([
      for s in try(var.instance_flexibility_policy.instance_selections, []) : s.rank >= 0
    ])
    error_message = "Each instance_flexibility_policy.instance_selections[*].rank must be >= 0 (lower rank is tried first)."
  }

References

1. To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.

[gemini-code-assist]

Accessing index [0] directly on google_compute_region_target_https_proxy.default or google_compute_region_target_http_proxy.default will throw an 'index out of bounds' error during Terraform evaluation because one of these resources will have count = 0 depending on local.is_https. Use the one() function with splat syntax to safely extract the ID of the active proxy.

  target                = coalesce(one(google_compute_region_target_https_proxy.default[*].id), one(google_compute_region_target_http_proxy.default[*].id))

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several key features and updates, including a new --parallelism flag for tuning Terraform concurrency during deployment and destruction, a new internal application load balancer module (int-app-lb), and new security modules for managing Customer-Managed Encryption Keys (CMEK) along with corresponding example blueprints. Additionally, the a3-highgpu-8g Slurm blueprint is updated to use the TCPXO (FasTrak) networking stack by default, and GKE job orchestration is improved with better signal handling and storage gateway cleanup. The code review identified several critical issues: a potential plan-time crash in the nodeset module's outputs due to unsafe null attribute access, an index out-of-bounds error in the load balancer's proxy target evaluation, incorrect backslash escaping of shell variables in a YAML literal block within hcls-blueprint.yaml, and an ambiguous subnetwork reference in the high-availability Slurm blueprint.

[gemini-code-assist]

Accessing local.instance_flexibility_policy.instance_selections directly when local.instance_flexibility_policy is null (which is the default) will cause a Terraform evaluation crash during planning. Wrap the attribute access in try() to safely handle the null case.

    condition = !local.has_flex_policy || (
      length(setunion([var.machine_type], local.flex_machine_types)) <= 10 &&
      length(distinct([for s in try(local.instance_flexibility_policy.instance_selections, []) : s.name])) == length(try(local.instance_flexibility_policy.instance_selections, [])) &&
      (contains(local.flex_machine_types, var.machine_type) || alltrue([for s in try(local.instance_flexibility_policy.instance_selections, []) : s.rank > 0]))
    )

References

1. To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.

[gemini-code-assist]

Terraform evaluates both branches of a ternary operator. If local.is_https is false, google_compute_region_target_https_proxy.default[0] will fail with an index out of bounds error because its count is 0. Use the one() function with splat syntax to safely resolve the target ID.

  target                = local.is_https ? one(google_compute_region_target_https_proxy.default[*].id) : one(google_compute_region_target_http_proxy.default[*].id)

[gemini-code-assist]

In a YAML literal block (|), backslashes are treated as literal characters. Escaping the $ sign as \$ will result in literal \$ in the generated script, which escapes the $ in Bash and prevents the command substitutions $(find ...) and $(mktemp -d) from executing. Remove the backslashes to allow the command substitutions to run correctly.

        vmd_tarball=$(find /user_provided_software -name "vmd-1.9.*.bin.LINUXAMD64*.tar.gz" -print -quit 2>/dev/null)
        if [ -z "${vmd_tarball}" ]; then
          echo "VMD tarball not found in /user_provided_software; skipping VMD installation."
          exit 0
        fi
        set -e
        echo "Installing VMD from ${vmd_tarball}"
        build_dir=$(mktemp -d)
        tar xzf "${vmd_tarball}" -C "${build_dir}"

[gemini-code-assist]

Since the network module defines multiple subnetworks, referencing $(network.subnetwork_self_link) will resolve to null or cause an error. Use a specific subnetwork reference from the subnetworks map instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several significant updates to the Cluster Toolkit, including a new --parallelism flag for Terraform concurrency control, support for machine-type fallback in MIG NodeSets via instance_flexibility_policy, a new Internal Application Load Balancer (int-app-lb) module, end-to-end CMEK support for Slurm clusters, a new cloud-build module, and default TCPXO networking for the a3-highgpu-8g platform. Review feedback highlights three key areas for improvement: first, in the int-app-lb module, using the one() function with the splat operator [*] is recommended to safely handle conditional proxy target evaluation and avoid plan-time out-of-bounds errors; second, in the Slurm nodeset module, wrapping the flexibility policy selections in try() is necessary to prevent plan-time crashes when the policy is null; and third, in the high-availability Slurm blueprint, the use block should be preferred for module dependencies instead of explicit variable linking to align with the repository style guide.

[gemini-code-assist]

In Terraform, the ternary operator cond ? a : b evaluates both branches during static analysis and planning. If local.is_https is false, google_compute_region_target_https_proxy.default will have a count of 0, and attempting to access index [0] will cause a plan-time evaluation error. Using the one() function with the splat operator [*] safely handles this conditional evaluation by returning null instead of throwing an out-of-bounds error.

  target                = local.is_https ? one(google_compute_region_target_https_proxy.default[*].id) : one(google_compute_region_target_http_proxy.default[*].id)

[gemini-code-assist]

If local.has_flex_policy is false, local.instance_flexibility_policy evaluates to null. During Terraform's plan-time evaluation, accessing local.instance_flexibility_policy.instance_selections inside the list comprehension can trigger an evaluation error because you cannot access attributes on a null value. Wrapping the access in try() prevents this potential crash when the flexibility policy is not configured.

    condition = !local.has_flex_policy || (
      length(setunion([var.machine_type], local.flex_machine_types)) <= 10 &&
      length(distinct([for s in try(local.instance_flexibility_policy.instance_selections, []) : s.name])) == length(try(local.instance_flexibility_policy.instance_selections, [])) &&
      (contains(local.flex_machine_types, var.machine_type) || alltrue([for s in try(var.instance_flexibility_policy.instance_selections, []) : s.rank > 0]))
    )

References

1. To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.

[gemini-code-assist]

The blueprint explicitly links network_id and subnetwork_self_link across multiple modules (such as home_fs, login_nodes, compute_nodes, controller_fs, slurm_db, and slurm_controller) instead of using the use block. According to the repository style guide, the use block is preferred for module dependencies to allow automatic resolution of outputs and prevent explicit variable linking unless necessary to resolve ambiguity.

  - id: home_fs
    source: modules/file-system/filestore
    use: [network]
    settings:

References

1. Ensure the use block is preferred for module dependencies within blueprints. Explicit variable linking (e.g., setting = $(module.output)) should only be used when necessary to resolve ambiguity. ^(link)
2. In the Cluster Toolkit framework, leverage automatic variable wiring by matching output names from a dependency module (defined in 'use') with top-level variable names in the consuming module to avoid redundant manual wiring. ^(link)

[aslam-quad]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several major features and updates, including a new --parallelism flag for controlling Terraform concurrency during deployment and destruction, support for Slurm v6 hybrid controller configurations, LNet Multi-Rail (Multi-NIC) client setup in the managed-lustre module, and a new cloud-build module for remote container builds. It also transitions the gke-a3-highgpu example from TCPX to the next-generation TCPXO (FasTrak) networking stack and adds automated storage gateway cleanup to gcluster job cancel. Feedback on these changes highlights an ambiguity in subnetwork references within the high-availability Slurm load balancer example, recommending specific subnetwork paths, and suggests extracting a complex inline VMD installation script from the HCLS blueprint into a separate file to improve maintainability.

[gemini-code-assist]

Since the network module defines multiple subnetworks (primary and proxy), referencing $(network.subnetwork_self_link) is ambiguous and will fail or bind the wrong subnetwork. Use a dynamic reference to the specific primary subnetwork instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Since the network module defines multiple subnetworks (primary and proxy), referencing $(network.subnetwork_self_link) is ambiguous and will fail or bind the wrong subnetwork. Use a dynamic reference to the specific primary subnetwork instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Since the network module defines multiple subnetworks (primary and proxy), referencing $(network.subnetwork_self_link) is ambiguous and will fail or bind the wrong subnetwork. Use a dynamic reference to the specific primary subnetwork instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

Since the network module defines multiple subnetworks (primary and proxy), referencing $(network.subnetwork_self_link) is ambiguous and will fail or bind the wrong subnetwork. Use a dynamic reference to the specific primary subnetwork instead.

      subnetwork_self_link: $(network.subnetworks["${vars.region}/${vars.deployment_name}-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[gemini-code-assist]

This inline shell script contains complex logic for compiling and installing VMD from source. To improve readability and maintainability, extract this complex inline shell script from the YAML configuration into a separate script file.

References

1. Extract complex inline shell scripts from YAML into separate script files to improve readability and maintainability.

[deepakrajk-quad]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several major features to the Cluster Toolkit, including a --parallelism flag to manage Terraform concurrency, new community modules for Cloud KMS (CMEK) and Cloud Build, and an Internal Application Load Balancer. It also enhances Slurm modules to support hybrid clusters and instance flexibility policies for MIG NodeSets, alongside dependency updates and new example blueprints. The review feedback correctly identifies that the new hpc-slurm-ha-lb.yaml example blueprint is missing from the index in examples/README.md and provides a code suggestion to resolve this style guide violation.

[gemini-code-assist]

The new example blueprint hpc-slurm-ha-lb.yaml is missing from the examples index in examples/README.md. According to the repository style guide, all new examples must be added to the index.

Suggested change

	* [hpc-slurm6-apptainer.yaml](#hpc-slurm6-apptaineryaml--) ![community-badge] ![experimental-badge]
	* [hpc-slurm6-hybrid.yaml](#hpc-slurm6-hybridyaml--) ![community-badge] ![experimental-badge]
	* [hpc-slurm6-apptainer.yaml](#hpc-slurm6-apptaineryaml--) ![community-badge] ![experimental-badge]
	* [hpc-slurm6-hybrid.yaml](#hpc-slurm6-hybridyaml--) ![community-badge] ![experimental-badge]
	* [hpc-slurm-ha-lb.yaml](#hpc-slurm-ha-lbyaml--) ![community-badge] ![experimental-badge]

References

1. If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several major features, including support for hybrid Slurm v6 clusters, instance flexibility policies (machine-type fallback) for MIG NodeSets, a new Google Cloud Build execution module, an internal application load balancer for Slurm REST API high-availability, and Customer-Managed Encryption Keys (CMEK) support via new KMS modules. It also updates GKE A3 HighGPU blueprints to use GPUDirect-TCPXO. Feedback on these changes highlights critical Terraform evaluation errors in the Slurm controller module where accessing module.slurm_controller_template[0] directly will fail when enable_hybrid is true; using the one() function with splat expressions is recommended. Additionally, several modules incorrectly constrain the Google provider version to non-existent >= 7.x versions, which must be corrected to >= 5.33.0 or >= 6.0.0.

[gemini-code-assist]

When enable_hybrid is true, module.slurm_controller_template has a count of 0, meaning it is an empty list. Accessing index 0 directly via module.slurm_controller_template[0] will cause a Terraform evaluation error ("Invalid index") even though it is inside a ternary operator. Use the one() function with a splat expression to safely handle the conditional list.

    var.enable_hybrid ? local.hybrid_sa : "serviceAccount:${one(module.slurm_controller_template[*].service_account.email)}",

[gemini-code-assist]

When enable_hybrid is true, module.slurm_controller_template has a count of 0. Accessing index 0 directly via module.slurm_controller_template[0] will cause a Terraform evaluation error ("Invalid index") during plan. Use the one() function with a splat expression to safely handle the conditional list.

  source_instance_template = one(module.slurm_controller_template[*].self_link)
  # Due to https://github.com/hashicorp/terraform-provider-google/issues/21693
  # we have to explicitly override instance labels instead of inheriting them from template.
  labels = one(module.slurm_controller_template[*].labels)

[gemini-code-assist]

When enable_hybrid is true, module.slurm_controller_template has a count of 0. Accessing index 0 directly via module.slurm_controller_template[0] will cause a Terraform evaluation error ("Invalid index") during plan. Use the one() function with a splat expression to safely handle the conditional list.

    instance_template = one(module.slurm_controller_template[*].self_link)

[gemini-code-assist]

When enable_hybrid is true, module.slurm_controller_template has a count of 0. Accessing index 0 directly via module.slurm_controller_template[0] will cause a Terraform evaluation error ("Invalid index") during plan. Use the one() function with a splat expression to safely handle the conditional list.

    instance_template = one(module.slurm_controller_template[*].self_link)

[gemini-code-assist]

When enable_hybrid is true, module.slurm_controller_template has a count of 0. Accessing index 0 directly via module.slurm_controller_template[0] will cause a Terraform evaluation error ("Invalid index") during plan even if the ternary condition evaluates to the true branch. Use the one() function with a splat expression to safely handle the conditional list.

  replace_trigger = var.enable_hybrid ? null : (var.enable_backup_controller ? one(module.slurm_controller_template[*].self_link) : one(google_compute_instance_from_template.controller[*].self_link))

[gemini-code-assist]

The Google provider has not reached version 7.x yet. Specifying >= 7.33.0 will cause terraform init to fail because no matching provider version can be found. Change this to >= 5.33.0 (which is when the deletion_policy feature was introduced) or >= 6.0.0 depending on your requirements.

      version = ">= 5.33.0"

References

1. Use open-ended provider version constraints (e.g., '>= 6.0') in Terraform modules rather than restricting them with an upper bound (e.g., '< 7.0') to ensure compatibility and prevent conflicts for module users.

[gemini-code-assist]

The Google provider has not reached version 7.x yet. Specifying >= 7.33.0 will cause terraform init to fail because no matching provider version can be found. Change this to >= 5.33.0 (which is when the deletion_policy feature was introduced) or >= 6.0.0 depending on your requirements.

      version = ">= 5.33.0"

References

1. Use open-ended provider version constraints (e.g., '>= 6.0') in Terraform modules rather than restricting them with an upper bound (e.g., '< 7.0') to ensure compatibility and prevent conflicts for module users.

[gemini-code-assist]

The Google provider has not reached version 7.x yet. Specifying >= 7.33.0 will cause terraform init to fail because no matching provider version can be found. Change this to >= 5.33.0 (which is when the deletion_policy feature was introduced) or >= 6.0.0 depending on your requirements.

      version = ">= 5.33.0"

References

1. Use open-ended provider version constraints (e.g., '>= 6.0') in Terraform modules rather than restricting them with an upper bound (e.g., '< 7.0') to ensure compatibility and prevent conflicts for module users.

[gemini-code-assist]

The Google provider has not reached version 7.x yet. Specifying >= 7.10.0 will cause terraform init to fail because no matching provider version can be found. Change this to >= 5.10.0 or >= 6.0.0 depending on your requirements.

      version = ">= 5.10.0"

References

1. Use open-ended provider version constraints (e.g., '>= 6.0') in Terraform modules rather than restricting them with an upper bound (e.g., '< 7.0') to ensure compatibility and prevent conflicts for module users.

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several significant features, including support for Terraform parallelism limits across deploy, destroy, and export commands, new security modules for Customer-Managed Encryption Keys (CMEK), a new Cloud Build module, and an Internal Application Load Balancer module. It also adds examples for hybrid Slurm clusters and high-availability Slurm setups. The review feedback highlights a few critical issues: the new example and load balancer module must be indexed in their respective README files; the Cloud Build module's local directory upload can cause remote Git clones to fail, and its source hashing should exclude Terraform state and configuration files to ensure idempotency; and the KMS IAM module contains a type mismatch in a conditional expression that will cause a plan-time error.

[gemini-code-assist]

The new example 'community/examples/hpc-slurm-ha-lb.yaml' has been added to the repository but is missing from the index in 'examples/README.md'. Per the Repository Style Guide (Section 4), all new examples must be added to the index in 'examples/README.md'. Please add it to maintain consistency and discoverability.

References

1. CRITICAL: If new examples (core or community) are added, ensure they are added to the index in examples/README.md. ^(link)

[gemini-code-assist]

The new module 'community/modules/network/int-app-lb' has been added to the repository but is missing from the index in 'modules/README.md'. Per the Repository Style Guide (Section 4), all new modules must be added to the index in 'modules/README.md'. Please add it to maintain consistency and discoverability.

References

1. CRITICAL: If new modules (core or community) are added, ensure they are added to the index in modules/README.md. ^(link)

[gemini-code-assist]

When 'is_target_dir_local' is true and 'REPO_URL' is set, 'gcloud builds submit' is run with 'STAGE_DIR' as the source, which uploads the local target directory overlay to '/workspace' in Cloud Build. However, any standard 'git clone' step in the Cloud Build configuration that clones into '.' (such as the one in 'build-alphafold3.yaml.tfpl') will fail because '/workspace' is not empty, throwing 'fatal: destination path '.' already exists and is not an empty directory.' Consider restructuring the clone step to clone into a temporary subdirectory first, or copying the overlay after cloning.

[gemini-code-assist]

'local_source_hash' calculates the SHA256 of all files in 'local.source_dir'. Since 'cloud_build_dir' defaults to '.' (the root of the terraform deployment directory) and 'local.is_local_dir_mode' is true by default when 'repo_url' is omitted, this will hash the entire terraform directory. This includes '.tf', '.tfvars', and '.tfstate' files. Any subsequent terraform run or state update will change this hash, triggering an unnecessary complete rebuild of the container via 'triggers_replace' in 'terraform_data.build'. Consider excluding '.tf', '.tfvars', and '.tfstate*' files from the fileset to maintain idempotency.

  local_source_hash = local.is_local_dir_mode && local.source_dir != "" ? try(
    sha256(join("", [for f in sort(fileset(local.source_dir, "**")) : filesha256("${local.source_dir}/${f}") if !can(regex("(^|/)(\\.(git|ghpc|terraform)|[^/]*\\.tf|[^/]*\\.tfvars|[^/]*\\.tfstate.*)$", f))])),
    ""
  ) : ""

[gemini-code-assist]

The conditional expression 'var.skip_iam_role_grants ? [] : local.principals' uses a list '[]' in one branch and a set 'local.principals' in the other. Terraform requires both branches of a conditional to have compatible types, and 'for_each' must be a map or a set of strings. Providing a list will cause a plan-time type mismatch error. Use 'toset([])' instead of '[]' to ensure type compatibility and prevent deployment failures.

resource "google_kms_crypto_key_iam_member" "this" {
  for_each = var.skip_iam_role_grants ? toset([]) : local.principals

  crypto_key_id = var.crypto_key_id
  role          = "roles/cloudkms.cryptoKeyEncrypterDecrypter"
  member        = each.value
}

[rahimkhan19]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several major features and enhancements to the Cluster Toolkit, including support for Terraform parallelism (--parallelism) to accelerate large-scale GKE deployments, a new cloud-build module, and an int-app-lb module for Internal Application Load Balancers. It also adds support for hybrid Slurm clusters (Slurm v6), Customer-Managed Encryption Keys (CMEK) across multiple modules, and instance_flexibility_policy (machine-type fallback) in MIG NodeSets. Additionally, GKE job cancellation now automatically reclaims unused storage gateways. The review comment correctly identifies an issue in the high-availability Slurm example where an ambiguous subnetwork reference is used, and recommends replacing it with an explicit map-based reference to prevent deployment race conditions.

[gemini-code-assist]

The blueprint defines multiple subnetworks for the network module (primary and proxy subnets). Relying on $(network.subnetwork_self_link) is ambiguous and fragile when multiple subnetworks are present, as it may not resolve to the intended primary subnetwork. It is highly recommended to use explicit map-based references to ensure the correct subnetwork is targeted.

subnetwork_self_link: $(network.subnetworks["$(vars.region)/$(vars.deployment_name)-primary-subnet"].self_link)

References

1. In Cluster Toolkit blueprints, avoid using hardcoded string literals for resource references (such as subnetwork self-links). Instead, use dynamic references (e.g., $(network.subnetworks[...].self_link)) to allow Cluster Toolkit and Terraform to automatically infer dependencies and prevent deployment race conditions.

[rahimkhan19]

/gemini review

[ctk-pr-bot]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a wide range of updates across the Cluster Toolkit, including a new auto-generated blueprint catalog tool, support for Kueue's waitForPodsReady configuration, and GCE reservation capacity polling for integration tests. It also switches GKE clusters to use Private Service Connect (PSC) by defaulting master_ipv4_cidr_block to null, and refactors Docker credential checks to target regional registries during Crane builds. Feedback on these changes highlights several key improvement opportunities: ensuring the Spack setup playbook checks for the existence of /opt/rh to avoid failures on nodes without developer toolsets; using --follow-symlinks with sed in bootstrap scripts to prevent breaking repository symlinks; guarding the spack directory validation in the cluster views against None values; and refactoring the awk script in the reservation polling tool to ensure idempotency when reservation projects change.

[gemini-code-assist]

The ansible.builtin.find task will fail if the /opt/rh directory does not exist (which is common on nodes without developer toolsets, such as the controller). To prevent the playbook from failing, check if /opt/rh exists using ansible.builtin.stat first, and conditionally run the find task. Additionally, use the default([]) filter on devtoolsets.files to prevent undefined variable errors when the find task is skipped.

- name: Check if /opt/rh exists
  ansible.builtin.stat:
    path: /opt/rh
  register: opt_rh

- name: Find installed developer toolsets
  ansible.builtin.find:
    paths: /opt/rh
    patterns: "gcc-toolset-*,devtoolset-*"
    file_type: directory
  register: devtoolsets
  when: opt_rh.stat.exists and opt_rh.stat.isdir

- name: Spack Find DevTool compilers
  ansible.builtin.command: "{{ spack_dir }}/bin/spack compiler find --scope system {{ item.path }}/root/usr/bin"
  loop: "{{ (devtoolsets.files | default([])) | sort(attribute='path') }}"
  loop_control:
    label: "{{ item.path | basename }}"

[gemini-code-assist]

Using sed -i without --follow-symlinks on /etc/yum.repos.d/*.repo can break symlinks by replacing them with regular files. To ensure consistency and prevent breaking any symlinked repository configurations, use --follow-symlinks as done for the main configuration files above.

Suggested change

	sed -i 's/^[[:space:]]*skip_if_unavailable[[:space:]]*=.*/skip_if_unavailable=True/' /etc/yum.repos.d/*.repo 2>/dev/null || true
	sed -i --follow-symlinks 's/^[[:space:]]*skip_if_unavailable[[:space:]]*=.*/skip_if_unavailable=True/' /etc/yum.repos.d/*.repo 2>/dev/null || true

[gemini-code-assist]

Using sed -i without --follow-symlinks on /etc/yum.repos.d/*.repo can break symlinks by replacing them with regular files. To ensure consistency and prevent breaking any symlinked repository configurations, use --follow-symlinks as done for the main configuration files above.

Suggested change

	sed -i 's/^[[:space:]]*skip_if_unavailable[[:space:]]*=.*/skip_if_unavailable=True/' /etc/yum.repos.d/*.repo 2>/dev/null || true
	sed -i --follow-symlinks 's/^[[:space:]]*skip_if_unavailable[[:space:]]*=.*/skip_if_unavailable=True/' /etc/yum.repos.d/*.repo 2>/dev/null || true

[gemini-code-assist]

Using sed -i without --follow-symlinks on /etc/yum.repos.d/*.repo can break symlinks by replacing them with regular files. To ensure consistency and prevent breaking any symlinked repository configurations, use --follow-symlinks as done for the main configuration files above.

Suggested change

	sed -i 's/^[[:space:]]*skip_if_unavailable[[:space:]]*=.*/skip_if_unavailable=True/' /etc/yum.repos.d/*.repo 2>/dev/null || true
	sed -i --follow-symlinks 's/^[[:space:]]*skip_if_unavailable[[:space:]]*=.*/skip_if_unavailable=True/' /etc/yum.repos.d/*.repo 2>/dev/null || true

[gemini-code-assist]

If spackdir is None (e.g., if it is optional or not configured), calling spackdir.startswith(...) will raise an AttributeError: 'NoneType' object has no attribute 'startswith', causing a 500 error. Guard this validation block by checking if spackdir is set.

Suggested change

	spackdir = self.object.spackdir
	mount_paths = [
	mp_form.cleaned_data.get("mount_path")
	for mp_form in mountpoints.forms
	if mp_form.cleaned_data
	and not mp_form.cleaned_data.get("DELETE", False)
	and mp_form.cleaned_data.get("mount_path")
	]
	if not any(
	spackdir == mp or spackdir.startswith(mp.rstrip("/") + "/")
	for mp in mount_paths
	):
	form.add_error(
	None,
	f"The spack directory ({spackdir}) is not on any shared mount "
	"point. Add a mount point whose path is a parent of the spack "
	"directory, or change the spack directory.",
	)
	return self.form_invalid(form)
	spackdir = self.object.spackdir
	if spackdir:
	mount_paths = [
	mp_form.cleaned_data.get("mount_path")
	for mp_form in mountpoints.forms
	if mp_form.cleaned_data
	and not mp_form.cleaned_data.get("DELETE", False)
	and mp_form.cleaned_data.get("mount_path")
	]
	if not any(
	spackdir == mp or spackdir.startswith(mp.rstrip("/") + "/")
	for mp in mount_paths
	):
	form.add_error(
	None,
	f"The spack directory ({spackdir}) is not on any shared mount "
	"point. Add a mount point whose path is a parent of the spack "
	"directory, or change the spack directory.",
	)
	return self.form_invalid(form)

[gemini-code-assist]

The current awk script is not idempotent when the reservation project changes. If the blueprint already contains a project: line under specific_reservations, the script will silently keep the old project instead of updating it to the new one. Use a state variable to track and replace the existing project: line immediately following the - name: line.

Suggested change

	awk -v proj="${RESERVATION_PROJECT}" '
	pending != "" {
	if ($0 !~ /^[[:space:]]*project:/) print pending
	pending = ""
	}
	/^[[:space:]]*specific_reservations:[[:space:]]*$/ { in_spec = 1 }
	in_spec && /^[[:space:]]*-[[:space:]]*name:/ {
	indent = $0
	sub(/-[[:space:]].*$/, "", indent)
	pending = indent " project: \"" proj "\""
	in_spec = 0
	}
	{ print }
	END { if (pending != "") print pending }
	' "${BLUEPRINT_FILE}" >"${BLUEPRINT_FILE}.tmp" && mv "${BLUEPRINT_FILE}.tmp" "${BLUEPRINT_FILE}"
	awk -v proj="${RESERVATION_PROJECT}" '
	skip_project {
	skip_project = 0
	if ($0 ~ /^[[:space:]]*project:/) next
	}
	/^[[:space:]]*specific_reservations:[[:space:]]*$/ { in_spec = 1 }
	in_spec && /^[[:space:]]*-[[:space:]]*name:/ {
	print
	indent = $0
	sub(/-[[:space:]].*$/, "", indent)
	print indent " project: \"" proj "\""
	in_spec = 0
	skip_project = 1
	next
	}
	{ print }
	' "${BLUEPRINT_FILE}" >"${BLUEPRINT_FILE}.tmp" && mv "${BLUEPRINT_FILE}.tmp" "${BLUEPRINT_FILE}"

[deepakrajk-quad]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive blueprint and module wiring catalog generator, adds support for GCE disk provisioned IOPS and throughput across multiple modules, enables GKE Private Service Connect (PSC) control plane endpoints, and refactors Filestore cleanup and GCE reservation polling in CI workflows. Feedback on the changes highlights critical issues in Django view validation where stale database values are referenced instead of newly submitted form data, as well as shell variable expansion bugs in Cloud Build YAML configurations caused by incorrect backslash escaping.

[gemini-code-assist]

In Django's form_valid, self.object still holds the old database values (or unsaved defaults) because form.save() has not been called yet. To validate the user's newly submitted values, you must retrieve them from form.cleaned_data instead of self.object.

        for field_name in ["controller_instance_type", "login_node_instance_type"]:
            instance_type = form.cleaned_data.get(field_name)

[gemini-code-assist]

Similar to the machine type validation above, self.object.spackdir contains the stale database value. Use form.cleaned_data.get("spackdir") to validate the newly submitted spack directory.

Suggested change

	spackdir = self.object.spackdir
	spackdir = form.cleaned_data.get("spackdir")

[gemini-code-assist]

In Cloud Build, $$ is used to escape the $ character so it is passed literally to the shell. However, prefixing it with a backslash (e.g., \$$ or \$) escapes the $ in the shell as well, preventing bash from expanding the variables at runtime. Inside double quotes, "\$${BP_NAME:-*}" and "\$$DEPLOYMENT_NAME" will be treated as literal strings instead of being expanded.

To allow bash to expand these variables and successfully clear the locks, remove the backslashes.

              gcloud storage rm -r "gs://daily-tests-tf-state/$${BP_NAME:-*}/$${DEPLOYMENT_NAME}/**.tflock" 2>/dev/null || true

References

1. In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g., $${VAR}). Avoid adding an extra dollar sign (like $$$${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.

[gemini-code-assist]

Similar to the variable expansion issue, "\$$SG_EXAMPLE" will be evaluated by bash as the literal string "$SG_EXAMPLE" instead of expanding to "examples/hpc-gke.yaml". This will cause sed, grep, and echo to fail or target a non-existent file. Remove the backslash so it is written as "$${SG_EXAMPLE}".

              sed -i '/^      subnetwork_name: \$(vars.deployment_name)-subnet$/a\      network_address_range: 172.16.0.0/12\n      default_primary_subnetwork_size: 8' "$${SG_EXAMPLE}"
              sed -i 's/use: \[network1, workload_service_account\]/use: [existing_network, workload_service_account]/' "$${SG_EXAMPLE}"
              grep -q "network_address_range: 172.16.0.0/12" "$${SG_EXAMPLE}"
              grep -q "use: \[existing_network, workload_service_account\]" "$${SG_EXAMPLE}"
              echo '  - id: existing_network'                    >> "$${SG_EXAMPLE}"
              echo '    source: modules/network/pre-existing-vpc' >> "$${SG_EXAMPLE}"
              echo '    use: [network1]'                         >> "$${SG_EXAMPLE}"

References

1. In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g., $${VAR}). Avoid adding an extra dollar sign (like $$$${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.

[ctk-pr-bot]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several major enhancements and fixes across the Cluster Toolkit. Key updates include adding a Go-based blueprint catalog generator (pkg/catalog) that updates examples/README.md, refactoring GKE job submission and Docker credential checks, implementing GCE reservation capacity polling for integration tests, and adding support for provisioned IOPS and throughput on boot and additional disks across various Slurm and VM modules. Additionally, GKE clusters now default to Private Service Connect (PSC) for control plane endpoints by setting master_ipv4_cidr_block to null. Feedback on these changes highlights critical issues in the Django web server where self.object is accessed prematurely in form_valid instead of using form.cleaned_data, an Ansible task in spack_setup that fails on Debian/Ubuntu nodes due to a missing /opt/rh directory check, and a missing allow_null: true parameter in the master_ipv4_cidr_block regex validator that will break GKE cluster blueprint validation.

[gemini-code-assist]

In Django's form_valid method, self.object holds either None (for CreateView) or the stale database instance (for UpdateView) before form.save() is called. Accessing self.object.controller_instance_type and self.object.login_node_instance_type directly will either cause an AttributeError on creation or validate the old values on update. You should retrieve these values from form.cleaned_data instead.

        for field_name in ["controller_instance_type", "login_node_instance_type"]:
            instance_type = form.cleaned_data.get(field_name)

[gemini-code-assist]

Similarly, self.object.spackdir should be retrieved from form.cleaned_data to ensure the newly submitted value is validated rather than the stale or None value on self.object.

Suggested change

	spackdir = self.object.spackdir
	spackdir = form.cleaned_data.get("spackdir")

[gemini-code-assist]

The ansible.builtin.find module will fail if the directory /opt/rh does not exist (which is the case on Debian/Ubuntu or controller nodes). Additionally, if the find task is skipped or fails, devtoolsets.files will be undefined, causing the subsequent loop to fail.

You should check if /opt/rh exists first, and use default([]) for the loop.

- name: Check if /opt/rh exists
  ansible.builtin.stat:
    path: /opt/rh
  register: opt_rh_dir

- name: Find installed developer toolsets
  ansible.builtin.find:
    paths: /opt/rh
    patterns: "gcc-toolset-*,devtoolset-*"
    file_type: directory
  register: devtoolsets
  when: opt_rh_dir.stat.exists

- name: Spack Find DevTool compilers
  ansible.builtin.command: "{{ spack_dir }}/bin/spack compiler find --scope system {{ item.path }}/root/usr/bin"
  loop: "{{ (devtoolsets.files | default([])) | sort(attribute='path') }}"
  loop_control:
    label: "{{ item.path | basename }}"

[gemini-code-assist]

The regex validator for master_ipv4_cidr_block is missing allow_null: true. Since master_ipv4_cidr_block defaults to null, this validator will fail during blueprint validation when the default value is used. Please add allow_null: true to the regex validator inputs.

  - validator: regex
    inputs:
      vars: [master_ipv4_cidr_block]
      pattern: ^([0-9]{1,3}\.){3}[0-9]{1,3}/28$
      allow_null: true
    error_message: "master_ipv4_cidr_block must be null or a valid IPv4 /28 CIDR block (e.g. 1.2.3.0/28)."

[ctk-pr-bot]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a machine-readable blueprint and module wiring catalog generator (catalog-gen), adds pre-commit validation for README snippets and links, and implements GCE reservation capacity polling for integration tests. It also updates Slurm v6 modules to support provisioned IOPS and throughput on boot and additional disks, refactors Docker credential checks for regional registries, and refines GKE cluster control plane endpoint configurations. In the frontend views, it adds validation for minimum RAM and shared mount points for Spack, alongside atomic database updates to prevent race conditions. A review comment identifies a potential AttributeError in clusters.py if spackdir is None and suggests guarding the shared mount point check.

[gemini-code-assist]

If self.object.spackdir is None or empty, calling spackdir.startswith() will raise an AttributeError (since NoneType has no startswith attribute). To prevent this crash and handle cases where Spack is not configured, we should guard the check by ensuring spackdir is a non-empty string. This follows defensive programming practices for nullable references.

Suggested change

	spackdir = self.object.spackdir
	mount_paths = [
	mp_form.cleaned_data.get("mount_path")
	for mp_form in mountpoints.forms
	if mp_form.cleaned_data
	and not mp_form.cleaned_data.get("DELETE", False)
	and mp_form.cleaned_data.get("mount_path")
	]
	if not any(
	spackdir == mp or spackdir.startswith(mp.rstrip("/") + "/")
	for mp in mount_paths
	):
	form.add_error(
	None,
	f"The spack directory ({spackdir}) is not on any shared mount "
	"point. Add a mount point whose path is a parent of the spack "
	"directory, or change the spack directory.",
	)
	return self.form_invalid(form)
	spackdir = self.object.spackdir
	if spackdir:
	mount_paths = [
	mp_form.cleaned_data.get("mount_path")
	for mp_form in mountpoints.forms
	if mp_form.cleaned_data
	and not mp_form.cleaned_data.get("DELETE", False)
	and mp_form.cleaned_data.get("mount_path")
	]
	if not any(
	spackdir == mp or spackdir.startswith(mp.rstrip("/") + "/")
	for mp in mount_paths
	):
	form.add_error(
	None,
	f"The spack directory ({spackdir}) is not on any shared mount "
	"point. Add a mount point whose path is a parent of the spack "
	"directory, or change the spack directory.",
	)
	return self.form_invalid(form)

[deepakrajk-quad]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a machine-readable blueprint and module catalog generator, adds robust documentation and snippet validation tools, and implements support for provisioned IOPS and throughput on GCE disks across Slurm v6 modules. It also updates GKE orchestration to handle dry-run conflict bypasses and exclusive-topology adjustments, alongside several compatibility and stability fixes for the frontend (OFE) components. The review feedback highlights opportunities to further harden these changes. Key recommendations include conditionally running the Spack compiler search in Ansible based on directory existence, using try() blocks in Terraform validations to prevent plan-time crashes when the controller state disk is null, implementing a timeout for the dnf-automatic wait loop, and defensively handling missing keys and QuerySet evaluations in the Django views.

[gemini-code-assist]

The ansible.builtin.find task will fail if the /opt/rh directory does not exist on the target node (such as the controller). To prevent playbook failures, check if the directory exists first using ansible.builtin.stat and run the find and compiler tasks conditionally. Additionally, use the default([]) filter in the loop to prevent undefined variable errors when the find task is skipped.

- name: Check if /opt/rh exists
  ansible.builtin.stat:
    path: /opt/rh
  register: opt_rh_dir

- name: Find installed developer toolsets
  ansible.builtin.find:
    paths: /opt/rh
    patterns: "gcc-toolset-*,devtoolset-*"
    file_type: directory
  register: devtoolsets
  when: opt_rh_dir.stat.exists

- name: Spack Find DevTool compilers
  ansible.builtin.command: "{{ spack_dir }}/bin/spack compiler find --scope system {{ item.path }}/root/usr/bin"
  loop: "{{ (devtoolsets.files | default([])) | sort(attribute='path') }}"
  loop_control:
    label: "{{ item.path | basename }}"

[gemini-code-assist]

If var.controller_state_disk is set to null (which is the documented way to disable the state disk), accessing its attributes directly (e.g., var.controller_state_disk.storage_pool) inside the validation blocks will cause a Terraform evaluation error and fail the plan. Use try() to safely access these attributes when the object is null.

  validation {
    condition     = var.controller_state_disk == null || try(var.controller_state_disk.storage_pool, null) == null || try(var.controller_state_disk.storage_pool, "") == "" || contains(["hyperdisk-balanced", "hyperdisk-throughput"], lower(try(var.controller_state_disk.type, "")))
    error_message = "Storage pools are only supported with Hyperdisk types (balanced or throughput)."
  }

  validation {
    condition     = var.controller_state_disk == null || (try(var.controller_state_disk.disk_provisioned_iops, null) == null && try(var.controller_state_disk.provisioned_iops, null) == null) || contains(["hyperdisk-balanced", "hyperdisk-extreme", "pd-extreme"], lower(try(var.controller_state_disk.type, "")))
    error_message = "disk_provisioned_iops is only supported with hyperdisk-balanced, hyperdisk-extreme, or pd-extreme disk types."
  }

  validation {
    condition     = var.controller_state_disk == null || (try(var.controller_state_disk.disk_provisioned_throughput, null) == null && try(var.controller_state_disk.provisioned_throughput, null) == null) || contains(["hyperdisk-balanced", "hyperdisk-throughput", "hyperdisk-ml"], lower(try(var.controller_state_disk.type, "")))
    error_message = "disk_provisioned_throughput is only supported with hyperdisk-balanced, hyperdisk-throughput, or hyperdisk-ml disk types."
  }

References

1. To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.

[gemini-code-assist]

This loop can potentially run indefinitely if dnf-automatic.service gets stuck in an active or activating state, which would block node initialization. It is safer to implement a maximum retry count or timeout to allow the script to proceed or fail gracefully.

Suggested change

	while [[ $(systemctl is-active dnf-automatic.service 2>/dev/null) =~ ^(activating|active|deactivating)$ ]]; do
	sleep 5
	done
	dnf_wait_count=0
	while [[ $(systemctl is-active dnf-automatic.service 2>/dev/null) =~ ^(activating|active|deactivating)$ ]]; do
	if (( dnf_wait_count++ >= 60 )); then
	echo "Warning: dnf-automatic.service did not finish within 5 minutes. Proceeding anyway."
	break
	fi
	sleep 5
	done

References

1. When a user provides an incompatible or unsupported configuration in boot-time setup scripts, log a warning instead of raising a hard error/exception to avoid completely blocking the instance startup.

[gemini-code-assist]

Accessing machine_info[instance_type]["memory"] directly can raise a KeyError or TypeError if machine_info is not fully populated or if instance_type is missing. It is safer to use .get() to handle missing keys defensively, matching the pattern used for partition validation below.

Suggested change

	try:
	node_memory = machine_info[instance_type]["memory"]
	except KeyError:
	form.add_error(field_name, f"Invalid machine type {instance_type}")
	return self.form_invalid(form)
	node_info = machine_info.get(instance_type)
	if not isinstance(node_info, dict) or "memory" not in node_info:
	form.add_error(field_name, f"Invalid machine type {instance_type}")
	return self.form_invalid(form)
	node_memory = node_info["memory"]

References

1. When retrieving dictionary values that might be omitted, using .get(key, default) is sufficient if the key is guaranteed not to be explicitly set to null in the source data. Avoid adding redundant or default fallback logic unless explicit null values are expected.

[gemini-code-assist]

If self.object.spackdir is None or empty, calling spackdir.startswith(...) will raise an AttributeError. Defensively default spackdir to an empty string to prevent runtime crashes.

Suggested change

	spackdir = self.object.spackdir
	mount_paths = [
	mp_form.cleaned_data.get("mount_path")
	for mp_form in mountpoints.forms
	if mp_form.cleaned_data
	and not mp_form.cleaned_data.get("DELETE", False)
	and mp_form.cleaned_data.get("mount_path")
	]
	if not any(
	spackdir == mp or spackdir.startswith(mp.rstrip("/") + "/")
	for mp in mount_paths
	):
	spackdir = self.object.spackdir or ""
	mount_paths = [
	mp_form.cleaned_data.get("mount_path")
	for mp_form in mountpoints.forms
	if mp_form.cleaned_data
	and not mp_form.cleaned_data.get("DELETE", False)
	and mp_form.cleaned_data.get("mount_path")
	]
	if not spackdir or not any(
	spackdir == mp or spackdir.startswith(mp.rstrip("/") + "/")
	for mp in mount_paths
	):

[gemini-code-assist]

In Django, deleting database rows while iterating over a live QuerySet can cause unexpected behavior or database cursor errors depending on the database backend. It is a safer practice to evaluate the QuerySet into a list before iterating and deleting.

                existing_mount_points = list(MountPoint.objects.filter(cluster=self.object))

                # Iterate through the existing mount points and check if they are in the updated formset
                for mount_point in existing_mount_points:
                    if not any(mount_point_form.instance == mount_point for mount_point_form in mountpoints.forms):
                        # The mount point is not in the updated formset, so delete it
                        mount_point_path = mount_point.mount_path
                        mount_point_id = mount_point.pk
                        logger.info(f"Deleting mount point: {mount_point_path}, ID: {mount_point_id}")
                        mount_point.delete()

                # Get the existing ClusterPartition objects associated with the cluster
                existing_partitions = list(ClusterPartition.objects.filter(cluster=self.object))