Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/compute/vm-instance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ limitations under the License.
| <a name="input_network_self_link"></a> [network\_self\_link](#input\_network\_self\_link) | The self link of the network to attach the VM. Can use "default" for the default network. | `string` | `null` | no |
| <a name="input_network_storage"></a> [network\_storage](#input\_network\_storage) | An array of network attached storage mounts to be configured. | <pre>list(object({<br/> server_ip = string,<br/> remote_mount = string,<br/> local_mount = string,<br/> fs_type = string,<br/> mount_options = string,<br/> client_install_runner = map(string)<br/> mount_runner = map(string)<br/> }))</pre> | `[]` | no |
| <a name="input_on_host_maintenance"></a> [on\_host\_maintenance](#input\_on\_host\_maintenance) | Describes maintenance behavior for the instance. If left blank this will default to `MIGRATE` except for when `placement_policy`, spot provisioning, or GPUs require it to be `TERMINATE` | `string` | `null` | no |
| <a name="input_placement_policy"></a> [placement\_policy](#input\_placement\_policy) | Control where your VM instances are physically located relative to each other within a zone.<br/>See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_resource_policy#nested_group_placement_policy | `any` | `null` | no |
| <a name="input_placement_policy"></a> [placement\_policy](#input\_placement\_policy) | Control where your VM instances are physically located relative to each other within a zone.<br/>See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_resource_policy#nested_group_placement_policy<br/><br/>Supported fields:<br/> vm_count (number) </br> availability_domain_count (number)</br> collocation (string), max_distance (number)</br> gpu_topology (string). | `any` | `null` | no |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The documentation for placement_policy has some issues:

  1. The HTML line break tags are incorrect (</br>). They should be <br/>.
  2. The list of supported fields is not very readable. Using a list format would improve clarity.
  3. The type column is still any, but it has been updated to a specific object type in variables.tf. The documentation should reflect this change, which may require regenerating the documentation.
Suggested change
| <a name="input_placement_policy"></a> [placement\_policy](#input\_placement\_policy) | Control where your VM instances are physically located relative to each other within a zone.<br/>See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_resource_policy#nested_group_placement_policy<br/><br/>Supported fields:<br/> vm_count (number) </br> availability_domain_count (number)</br> collocation (string), max_distance (number)</br> gpu_topology (string). | `any` | `null` | no |
| <a name="input_placement_policy"></a> [placement\_policy](#input\_placement\_policy) | Control where your VM instances are physically located relative to each other within a zone.<br/>See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_resource_policy#nested_group_placement_policy<br/><br/>Supported fields:<ul><li>`vm_count` (number)</li><li>`availability_domain_count` (number)</li><li>`collocation` (string)</li><li>`max_distance` (number)</li><li>`gpu_topology` (string)</li></ul> | <pre>object({<br/> vm_count = optional(number)<br/> availability_domain_count = optional(number)<br/> collocation = optional(string)<br/> max_distance = optional(number)<br/> gpu_topology = optional(string)<br/> })</pre> | `null` | no |

| <a name="input_project_id"></a> [project\_id](#input\_project\_id) | Project in which the HPC deployment will be created | `string` | n/a | yes |
| <a name="input_provisioning_model"></a> [provisioning\_model](#input\_provisioning\_model) | Provisioning model for cloud instance. | `string` | `null` | no |
| <a name="input_region"></a> [region](#input\_region) | The region to deploy to | `string` | n/a | yes |
Expand Down
2 changes: 2 additions & 0 deletions modules/compute/vm-instance/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ resource "google_compute_resource_policy" "placement_policy" {
availability_domain_count = try(var.placement_policy.availability_domain_count, null)
collocation = try(var.placement_policy.collocation, null)
max_distance = try(var.placement_policy.max_distance, null)
gpu_topology = try(var.placement_policy.gpu_topology, null)
}
}

Expand All @@ -145,6 +146,7 @@ resource "null_resource" "replace_vm_trigger_from_placement" {
availability_domain_count = try(tostring(var.placement_policy.availability_domain_count), "")
max_distance = try(tostring(var.placement_policy.max_distance), "")
collocation = try(var.placement_policy.collocation, "")
gpu_topology = try(var.placement_policy.gpu_topology, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a trailing whitespace at the end of this line. Please remove it to maintain code style consistency.

    gpu_topology              = try(var.placement_policy.gpu_topology, "")

}
}

Expand Down
15 changes: 10 additions & 5 deletions modules/compute/vm-instance/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -314,25 +314,30 @@ variable "placement_policy" {
Control where your VM instances are physically located relative to each other within a zone.
See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_resource_policy#nested_group_placement_policy
EOT

type = any # It's a workaround of lack of `optional` in Terraform 1.2
type = object({
vm_count = optional(number)
availability_domain_count = optional(number)
collocation = optional(string)
max_distance = optional(number)
gpu_topology = optional(string)
})
default = null
validation {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new type check, the validation blocks are not needed. The type checking will ensure the placement_policy is either null or a map of provided keys with each value of the specified type.

Please ensure that this type validation is working on terraform version 1.5.7 (the ctk supported version). You can remove the valdiation blocks after testing to ensure that wrong values are flagged by the type checker

condition = var.placement_policy == null ? true : try(keys(var.placement_policy), null) != null
error_message = <<-EOT
The var.placement_policy should be either unset/null or be a map/object with
fields: vm_count (number), availability_domain_count (number), collocation (string), max_distance (number).
fields: vm_count (number), availability_domain_count (number), collocation (string), max_distance (number), gpu_topology (string).
EOT
}
Comment on lines 325 to 331
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since you've changed the type of placement_policy from any to a specific object, this validation block can be improved. The current condition doesn't prevent providing an empty object (e.g., {}), which would cause a failure at apply time because google_compute_resource_policy requires at least one argument in group_placement_policy. A better validation would ensure that if placement_policy is provided, it contains at least one non-null value.

  validation {
    condition     = var.placement_policy == null || length([for v in values(var.placement_policy) : v if v != null]) > 0
    error_message = <<-EOT
    The var.placement_policy should be either unset/null or be an object with at least one non-null field. Supported fields are: vm_count, availability_domain_count, collocation, max_distance, gpu_topology.
    EOT
  }


validation {
condition = alltrue([
for k in try(keys(var.placement_policy), []) : contains([
"vm_count", "availability_domain_count", "collocation", "max_distance"], k)
"vm_count", "availability_domain_count", "collocation", "max_distance", "gpu_topology"], k)
])
error_message = <<-EOT
The supported fields for var.placement_policy are:
vm_count (number), availability_domain_count (number), collocation (string), max_distance (number).
vm_count (number), availability_domain_count (number), collocation (string), max_distance (number), gpu_topology (string).
EOT
}
}
Expand Down
Loading