Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reserve IP addresses for Slurm instances. #149

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
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
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ No modules.
|------|-------------|------|---------|:--------:|
| <a name="input_access_config"></a> [access\_config](#input\_access\_config) | Access configurations, i.e. IPs via which the VM instance can be accessed via the Internet. | <pre>list(object({<br> nat_ip = string<br> network_tier = string<br> }))</pre> | `[]` | no |
| <a name="input_additional_disks"></a> [additional\_disks](#input\_additional\_disks) | List of maps of additional disks. See https://www.terraform.io/docs/providers/google/r/compute_instance_template#disk_name | <pre>list(object({<br> disk_name = string<br> device_name = string<br> auto_delete = bool<br> boot = bool<br> disk_size_gb = number<br> disk_type = string<br> disk_labels = map(string)<br> }))</pre> | `[]` | no |
| <a name="input_additional_networks"></a> [additional\_networks](#input\_additional\_networks) | Additional network interface details for GCE, if any. | <pre>list(object({<br> network = string<br> subnetwork = string<br> subnetwork_project = string<br> network_ip = string<br> access_config = list(object({<br> nat_ip = string<br> network_tier = string<br> }))<br> ipv6_access_config = list(object({<br> network_tier = string<br> }))<br> }))</pre> | `[]` | no |
| <a name="input_additional_networks"></a> [additional\_networks](#input\_additional\_networks) | Additional network interface details for GCE, if any. | <pre>list(object({<br> network = string<br> subnetwork = string<br> network_ip = string<br> access_config = list(object({<br> nat_ip = string<br> network_tier = string<br> }))<br> ipv6_access_config = list(object({<br> network_tier = string<br> }))<br> }))</pre> | `[]` | no |
| <a name="input_alias_ip_range"></a> [alias\_ip\_range](#input\_alias\_ip\_range) | An array of alias IP ranges for this network interface. Can only be specified for network interfaces on subnet-mode networks.<br>ip\_cidr\_range: The IP CIDR range represented by this alias IP range. This IP CIDR range must belong to the specified subnetwork and cannot contain IP addresses reserved by system or used by other network interfaces. At the time of writing only a netmask (e.g. /24) may be supplied, with a CIDR format resulting in an API error.<br>subnetwork\_range\_name: The subnetwork secondary range name specifying the secondary range from which to allocate the IP CIDR range for this alias IP range. If left unspecified, the primary range of the subnetwork will be used. | <pre>object({<br> ip_cidr_range = string<br> subnetwork_range_name = string<br> })</pre> | `null` | no |
| <a name="input_auto_delete"></a> [auto\_delete](#input\_auto\_delete) | Whether or not the boot disk should be auto-deleted | `string` | `"true"` | no |
| <a name="input_automatic_restart"></a> [automatic\_restart](#input\_automatic\_restart) | (Optional) Specifies whether the instance should be automatically restarted if it is terminated by Compute Engine (not terminated by a user). | `bool` | `true` | no |
Expand Down Expand Up @@ -87,8 +87,7 @@ No modules.
| <a name="input_spot"></a> [spot](#input\_spot) | Provision as a SPOT preemptible instance.<br>See https://cloud.google.com/compute/docs/instances/spot for more details. | `bool` | `false` | no |
| <a name="input_stack_type"></a> [stack\_type](#input\_stack\_type) | The stack type for this network interface to identify whether the IPv6 feature is enabled or not. Values are `IPV4_IPV6` or `IPV4_ONLY`. Default behavior is equivalent to IPV4\_ONLY. | `string` | `null` | no |
| <a name="input_startup_script"></a> [startup\_script](#input\_startup\_script) | User startup script to run when instances spin up | `string` | `""` | no |
| <a name="input_subnetwork"></a> [subnetwork](#input\_subnetwork) | The name of the subnetwork to attach this interface to. The subnetwork must exist in the same region this instance will be created in. Either network or subnetwork must be provided. | `string` | `""` | no |
| <a name="input_subnetwork_project"></a> [subnetwork\_project](#input\_subnetwork\_project) | The ID of the project in which the subnetwork belongs. If it is not provided, the provider project is used. | `string` | `""` | no |
| <a name="input_subnetwork"></a> [subnetwork](#input\_subnetwork) | The subnetwork self-link to attach this interface to. The subnetwork must exist in the same region this instance will be created in. Either network or subnetwork must be provided. | `string` | `""` | no |
| <a name="input_tags"></a> [tags](#input\_tags) | Network tags, provided as a list | `list(string)` | `[]` | no |
| <a name="input_threads_per_core"></a> [threads\_per\_core](#input\_threads\_per\_core) | The number of threads per physical core. To disable simultaneous multithreading (SMT) set this to 1. | `number` | `null` | no |
| <a name="input_total_egress_bandwidth_tier"></a> [total\_egress\_bandwidth\_tier](#input\_total\_egress\_bandwidth\_tier) | Network bandwidth tier. Note: machine\_type must be a supported type. Values are 'TIER\_1' or 'DEFAULT'.<br>See https://cloud.google.com/compute/docs/networking/configure-vm-with-high-bandwidth-configuration for details. | `string` | `"DEFAULT"` | no |
Expand Down
18 changes: 8 additions & 10 deletions terraform/slurm_cluster/modules/_instance_template/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,11 @@ resource "google_compute_instance_template" "tpl" {
}

network_interface {
network = var.network
subnetwork = var.subnetwork
subnetwork_project = var.subnetwork_project
network_ip = length(var.network_ip) > 0 ? var.network_ip : null
nic_type = local.nic_type
stack_type = var.stack_type
network = var.network
subnetwork = var.subnetwork
network_ip = length(var.network_ip) > 0 ? var.network_ip : null
nic_type = local.nic_type
stack_type = var.stack_type
dynamic "access_config" {
for_each = var.access_config
content {
Expand All @@ -140,10 +139,9 @@ resource "google_compute_instance_template" "tpl" {
dynamic "network_interface" {
for_each = var.additional_networks
content {
network = network_interface.value.network
subnetwork = network_interface.value.subnetwork
subnetwork_project = network_interface.value.subnetwork_project
network_ip = length(network_interface.value.network_ip) > 0 ? network_interface.value.network_ip : null
network = network_interface.value.network
subnetwork = network_interface.value.subnetwork
network_ip = length(network_interface.value.network_ip) > 0 ? network_interface.value.network_ip : null
dynamic "access_config" {
for_each = network_interface.value.access_config
content {
Expand Down
15 changes: 4 additions & 11 deletions terraform/slurm_cluster/modules/_instance_template/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,7 @@ variable "nic_type" {
}

variable "subnetwork" {
description = "The name of the subnetwork to attach this interface to. The subnetwork must exist in the same region this instance will be created in. Either network or subnetwork must be provided."
type = string
default = ""
}

variable "subnetwork_project" {
description = "The ID of the project in which the subnetwork belongs. If it is not provided, the provider project is used."
description = "The subnetwork self-link to attach this interface to. The subnetwork must exist in the same region this instance will be created in. Either network or subnetwork must be provided."
type = string
default = ""
}
Expand All @@ -219,10 +213,9 @@ variable "additional_networks" {
description = "Additional network interface details for GCE, if any."
default = []
type = list(object({
network = string
subnetwork = string
subnetwork_project = string
network_ip = string
network = string
subnetwork = string
network_ip = string
access_config = list(object({
nat_ip = string
network_tier = string
Expand Down
10 changes: 5 additions & 5 deletions terraform/slurm_cluster/modules/_slurm_instance/README_TF.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ No modules.

| Name | Type |
|------|------|
| [google_compute_address.static_ip](https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_address) | resource |
| [google_compute_instance_from_template.slurm_instance](https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_instance_from_template) | resource |
| [null_resource.replace_trigger](https://registry.terraform.io/providers/hashicorp/null/latest/docs/resources/resource) | resource |
| [google_compute_instance_template.base](https://registry.terraform.io/providers/hashicorp/google/latest/docs/data-sources/compute_instance_template) | data source |
Expand All @@ -53,9 +54,9 @@ No modules.
|------|-------------|------|---------|:--------:|
| <a name="input_access_config"></a> [access\_config](#input\_access\_config) | Access configurations, i.e. IPs via which the VM instance can be accessed via the Internet. | <pre>list(object({<br> nat_ip = string<br> network_tier = string<br> }))</pre> | `[]` | no |
| <a name="input_add_hostname_suffix"></a> [add\_hostname\_suffix](#input\_add\_hostname\_suffix) | Adds a suffix to the hostname | `bool` | `true` | no |
| <a name="input_additional_networks"></a> [additional\_networks](#input\_additional\_networks) | Additional network interface details for GCE, if any. | <pre>list(object({<br> access_config = optional(list(object({<br> nat_ip = string<br> network_tier = string<br> })), [])<br> alias_ip_range = optional(list(object({<br> ip_cidr_range = string<br> subnetwork_range_name = string<br> })), [])<br> ipv6_access_config = optional(list(object({<br> network_tier = string<br> })), [])<br> network = optional(string)<br> network_ip = optional(string, "")<br> nic_type = optional(string)<br> queue_count = optional(number)<br> stack_type = optional(string)<br> subnetwork = optional(string)<br> subnetwork_project = optional(string)<br> }))</pre> | `[]` | no |
| <a name="input_hostname"></a> [hostname](#input\_hostname) | Hostname of instances | `string` | `""` | no |
| <a name="input_hostname_suffix_separator"></a> [hostname\_suffix\_separator](#input\_hostname\_suffix\_separator) | Separator character to compose hostname when add\_hostname\_suffix is set to true. | `string` | `"-"` | no |
| <a name="input_additional_networks"></a> [additional\_networks](#input\_additional\_networks) | Additional network interface details for GCE, if any. | <pre>list(object({<br> access_config = optional(list(object({<br> nat_ip = string<br> network_tier = string<br> })), [])<br> alias_ip_range = optional(list(object({<br> ip_cidr_range = string<br> subnetwork_range_name = string<br> })), [])<br> ipv6_access_config = optional(list(object({<br> network_tier = string<br> })), [])<br> network = optional(string)<br> network_ip = optional(string, "")<br> nic_type = optional(string)<br> queue_count = optional(number)<br> stack_type = optional(string)<br> subnetwork = optional(string)<br> }))</pre> | `[]` | no |
| <a name="input_disable_address_reservation"></a> [disable\_address\_reservation](#input\_disable\_address\_reservation) | Disable reserving IP addresses in network for instance | `bool` | `false` | no |
| <a name="input_hostname"></a> [hostname](#input\_hostname) | Hostname of instances | `string` | n/a | yes |
| <a name="input_instance_template"></a> [instance\_template](#input\_instance\_template) | Instance template self\_link used to create compute instances | `string` | n/a | yes |
| <a name="input_labels"></a> [labels](#input\_labels) | Labels, provided as a map. Merged and takes precedence over labels on instance template | `map(string)` | `{}` | no |
| <a name="input_metadata"></a> [metadata](#input\_metadata) | Metadata, provided as a map | `map(string)` | `{}` | no |
Expand All @@ -67,8 +68,7 @@ No modules.
| <a name="input_slurm_cluster_name"></a> [slurm\_cluster\_name](#input\_slurm\_cluster\_name) | Cluster name, used for resource naming. | `string` | n/a | yes |
| <a name="input_slurm_instance_role"></a> [slurm\_instance\_role](#input\_slurm\_instance\_role) | Slurm instance type. Must be one of: controller; login; compute. | `string` | `null` | no |
| <a name="input_static_ips"></a> [static\_ips](#input\_static\_ips) | List of static IPs for VM instances | `list(string)` | `[]` | no |
| <a name="input_subnetwork"></a> [subnetwork](#input\_subnetwork) | Subnet to deploy to. Only one of network or subnetwork should be specified. | `string` | `""` | no |
| <a name="input_subnetwork_project"></a> [subnetwork\_project](#input\_subnetwork\_project) | The project that subnetwork belongs to | `string` | `""` | no |
| <a name="input_subnetwork"></a> [subnetwork](#input\_subnetwork) | Subnetwork self-link to deploy to. Only one of network or subnetwork should be specified. | `string` | `""` | no |
| <a name="input_zone"></a> [zone](#input\_zone) | Zone where the instances should be created. If not specified, instances will be spread across available zones in the region. | `string` | `null` | no |

## Outputs
Expand Down
85 changes: 62 additions & 23 deletions terraform/slurm_cluster/modules/_slurm_instance/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,14 @@
##########

locals {
hostname = var.hostname == "" ? "default" : var.hostname
wiktorn marked this conversation as resolved.
Show resolved Hide resolved
_hostnames = [
for index in range(local.num_instances) : (
var.add_hostname_suffix
? format("%s%s%s", var.hostname, "-", format("%03d", index + 1))
: var.hostname
)
]

num_instances = length(var.static_ips) == 0 ? var.num_instances : length(var.static_ips)

# local.static_ips is the same as var.static_ips with a dummy element appended
Expand All @@ -34,24 +41,40 @@ locals {
#################

locals {
network_interfaces = [for index in range(local.num_instances) :
concat([
{
access_config = var.access_config
alias_ip_range = []
ipv6_access_config = []
network = var.network
network_ip = length(var.static_ips) == 0 ? "" : element(local.static_ips, index)
nic_type = null
queue_count = null
stack_type = null
subnetwork = var.subnetwork
subnetwork_project = var.subnetwork_project
}
_network_interfaces = [for index in range(local.num_instances) :
concat(
[
{
access_config = var.access_config
alias_ip_range = []
ipv6_access_config = []
network = var.network
network_ip = length(var.static_ips) == 0 ? "" : element(local.static_ips, index)
nic_type = null
queue_count = null
stack_type = null
subnetwork = var.subnetwork
}
],
var.additional_networks
)
]
network_config = [
for index in range(local.num_instances) : {
hostname = local._hostnames[index]
network_interfaces = [
for nic_ind, nic in local._network_interfaces[index] : merge(
nic,
{
# generate unique name for ip address name based on hostname and index in the list
# regrettably, can't use anything derived from network / subnetwork here, as those are known
wiktorn marked this conversation as resolved.
Show resolved Hide resolved
# only after apply
ip_address_name = "${var.slurm_cluster_name}-${local._hostnames[index]}-nic${nic_ind}"
}
)
]
}
]

slurm_instance_role = lower(var.slurm_instance_role)

Expand Down Expand Up @@ -87,14 +110,14 @@ resource "null_resource" "replace_trigger" {

resource "google_compute_instance_from_template" "slurm_instance" {
count = local.num_instances
name = var.add_hostname_suffix ? format("%s%s%s", local.hostname, var.hostname_suffix_separator, format("%03d", count.index + 1)) : local.hostname
name = local.network_config[count.index].hostname
project = var.project_id
zone = var.zone == null ? data.google_compute_zones.available.names[count.index % length(data.google_compute_zones.available.names)] : var.zone

allow_stopping_for_update = true

dynamic "network_interface" {
for_each = local.network_interfaces[count.index]
for_each = local.network_config[count.index].network_interfaces
iterator = nic
content {
dynamic "access_config" {
Expand All @@ -118,12 +141,11 @@ resource "google_compute_instance_from_template" "slurm_instance" {
network_tier = access_config.value.network_tier
}
}
network = nic.value.network
network_ip = nic.value.network_ip
nic_type = nic.value.nic_type
queue_count = nic.value.queue_count
subnetwork = nic.value.subnetwork
subnetwork_project = nic.value.subnetwork_project
network = nic.value.network
network_ip = var.disable_address_reservation ? nic.value.network_ip : google_compute_address.static_ip[nic.value.ip_address_name].address
nic_type = nic.value.nic_type
queue_count = nic.value.queue_count
subnetwork = nic.value.subnetwork
}
}

Expand Down Expand Up @@ -153,3 +175,20 @@ resource "google_compute_instance_from_template" "slurm_instance" {
replace_triggered_by = [null_resource.replace_trigger.id]
}
}

##############
# IP ADDRESS #
##############

resource "google_compute_address" "static_ip" {
for_each = var.disable_address_reservation ? {} : {
for nic in flatten(
[for net_cfg in local.network_config : [for nic in net_cfg.network_interfaces : nic]]
) : nic.ip_address_name => nic
}
name = each.value.ip_address_name
subnetwork = each.value.subnetwork
address_type = "INTERNAL"
region = var.region
address = each.value.network_ip
Copy link
Collaborator

@mr0re1 mr0re1 Aug 2, 2024

Choose a reason for hiding this comment

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

Isn't project should be set to handle SharedVPC?
I guess it should be a project from subnetwork.
Caveat: we never provide var.subnetwork_project, instead subnetwork is always a selfLink, that is sufficient for the providers.
In your case you would need to:

  • Rename var.subnetwork -> var.subnetwork_self_link
  • And either parse selfLink or create data source to extract project_id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The project doesn't have to (or even can't?) be in the SharedVPC. For sure what worked for me, creating them in service project. And I think it is better from management perspective (you see all the reserved IP addresses in service project).

But I'll do the renames and remove subnetwork_project and see how it works.

Copy link
Contributor Author

@wiktorn wiktorn Aug 6, 2024

Choose a reason for hiding this comment

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

Rename in _slurm_instance will require aligning the call sites of the module in cluster-toolkit.

Preliminary version of changes is available here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per discussion, I rolled back renaming subnetwork to subnetwork_self_link, but I dropped the subnetwork_project.

Should I mark it as DEPRECATED instead of silently dropping, as it is done in Cluster Toolkit?

}
34 changes: 13 additions & 21 deletions terraform/slurm_cluster/modules/_slurm_instance/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,14 @@ variable "network" {
}

variable "subnetwork" {
description = "Subnet to deploy to. Only one of network or subnetwork should be specified."
type = string
default = ""
}

variable "subnetwork_project" {
description = "The project that subnetwork belongs to"
description = "Subnetwork self-link to deploy to. Only one of network or subnetwork should be specified."
type = string
default = ""
}

variable "hostname" {
description = "Hostname of instances"
type = string
default = ""
}

variable "add_hostname_suffix" {
Expand All @@ -66,17 +59,22 @@ variable "additional_networks" {
ipv6_access_config = optional(list(object({
network_tier = string
})), [])
network = optional(string)
network_ip = optional(string, "")
nic_type = optional(string)
queue_count = optional(number)
stack_type = optional(string)
subnetwork = optional(string)
subnetwork_project = optional(string)
network = optional(string)
network_ip = optional(string, "")
nic_type = optional(string)
queue_count = optional(number)
stack_type = optional(string)
subnetwork = optional(string)
}))
nullable = false
}

variable "disable_address_reservation" {
type = bool
description = "Disable reserving IP addresses in network for instance"
default = false
}

variable "static_ips" {
description = "List of static IPs for VM instances"
type = list(string)
Expand Down Expand Up @@ -115,12 +113,6 @@ variable "zone" {
default = null
}

variable "hostname_suffix_separator" {
description = "Separator character to compose hostname when add_hostname_suffix is set to true."
type = string
default = "-"
}

variable "metadata" {
type = map(string)
description = "Metadata, provided as a map"
Expand Down
Loading