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

Adds workaround for data.google_compute_zones.available weirdness #788

Merged
merged 1 commit into from
Aug 20, 2019

Conversation

jlerche
Copy link
Contributor

@jlerche jlerche commented Aug 19, 2019

What problem does this PR solve?

length(data.google_compute_zones.available)

returns 5 for us-west1.

What is changed and how does it work?

In deploy/modules/gcp/tidb-cluster/main.tf switches to

length(data.google_compute_zones.available.names)

which correctly reteurns 3

Check List

Tests

  • Manual test (add detailed scripts or steps below):

    • terraform apply
    • When complete, note that number of pods is equal to the number of availability zones in the region (3 for us-west1).
    • terraform destroy

Code changes

  • Has Terraform changes

Related changes

  • Need to cherry-pick to the release branch

Does this PR introduce a user-facing change?:

NONE

@jlerche jlerche requested review from gregwebs and aylei August 19, 2019 16:41
@jlerche jlerche self-assigned this Aug 19, 2019
Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM

@aylei
Copy link
Contributor

aylei commented Aug 20, 2019

/run-e2e-in-kind

@cofyc
Copy link
Contributor

cofyc commented Aug 20, 2019

have you tested it in region us-central1? it has four zones. Does it have a problem?

@jlerche
Copy link
Contributor Author

jlerche commented Aug 20, 2019

have you tested it in region us-central1? it has four zones. Does it have a problem?

It correctly returns 4. Here's a quick script to demonstrate:

$ cat main.tf 
variable "region" {
  default = "us-west1"
}

variable "project" { default = "jacob-operator" }

provider "google" {
  project = var.project
  region  = var.region
  credentials = file("/path/to/credentials/file.json")
}
data "google_compute_zones" "available" {}

output "zones" {
  value = "${data.google_compute_zones.available.names}"
}

output "length" {value=length(data.google_compute_zones.available.names)}
$ terraform apply -var 'region=us-central1'
data.google_compute_zones.available: Refreshing state...

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

length = 4
zones = [
  "us-central1-a",
  "us-central1-b",
  "us-central1-c",
  "us-central1-f",
]

@cofyc
Copy link
Contributor

cofyc commented Aug 20, 2019

/run-e2e-in-kind

@jlerche jlerche merged commit 89d41bd into pingcap:master Aug 20, 2019
@jlerche jlerche deleted the gcp_terraform_fix_replica_count branch August 20, 2019 06:47
@cofyc
Copy link
Contributor

cofyc commented Aug 21, 2019

this place may not be right

pd_count = var.pd_node_count * local.num_availability_zones
tikv_count = var.tikv_node_count * local.num_availability_zones
tidb_count = var.tidb_node_count * local.num_availability_zones

the default number of zones of the regional cluster is three but we will create node*4 instances.

I guess we need to use the number of cluster.locations. It's not available in google_container_cluster resource attributes. One hack method is to run this command:

gcloud container clusters list --region us-central1 --filter="name='$name'" --format='csv[no-heading](locations)'

@jlerche
Copy link
Contributor Author

jlerche commented Aug 21, 2019

So even in us-central1, a regional cluster will still have 3 zones by default? Will the gcloud command also work for regions like us-west1?

@cofyc
Copy link
Contributor

cofyc commented Aug 21, 2019

yes, by default it will choose 3 from all available zones (4 in us-central1). This behavior can be changed by specifying the desired node locations with --node-locations flag. I'm working on a PR to support the zonal/multi-zonal cluster.

Will the gcloud command also work for regions like us-west1?

It works too. This command retrieves the locations field from the cluster object. It's cluster's attribute (I think we can send a PR to google terraform provider for this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants