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.

Sign up for GitHub

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

Terraform, GKE - Option to create regional cluster as well as option to create autoscaling nodepool #2813

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Terraform, GKE - Option to create regional cluster as well as option to create autoscaling nodepool #2813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions build/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ KUBECONFIG ?= ~/.kube/config
# The (gcloud) test cluster that is being worked against
GCP_CLUSTER_NAME ?= test-cluster
GCP_CLUSTER_ZONE ?= us-west1-c
chiayi marked this conversation as resolved.
Show resolved Hide resolved
GCP_CLUSTER_LOCATION ?= $(GCP_CLUSTER_ZONE)
GCP_BUCKET_CHARTS ?= agones-chart
# the profile to use when developing on minikube
MINIKUBE_PROFILE ?= agones
Expand Down
5 changes: 4 additions & 1 deletion build/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,10 @@ See the table below for available customizations :
| Parameter | Description | Default |
|------------------------------------------------|---------------------------------------------------------------------------------------|-----------------|
| `GCP_CLUSTER_NAME` | The name of the cluster | `test-cluster` |
| `GCP_CLUSTER_ZONE` | The name of the Google Compute Engine zone in which the cluster will resides. | `us-west1-c` |
| `GCP_CLUSTER_ZONE` or `GCP_CLUSTER_LOCATION` | The name of the Google Compute Engine zone/location in which the cluster will resides | `us-west1-c` |
| `GCP_CLUSTER_NODEPOOL_AUTOSCALE` | Whether or not to enable autoscaling on game server nodepool | `false` |
| `GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT` | The number of minimum nodes if autoscale is enabled | `1` |
| `GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT` | The number of maximum nodes if autoscale is enabled | `5` |
| `GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT` | The number of nodes to create in this cluster. | `4` |
| `GCP_CLUSTER_NODEPOOL_MACHINETYPE` | The name of a Google Compute Engine machine type. | `e2-standard-4` |
| `GCP_CLUSTER_NODEPOOL_ENABLEIMAGESTREAMING` | Whether or not to enable image streaming for the `"default"` node pool in the cluster | `true` |
Expand Down
13 changes: 9 additions & 4 deletions build/includes/google-cloud.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,17 @@ gcloud-init: ensure-build-config
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT ?= 4
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_MACHINETYPE ?= e2-standard-4
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_ENABLEIMAGESTREAMING ?= true
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_AUTOSCALE ?= false
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT ?= 1
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT ?= 5
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_WINDOWSINITIALNODECOUNT ?= 0
gcloud-test-cluster: GCP_CLUSTER_NODEPOOL_WINDOWSMACHINETYPE ?= e2-standard-4
gcloud-test-cluster: $(ensure-build-image)
$(MAKE) gcloud-terraform-cluster GCP_TF_CLUSTER_NAME="$(GCP_CLUSTER_NAME)" GCP_CLUSTER_ZONE="$(GCP_CLUSTER_ZONE)" \
$(MAKE) gcloud-terraform-cluster GCP_TF_CLUSTER_NAME="$(GCP_CLUSTER_NAME)" \
GCP_CLUSTER_LOCATION="$(GCP_CLUSTER_LOCATION)" \
GCP_CLUSTER_AUTOSCALE="$(GCP_CLUSTER_NODEPOOL_AUTOSCALE)" \
GCP_CLUSTER_MIN_NODECOUNT="$(GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT)" \
GCP_CLUSTER_MAX_NODECOUNT="$(GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT)" \
GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT="$(GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT)" \
GCP_CLUSTER_NODEPOOL_MACHINETYPE="$(GCP_CLUSTER_NODEPOOL_MACHINETYPE)" \
GCP_CLUSTER_NODEPOOL_ENABLEIMAGESTREAMING="$(GCP_CLUSTER_NODEPOOL_ENABLEIMAGESTREAMING)" \
Expand Down Expand Up @@ -73,9 +80,7 @@ clean-gcloud-prow-build-cluster: $(ensure-build-image)
# Pulls down authentication information for kubectl against a cluster, name can be specified through GCP_CLUSTER_NAME
# (defaults to 'test-cluster')
gcloud-auth-cluster: $(ensure-build-image)
docker run --rm $(common_mounts) $(build_tag) gcloud config set container/cluster $(GCP_CLUSTER_NAME)
docker run --rm $(common_mounts) $(build_tag) gcloud config set compute/zone $(GCP_CLUSTER_ZONE)
docker run --rm $(common_mounts) $(build_tag) gcloud container clusters get-credentials $(GCP_CLUSTER_NAME)
docker run --rm $(common_mounts) $(build_tag) gcloud container clusters get-credentials $(GCP_CLUSTER_NAME) --zone $(GCP_CLUSTER_LOCATION)

