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

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

Conversation

chiayi
Copy link
Contributor

@chiayi chiayi commented Nov 15, 2022

What type of PR is this?
/kind bug

What this PR does / Why we need it:
It adds configuration to terraform so that regional clusters can be created.
It also added autoscaling option for default nodepools.

Which issue(s) this PR fixes:
Closes #1441
Closes #1467

Special notes for your reviewer:
I've also made changes to the README for /build to include the parameters.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 3751761c-f0ee-4470-9640-9fe9adb505b6

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-deb17a1-amd64

build/Makefile Show resolved Hide resolved
default = ""
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.

build/includes/google-cloud.mk Outdated Show resolved Hide resolved
default = ""
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.

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.

@gongmax
Copy link
Collaborator

gongmax commented Nov 16, 2022

I think it's better to also wire these options up to the example module and update the website page in case customer is following the instruction to create the cluster

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8292f534-0b3d-4939-a569-7254fab8137b

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-31df389-amd64

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 87ddce9a-8727-4a94-b492-2d6585e6919e

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8d675391-e10e-49f9-babb-4fb855048b4e

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-5c4a83b-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 53960b90-45ee-4cf1-908a-769d0ef30e3d

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-855e0bc-amd64

@@ -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.location : local.zone
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make this prioritize location.

It looks like warnings are not straightforward, so I think skipping that is fine. But at the very least, let's add an error if they're both non-empty and mismatched. I think it can be done like: https://medium.com/codex/terraform-variable-validation-b9b3e7eddd79

Later we can start erroring when zone is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we make the location a priority, do we still want location to have a default value? This would also effect the validation done on the variable.

As for cross variable validation, it's currently not supported but there seems to be hacky ways around it if we really need the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So zone no longer has a default and will only be used as the location if zone is not empty.

@@ -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

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.

@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Nov 16, 2022
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: cc92fd64-99a6-420d-ba15-244bfb7a1b10

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-fd680de-amd64

@@ -96,6 +96,14 @@ 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.

@google-oss-prow google-oss-prow bot added lgtm and removed lgtm labels Nov 16, 2022
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: c57fee7e-6da0-40e6-a275-34cf0327d970

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-4d41e67-amd64

Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

I noticed while reviewing your change that in build/includes/terraform.mk there is a lot of duplication between the gcloud-terraform-cluster and gcloud-terraform-install targets. If you agree and think there would be a clean way to simplify them, that would be a good change for a follow-up PR.

default = ""
description = "The GCP location to create the cluster in"
}

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.

@@ -30,7 +30,8 @@ variable "cluster" {
type = map

default = {
"zone" = "us-west1-c"
"location" = "us-west1-c"
"zone" = ""
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove zone from here to only have location in our sample configs?

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 we can. There isn't really a reason to have zone here anymore.

@google-oss-prow google-oss-prow bot removed the lgtm label Nov 17, 2022
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7742b8c1-88bd-416e-b68d-bd033161b5be

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-ba32d44-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ebe142e8-7f35-4faa-9d51-d17e206e7cc9

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-6362582-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 004ee176-1a2a-4d56-9de1-244138d6856d

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-ba32d44-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: aa55b7a1-9a91-4058-8d94-cb8b9b5c8dcc

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-367b52e-amd64

* Makefile changes for adding location variable

* added autoscale parameters to Makefile and README

* Markdown fix in readme

* Changed LOCATION to always be set with ZONE as default

* use  only if the variable has a value

* fixed extraneous characters

* update gke terraform exmaple module

* Added autoscale to example cluster and added to website docs

* Added defaults and feature expiry

* Remove zone from gke/variable.tf file.
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 6ff6b1e1-43cd-46b8-90d4-83c929dee02b

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2813/head:pr_2813 && git checkout pr_2813
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-4892725-amd64

@google-oss-prow google-oss-prow bot added the lgtm label Nov 18, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chiayi, gongmax, roberthbailey, zmerlynn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roberthbailey roberthbailey merged commit e3cb2a4 into googleforgames:main Nov 18, 2022
@mangalpalli mangalpalli added this to the 1.28.0 milestone Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform, GKE - add autoscaling Node Pools option Terraform, GKE - Option to create a Regional Cluster
6 participants