feat: add nodepool autoscaling vars avail in GKE 1.24.1

[jmymy]

No description provided.

[jmymy]

Any guidance on this as the new flags require a GKE version (1.24.1+) and not a specific provider version?

[apeabody]

Any guidance on this as the new flags require a GKE version (1.24.1+) and not a specific provider version?

Hi @jmymy - I haven't tested/tried, but you might be able to parse the kubernetes version and null the unsupported arguments for earlier versions.

[jmymy]

Any guidance on this as the new flags require a GKE version (1.24.1+) and not a specific provider version?

Hi @jmymy - I haven't tested/tried, but you might be able to parse the kubernetes version and null the unsupported arguments for earlier versions.

But we wouldnt know that version at cluster create time, only the release channel? Still thinking through this

[jmymy]

I guess we could do something like the following, but seems like a lot of logic and work to do for something that will be gone in a few months....

data "google_container_engine_versions" "central1b" {
  provider       = google-beta
  location       = "us-central1-b"
  version_prefix = "1.12."
}

resource "google_container_cluster" "foo" {
  name               = "terraform-test-cluster"
  location           = "us-central1-b"
  node_version       = data.google_container_engine_versions.central1b.latest_node_version
  initial_node_count = 1
}

output "stable_channel_version" {
  value = data.google_container_engine_versions.central1b.release_channel_default_version["STABLE"]
}

I would assume the API still takes these values, but just doesnt do anything if the cluster isnt on this version? need to dig in api spec.

[apeabody]

I guess we could do something like the following, but seems like a lot of logic and work to do for something that will be gone in a few months....

data "google_container_engine_versions" "central1b" {
  provider       = google-beta
  location       = "us-central1-b"
  version_prefix = "1.12."
}

resource "google_container_cluster" "foo" {
  name               = "terraform-test-cluster"
  location           = "us-central1-b"
  node_version       = data.google_container_engine_versions.central1b.latest_node_version
  initial_node_count = 1
}

output "stable_channel_version" {
  value = data.google_container_engine_versions.central1b.release_channel_default_version["STABLE"]
}

I would assume the API still takes these values, but just doesnt do anything if the cluster isnt on this version? need to dig in api spec.

Hi @jmymy - I seem to recall that local.master_version might already do something similar: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/main.tf#L46. Depending on the config, this is used for the min_master_version: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/cluster.tf#L57. Might be useful?

[jmymy]

cloudbuild can now send logs along with the github status checks so we can see them here. Dont know if thats something yall can enable @bharathkkb @Jberlinsky

[jmymy]

@apeabody so the issue i'm hitting is that i cant do a compare on versions like i can in a provider block

location_policy = lookup(autoscaling.value, "location_policy", var.release_channel == "RAPID" || local.master_version >= "1.24.1" ? "BALANCED" : null)

[apeabody]

@apeabody so the issue i'm hitting is that i cant do a compare on versions like i can in a provider block

location_policy = lookup(autoscaling.value, "location_policy", var.release_channel == "RAPID" || local.master_version >= "1.24.1" ? "BALANCED" : null)

Thanks @jmymy - I just re-triggered the CI. Where you able to confirm the API just ignores the values with earlier GKE versions? If that is true, it's probably OK, but we should note with a comment in the files to avoid confusion.

[jmymy]

@apeabody i went the easier route and just tried to do it instead lol
│ Error: error creating NodePool: googleapi: Error 400: Location policy cannot be set for nodepools in clusters that does not support the feature., badRequest

[apeabody]

@apeabody i went the easier route and just tried to do it instead lol │ Error: error creating NodePool: googleapi: Error 400: Location policy cannot be set for nodepools in clusters that does not support the feature., badRequest

Thanks @jmymy! - I was guessing that might be the result. My current suggestion would be two dynamic blocks (pre/post GKE 1.24.1) and enable the desired one either on (ideally) the GKE version, or at least a boolean with the default as the original behavior.

@bharathkkb - Rather than reinventing the wheel, do you happen to know if we have an existing example of a semver comparison (>=) function?

[bharathkkb]

@apeabody @jmymy sorry for the delay. I am not aware of an easy way to do semver compare but could we make the defaults null?
total_min_node_count = lookup(autoscaling.value, "total_min_count", null)
This way the users enable these features and if it is not supported the API will throw an error?

[jmymy]

@apeabody @jmymy sorry for the delay. I am not aware of an easy way to do semver compare but could we make the defaults null? total_min_node_count = lookup(autoscaling.value, "total_min_count", null) This way the users enable these features and if it is not supported the API will throw an error?

@bharathkkb all set and updated

[apeabody]

Thanks @bharathkkb!

Thanks @jmymy! - I just re-triggered the CI as it might be a transient error.

[comment-bot-dev]

@jmymy
Thanks for the PR! 🚀
✅ Lint checks have passed.

[apeabody]

Thanks @jmymy!