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

deploy: Modularize gcp terraform #717

Merged
merged 31 commits into from
Aug 15, 2019

Conversation

jlerche
Copy link
Contributor

@jlerche jlerche commented Aug 1, 2019

What problem does this PR solve?

Modularizes the GCP terraform script and uses released helm charts per #575

What is changed and how does it work?

Comprehensive refactor into modules so multiple tidb clusters can be managed

Check List

Tests

  • Manual test

Code changes

  • Has Terraform changes

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:


@jlerche jlerche requested review from gregwebs and aylei August 1, 2019 02:23
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.

Overall LGTM

deploy/gcp/variables.tf Outdated Show resolved Hide resolved
deploy/gcp/tidbclusters.tf Outdated Show resolved Hide resolved
deploy/gcp/variables.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/bastion/bastion.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/tidb-operator/main.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/tidb-operator/main.tf Outdated Show resolved Hide resolved
@jlerche jlerche marked this pull request as ready for review August 2, 2019 03:12
@jlerche jlerche requested a review from aylei August 2, 2019 03:12
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.

Rest LGTM

deploy/modules/gcp/bastion/bastion.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/bastion/outputs.tf Outdated Show resolved Hide resolved
deploy/gcp/data.tf Outdated Show resolved Hide resolved

maintenance_policy {
daily_maintenance_window {
start_time = "01:00"
Copy link
Contributor

Choose a reason for hiding this comment

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

Then it would be better if users can customize the time, because 01:00 GMT is 9:00 Asia/Beijing

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this get resolved

deploy/modules/gcp/tidb-operator/main.tf Show resolved Hide resolved
deploy/modules/share/tidb-cluster-release/variables.tf Outdated Show resolved Hide resolved
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

For the no new line at end of file, I suggest you configure your editor to auto add new line at end of file when invoking save.

deploy/modules/gcp/bastion/variables.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/project-credentials/main.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/project-credentials/main.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/project-credentials/outputs.tf Outdated Show resolved Hide resolved
deploy/gcp/data.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/tidb-operator/variables.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/tidb-operator/variables.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/tidb-operator/variables.tf Outdated Show resolved Hide resolved
deploy/modules/gcp/tidb-operator/variables.tf Show resolved Hide resolved
deploy/modules/gcp/vpc/outputs.tf Outdated Show resolved Hide resolved
@jlerche
Copy link
Contributor Author

jlerche commented Aug 2, 2019

For the no new line at end of file, I suggest you configure your editor to auto add new line at end of file when invoking save.

Yes I will do that going forward. I've been relying on terraform fmt but it doesn't seem to be adding a newline.

tennix
tennix previously approved these changes Aug 5, 2019
Copy link
Member

@tennix tennix 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
aylei previously approved these changes Aug 6, 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

@tennix tennix added cloud/gcp Google Cloud Platform needs-cherry-pick-1.0 labels Aug 7, 2019
@gregwebs
Copy link
Contributor

gregwebs commented Aug 7, 2019

The file templates/tidb-cluster-values.yaml.tpl seems unused now?

tennix
tennix previously approved these changes Aug 8, 2019
@gregwebs
Copy link
Contributor

gregwebs commented Aug 8, 2019

  • monitor storage is 100 GiB even for the small deployment
  • disks are getting orphaned now

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2019

CLA assistant check
All committers have signed the CLA.

deploy/modules/gcp/tidb-operator/main.tf Outdated Show resolved Hide resolved
@@ -158,4 +158,11 @@ EOS
KUBECONFIG = var.kubeconfig_path
}
}
provisioner "local-exec" {
when = destroy
Copy link
Member

Choose a reason for hiding this comment

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

What does terraform say when updating the tidb-cluster? Will it be recreating which means destroying and creating again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it does not destroy and create again if tidb-cluster changes. For a null_resource as long as triggers are not specified, or the ones that are specified don't change, then it should stay the same.

Copy link
Member

Choose a reason for hiding this comment

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

OK, cool! This is what we need.

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@gregwebs
Copy link
Contributor

apply + destroy works well for me, as does the patch of the reclaim policy (no more orhpaned disks).

@gregwebs gregwebs merged commit 81fe851 into pingcap:master Aug 15, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 15, 2019

cherry pick to release-1.0 failed

@weekface
Copy link
Contributor

@jlerche please open a cherry-pick to release-1.0 branch

jlerche pushed a commit to jlerche/tidb-operator that referenced this pull request Aug 15, 2019
refactor of gcp terraform into modules

* Add maintenance time variable with default
* Changes bastion image to centos
* Removes destroy trigger for patching reclaimpolicy PV
* Adds bash script to change pv reclaimpolicy from Retain to Delete

(cherry picked from commit 81fe851)
weekface pushed a commit that referenced this pull request Aug 15, 2019
refactor of gcp terraform into modules
(cherry picked from commit 81fe851)
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.

7 participants