-
Notifications
You must be signed in to change notification settings - Fork 799
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
Use GCS as the Terraform state backend #2938
Conversation
build/terraform/e2e/module.tf
Outdated
@@ -28,6 +28,10 @@ terraform { | |||
version = "~> 2.3" | |||
} | |||
} | |||
backend "gcs" { | |||
bucket = "agones-e2e-infra-bucket-tfstate" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that a bucket name has to be glopally unique - this should probably be dynamic (use the project name as a prefix?) so it can be run against development projects without failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's doable. But I have doubt whether it makes sense to create an e2e test cluster for development. I was thinking we should push user to just create one test-cluster and use it for the development purpose even the e2e test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With it the way it is, you can't test this Terraform script, except to test it in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was expecting the backend part would be relatively stable since it's only a thing for our maintainers who have access to the infra and won't have many change (if any). But it's still a fair point. I will make it not hardcoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly just something like {project}-tfstate
makes sense to me, that's what I was thinking of doing.
Build Succeeded 👏 Build Id: d206037d-40ed-4487-9e91-c0151e62a7a6 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:
|
Build Failed 😱 Build Id: c7584429-ea28-4b2b-8cb6-21cb9841c898 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: caab8303-f722-408e-8289-57d1b01e28c0 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:
|
build/includes/google-cloud.mk
Outdated
$(DOCKER_RUN) bash -c 'cd $(mount_path)/build/terraform/e2e && terraform destroy -var project=$(GCP_PROJECT) -auto-approve' | ||
|
||
# Creates a gcloud cluster for prow | ||
gcloud-prow-build-cluster: GCP_PROJECT ?= $(shell $(current_project)) | ||
gcloud-prow-build-cluster: $(ensure-build-image) | ||
gcloud-prow-build-cluster: | ||
$(MAKE) terraform-init DIRECTORY=prow | ||
$(MAKE) terraform-init BUCKET=$(GCP_PROJECT)-prow-infra-bucket-tfstate PREFIX=terraform/state DIRECTORY=prow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not strictly necessary here, but given that we are at an inflection point of changing the e2e infrastructure and I don't think any of us understands if the Prow configuration is even working, should we just instead turn down Prow? I am afraid that if we continue as is we'll give someone a false sense of "maybe it works" when I feel like it has reached the point of bitrot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to removing the prow cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to removing the prow cluster.
Who are you, and what have you done with the real @roberthbailey ? 😁
Build Failed 😱 Build Id: e15b8cf6-cc64-4685-b046-c484abfa4b46 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 5b5272ad-2b07-41fa-8daa-171d1ae1a159 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:
|
@@ -14,6 +14,7 @@ | |||
|
|||
|
|||
// Run: | |||
// terraform init -backend-config="bucket=<YOUR_GCP_ProjectID>-e2e-infra-bucket-tfstate" -backend-config="prefix=terraform/state" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiousity, what happens if you don't provide these variables? does terraform error out in an obvious way, or does it default to something bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I would like if it failed brightly, vs just continued without GCS, if that's something easy enough to do.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If starting with a clean state, i.e. there is no tfstate file for this module in your dev environment, terraform init
without those variables will succeed but the local root configuration directory for this module will be used as backend. But the following terraform apply
will fail because the state doesn't match with the existing resources, unless the related resources (clusters, firewalls) are all deleted, which is basically starting from a brand new state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think that's good enough, yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a minor question. It's non-blocking to merge.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gongmax, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 604fc25e-0783-4bfe-805c-b0a109dc840b 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:
|
Build Succeeded 👏 Build Id: 522b5ed9-8b27-4788-af9d-bb80713737c0 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:
|
* Use GCS to as the Terraform state backend * fix format * Make gcs bucket dynamic; Use gcs as terraform backend for prow cluster * remove prow cluster
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Use GCS as the backend of the e2e test cluster Terraform state, so the maintainers can have a central store of the state. This will also benefit the management of the additional e2e test clusters we will introduced in near future.
As a side work, the PR also remove the consul from the Autopilot e2e test cluster.
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer:
Since we will have the central authority of the e2e test clusters state, we should discourage users to create e2e test cluster for their local dev purpose. Users should be able to just create one
test-cluster
and perform all the dev on it including the e2e test. Instruction changes will be in following PRs.