# authenticate our docker configuration so that you can do a docker push directly
# to the gcr.io repository
Expand Down
18 changes: 16 additions & 2 deletions build/includes/terraform.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ terraform-clean:
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT ?= 4
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_MACHINETYPE ?= e2-standard-4
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_ENABLEIMAGESTREAMING ?= true
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_AUTOSCALE ?= false
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT ?= 1
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT ?= 5
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_WINDOWSINITIALNODECOUNT ?= 0
gcloud-terraform-cluster: GCP_CLUSTER_NODEPOOL_WINDOWSMACHINETYPE ?= e2-standard-4
gcloud-terraform-cluster: AGONES_VERSION ?= ''
Expand All @@ -53,8 +56,12 @@ gcloud-terraform-cluster:
-var name=$(GCP_TF_CLUSTER_NAME) -var machine_type="$(GCP_CLUSTER_NODEPOOL_MACHINETYPE)" \
-var values_file="" \
-var feature_gates=$(FEATURE_GATES) \
-var zone="$(GCP_CLUSTER_ZONE)" -var project="$(GCP_PROJECT)" \
-var project="$(GCP_PROJECT)" \
-var location="$(GCP_CLUSTER_LOCATION)" \
-var log_level="$(LOG_LEVEL)" \
-var autoscale=$(GCP_CLUSTER_NODEPOOL_AUTOSCALE) \
-var min_node_count=$(GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT) \
-var max_node_count=$(GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT) \
-var node_count=$(GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT) \
-var enable_image_streaming=$(GCP_CLUSTER_NODEPOOL_ENABLEIMAGESTREAMING) \
-var windows_node_count=$(GCP_CLUSTER_NODEPOOL_WINDOWSINITIALNODECOUNT) \
Expand All @@ -66,6 +73,9 @@ gcloud-terraform-cluster:
# Unifies previous `make gcloud-test-cluster` and `make install` targets
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT ?= 4
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_MACHINETYPE ?= e2-standard-4
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_AUTOSCALE ?= false
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT ?= 1
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT ?= 5
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_WINDOWSINITIALNODECOUNT ?= 0
gcloud-terraform-install: GCP_CLUSTER_NODEPOOL_WINDOWSMACHINETYPE ?= e2-standard-4
gcloud-terraform-install: ALWAYS_PULL_SIDECAR := true
Expand All @@ -87,8 +97,12 @@ gcloud-terraform-install:
-var crd_cleanup="$(CRD_CLEANUP)" \
-var chart="../../../install/helm/agones/" \
-var name=$(GCP_TF_CLUSTER_NAME) -var machine_type="$(GCP_CLUSTER_NODEPOOL_MACHINETYPE)" \
-var zone=$(GCP_CLUSTER_ZONE) -var project=$(GCP_PROJECT) \
-var project=$(GCP_PROJECT) \
-var location=$(GCP_CLUSTER_LOCATION) \
-var log_level=$(LOG_LEVEL) \
-var autoscale=$(GCP_CLUSTER_NODEPOOL_AUTOSCALE) \
-var min_node_count=$(GCP_CLUSTER_NODEPOOL_MIN_NODECOUNT) \
-var max_node_count=$(GCP_CLUSTER_NODEPOOL_MAX_NODECOUNT) \
-var feature_gates=$(FEATURE_GATES) \
-var node_count=$(GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT) \
-var windows_node_count=$(GCP_CLUSTER_NODEPOOL_WINDOWSINITIALNODECOUNT) \
Expand Down
25 changes: 23 additions & 2 deletions build/terraform/gke/module.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,14 @@ variable "ping_service_type" {
default = "LoadBalancer"
}

variable "zone" {
variable "location" {
default = "us-west1-c"
description = "The GCP zone to create the cluster in"
description = "The GCP location to create the cluster in"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just deprecate zone? IMO yes, but would like input from @roberthbailey. It would look like:

  • on new line 67, change the default to "us-west1-c"
  • on new line 72, change the default to ""
  • on new line 73, let's say "The GCP zone to create the cluster in (deprecated, use 'location')"

It looks like there is no formal deprecation procedure (hashicorp/terraform#18381) though, so I think to actually enforce the deprecation we'd have to add output to the module if zone were set.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the plan is to

make sure we keep things backwards compatible for at least 1 release and give folks a warning in the release notes that the variable name is changing.

So adding a deprecation note does follow that. But does this mean that zone would take precedence over location for the 1 release before being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the change I just made for this comment, GCP_CLUSTER_ZONE and GCP_CLUSTER_LOCATION will always be set. What would be the correct way to go about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

GCP_CLUSTER_ZONE and GCP_CLUSTER_LOCATION are both set, but I think the change you just made to gcloud-terraform-install sets only location, right?

The logic is a little fussy, but I think we should check:

  • if location is set but not zone, use location (good path)
  • if zone is set but not location, use zone and warn (deprecated path)
  • if location and zone are set but equal, use either and warn.
  • if both are set, and not equal, throw an error

I'm not exactly sure how possible that is in .tf, but it seems like the right approach. If it's too much effort, though, holler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add back zone to gcloud-terraform-install so that both variables will be available. As for adding warnings and errors, not sure if that is possible within the terraform files?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to add zone back in, I was pointing out that the Makefile is only sending the one - so there's no conflict between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay so I made it so that within the tf file, zone will be empty unless explicitly set. And it will only use zone if the variable is empty.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't see this comment thread earlier but I think where you ended up looks good - we use location and we can add to the release notes that folks should switch their configs to use location but we continue to support zone for N releases (maybe 1, maybe a few) to give people time to migrate their configs.

We don't have a formal policy about compatibility of terraform configs (or helm for that matter) but we do try to do our best not to have knife edge changes that would break users upgrading across a single release.

variable "zone" {
default = ""
description = "The GCP zone to create the cluster in (deprecated, use `location`)"
}

variable "pull_policy" {
Expand All @@ -88,6 +93,18 @@ variable "log_level" {
default = "info"
}

variable "autoscale" {
default = "false"
}

variable "min_node_count" {
default = "1"
}

variable "max_node_count" {
default = "5"
}

// Note: This is the number of gameserver nodes. The Agones module will automatically create an additional
// two node pools with 1 node each for "agones-system" and "agones-metrics".
variable "node_count" {
Expand Down Expand Up @@ -116,6 +133,7 @@ module "gke_cluster" {

cluster = {
"name" = var.name
"location" = var.location
"zone" = var.zone
"machineType" = var.machine_type
"initialNodeCount" = var.node_count
Expand All @@ -124,6 +142,9 @@ module "gke_cluster" {
"windowsInitialNodeCount" = var.windows_node_count
"project" = var.project
"network" = var.network
"autoscale" = var.autoscale
"minNodeCount" = var.min_node_count
"maxNodeCount" = var.max_node_count
}
}

Expand Down
25 changes: 23 additions & 2 deletions examples/terraform-submodules/gke/module.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,14 @@ variable "enable_image_streaming" {
default = "true"
}

variable "zone" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to update the website page to indicate the change. Noted for changes to the website, we generally gate them so that they do not appear until the next release. Even though this doesn't require new features in Agones, it still makes sense to wait to publish this guidance until our next release.
https://agones.dev/site/docs/contribute/#within-a-page describes how to add the feature gate tags to the website pages.

Copy link
Contributor Author

@chiayi chiayi Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Done! I think, not sure if I've done it correctly or not.

variable "location" {
default = "us-west1-c"
description = "The GCP zone to create the cluster in"
description = "The GCP location to create the cluster in"
}

variable "zone" {
default = ""
description = "The GCP zone to create the cluster in (deprecated, use `location`)"
}

chiayi marked this conversation as resolved.
Show resolved Hide resolved
variable "network" {
Expand Down Expand Up @@ -84,6 +89,18 @@ variable "windows_machine_type" {
default = "e2-standard-4"
}

variable "autoscale" {
default = "false"
}

variable "min_node_count" {
default = "1"
}

variable "max_node_count" {
default = "5"
}

module "gke_cluster" {
// ***************************************************************************************************
// Update ?ref= to the agones release you are installing. For example, ?ref=release-1.17.0 corresponds
Expand All @@ -93,6 +110,7 @@ module "gke_cluster" {

cluster = {
"name" = var.name
"location" = var.location
"zone" = var.zone
"machineType" = var.machine_type
"initialNodeCount" = var.node_count
Expand All @@ -102,6 +120,9 @@ module "gke_cluster" {
"subnetwork" = var.subnetwork
"windowsInitialNodeCount" = var.windows_node_count
"windowsMachineType" = var.windows_machine_type
"autoscale" = var.autoscale
"mindNodeCount" = var.min_node_count
"maxNodeCount" = var.max_node_count
}
}

Expand Down
18 changes: 15 additions & 3 deletions install/terraform/modules/gke/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ data "google_client_config" "default" {}
# Set values to default if not key was not set in original map
locals {
project = lookup(var.cluster, "project", "agones")
zone = lookup(var.cluster, "zone", "us-west1-c")
location = lookup(var.cluster, "location", "us-west1-c")
zone = lookup(var.cluster, "zone", "")
name = lookup(var.cluster, "name", "test-cluster")
machineType = lookup(var.cluster, "machineType", "e2-standard-4")
initialNodeCount = lookup(var.cluster, "initialNodeCount", "4")
Expand All @@ -33,6 +34,9 @@ locals {
kubernetesVersion = lookup(var.cluster, "kubernetesVersion", "1.23")
windowsInitialNodeCount = lookup(var.cluster, "windowsInitialNodeCount", "0")
windowsMachineType = lookup(var.cluster, "windowsMachineType", "e2-standard-4")
autoscale = lookup(var.cluster, "autoscale", false)
minNodeCount = lookup(var.cluster, "minNodeCount", "1")
maxNodeCount = lookup(var.cluster, "maxNodeCount", "5")
}

# echo command used for debugging purpose
Expand All @@ -56,7 +60,7 @@ resource "null_resource" "test-setting-variables" {

resource "google_container_cluster" "primary" {
name = local.name
location = local.zone
location = local.zone != "" ? local.zone : local.location
project = local.project
network = local.network
subnetwork = local.subnetwork
Expand All @@ -65,9 +69,17 @@ resource "google_container_cluster" "primary" {

node_pool {
name = "default"
node_count = local.initialNodeCount
node_count = local.autoscale ? null : local.initialNodeCount
version = local.kubernetesVersion

dynamic "autoscaling" {
for_each = local.autoscale ? [1] : []
content {
min_node_count = local.minNodeCount
max_node_count = local.maxNodeCount
}
}

management {
auto_upgrade = false
}
Expand Down
5 changes: 4 additions & 1 deletion install/terraform/modules/gke/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ variable "cluster" {
type = map

default = {
"zone" = "us-west1-c"
"location" = "us-west1-c"
"name" = "test-cluster"
"machineType" = "e2-standard-4"
"initialNodeCount" = "4"
Expand All @@ -40,6 +40,9 @@ variable "cluster" {
"kubernetesVersion" = "1.23"
"windowsInitialNodeCount" = "0"
"windowsMachineType" = "e2-standard-4"
"autoscale" = false
"minNodeCount" = "1"
"maxNodeCount" = "5"
}
}

Expand Down
12 changes: 12 additions & 0 deletions site/content/en/docs/Installation/Terraform/gke.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,14 @@ Configurable parameters:
- machine_type - machine type for hosting game servers (default is "e2-standard-4")
- node_count - count of game server nodes for the default node pool (default is "4")
- enable_image_streaming - whether or not to enable image streaming for the `"default"` node pool (default is true)
{{% feature expiryVersion="1.28" %}}
- zone - the name of the [zone](https://cloud.google.com/compute/docs/regions-zones) you want your cluster to be
created in (default is "us-west1-c")
{{% /feature %}}
{{% feature publishVersion="1.28" %}}
- zone - (Deprecated, use location) the name of the [zone](https://cloud.google.com/compute/docs/regions-zones) you want your cluster to be
created in (default is "us-west1-c")
{{% /feature %}}
- network - the name of the VPC network you want your cluster and firewall rules to be connected to (default is "default")
- subnetwork - the name of the subnetwork in which the cluster's instances are launched. (required when using non default network)
- log_level - possible values: Fatal, Error, Warn, Info, Debug (default is "info")
Expand All @@ -96,6 +102,12 @@ Configurable parameters:
- gameserver_maxPort - the upper bound of the port range which gameservers will listen on (default is "8000")
- gameserver_namespaces - a list of namespaces which will be used to run gameservers (default is `["default"]`). For example `["default", "xbox-gameservers", "mobile-gameservers"]`
- force_update - whether or not to force the replacement/update of resource (default is true, false may be required to prevent immutability errors when updating the configuration)
{{% feature publishVersion="1.28" %}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should add another version-guarded section at line 90 to mention that zone is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added feature expiry to the zone. Not sure if this is the best way to do it.

- location - the name of the [location](https://cloud.google.com/compute/docs/regions-zones) you want your cluster to eb created in (default is "us-west1-c")
- autoscale - whether you want to enable autoscale for the gameserver nodepool (default is false)
- min_node_count - the minimum number of nodes for a nodepool when autoscale is enabled (default is "1")
- max_node_count - the maximum number of nodes for a nodepool when autoscale is enabled (default is "5")
{{% /feature %}}

{{% alert title="Warning" color="warning"%}}
On the lines that read `source = "git::https://github.com/googleforgames/agones.git//install/terraform/modules/gke/?ref=main"`
Expand Down