Modularization for AWS terraform scripts #650

aylei · 2019-07-10T17:14:29Z

What problem does this PR solve?

close #628

What is changed and how does it work?

As the issue stated, this PR separate the modules of AWS terraform scripts more clearly and suggest the best practice to manage multiple environments.

The directory name and UX of aws is not changed in favor of consistence, but it should be easy for uses to create a new directory and write some terraform scripts from scratch following the README.

This PR also introduce two minor features based on the community feedbacks:

Now bastion machine can be configured to ssh to worker nodes, and this is enabled by default;
tidb-cluster can be configured to not create the real helm release (means only provision worker nodes), which is useful if users want to manage helm on their own;

Check List

Tests

Not yet

Side effects

Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
- docs-cn
- docs

Does this PR introduce a user-facing change?:

No

- fix hardcode chart version - fix unreachable bastion - adjust tidb-cluster no-wait to avoid scheduled backup pvc blocking - add version check - disable pd node public_ip Signed-off-by: Aylei <rayingecho@gmail.com>

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei · 2019-07-10T17:15:45Z

deploy/modules/share/tidb-cluster-release/main.tf

@@ -1,5 +1,5 @@
 resource "null_resource" "wait-tiller-ready" {


This module is separated in favor of reusing across cloud providers

Signed-off-by: Aylei <rayingecho@gmail.com>

gregwebs · 2019-07-10T20:40:30Z

Looks like there are some conflicts.

gregwebs · 2019-07-10T20:47:39Z

This looks great! However, I think putting this in a separate advanced section will end up with poor results. Once someone edits the module directly they will have multiple problems

It will be difficult to start a new environment because direct changes have already been made to the module.
It is very difficult (I don't feel comfortable doing this, but in theory it is possible with terraform import) to transition that existing setup into an environment setup

Consider direct edits to be like distributing a python library and telling users to edit constants in the source code rather than using it as a library and making function calls.

Its a lot better to make a clean break from the past since we just created this and only have users using it in a testing phase right now.

gregwebs · 2019-07-10T20:50:05Z

Following on the above comment, the way to make it easy to use it to start with using as modules is to make an existing environment called "default" that is focused on ease of use rather than flexibility.

gregwebs · 2019-07-10T21:40:09Z

The default environment shouldn't need to be much more than

module "default" {
  source = "../"
}

aylei · 2019-07-11T03:46:33Z

@gregwebs Thanks for your thoughtful suggestions!

The reason why I keep the deploy/aws directory "as is" is to not break users.

And I think we have to reach a consensus about the purpose of the deploy/aws directory.

In this PR, I consider it as an example of how to compose the reusable modules located in the deploy/modules directory rather than a reusable module itself. Because It is hard to write a single module that provisions multiple tidb-clusters.

It is possible to write a reusable module that provisions VPC, bastion, key-pair and EKS, which effectively composes these four modules. This will reduce the code length to start a new environment, but I suppose composing these modules should require little effort just as the README stated.

So, the first problem you mentioned is now addressed: users don't call the deploy/aws module, they call the modules in the deploy/modules dir instead.

Another problem you mentioned is transition, actually they don't need, because the deploy/aws is already an individual environment. It is parallel to the dirs created by users, e.g. deploy/aws-staging, deploy/aws-production.

aylei · 2019-07-11T03:58:27Z

The default environment shouldn't need to be much more than
module "default" {
  source = "../"
}

IIUC, this example is a different UX for users. There are two UXs that may make sense:

provide a "big", highly customizable module, users call this module like the code example above;
provide several customizable small modules as shown in the PR, users call these modules like the code example in the README.md

I suppose both of them has pros and cons.

Comparing to UX-2, UX-1 has:

Pros:
- Easy to use
- Less error-prone because of proper encapsulation
Cons:
- Inflexible, all the customization possibilities should be exposed via module variables
- (things I concern) requires too much developing efforts than we can afford (especially, provisioning multiple TiDB clusters in a single module)

Actually I don't bias on either method technically, but I do prefer the UX-2 in favor of the overall ROI.

aylei · 2019-07-11T03:59:21Z

@tennix Could you please take a look?

aylei · 2019-07-11T04:03:34Z

However, I think putting this in a separate advanced section will end up with poor results. Once someone edits the module directly they will have multiple problems.

Yes, I agree we should emphasize this section for the users who need multiple environment management. I will try to refine this README

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei · 2019-07-11T04:11:47Z

Having said that, I will try to investigate whether I can build a monolithic reusable module in an acceptable amount of development

tennix · 2019-07-11T04:15:05Z

The quick-start guide should be easy enough, so users can really quick start. In most cases, this setup would be for testing. After they get a glimpse of what it looks like, they will try to deploy a production environment. And the quick-start setup may be wiped out. So I agree with @aylei to keep the quick-start as it is and document the best practice guide of how to manage production environment in a separate section.

gregwebs · 2019-07-12T01:59:18Z

okay, I get your point, and I agree with it. Things are well organized now.
But I would still move what is in deploy/aws into an environment directory. I think this update will already break existing users.

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei · 2019-07-14T15:38:45Z

@gregwebs I've addressed some merge conflicts, could you please take a look again?

And, I've investigate if we can approach to the structure you suggested, and I find that it is possible in the future! Once the hashicorp/terraform#17179 addressed, we can define multiple tidb clusters by a single map variable and things won't break when some of the clusters are removed (which is impossible now because currently the loop of resources is index based, which means we cannot remove element in the middle of the list).

aylei · 2019-07-14T15:39:31Z

@tennix PTAL

aylei · 2019-07-15T02:58:32Z

/run-e2e-tests

aylei · 2019-07-16T03:44:57Z

/run-e2e-tests

deploy/modules/aws/tidb-operator/README.md

tennix · 2019-07-16T09:54:12Z

deploy/modules/aws/tidb-cluster/variables.tf

+  default = "c5.2xlarge"
+}
+
+variable "override_values" {


Is this the path to values.yaml file? It's better add a description.

Okay, this is the content of values.yaml, it is recommend to provide the content by the file() function call from a local file

tennix · 2019-07-16T10:03:02Z

deploy/modules/share/tidb-cluster-release/data.tf

+data "external" "tidb_hostname" {
+  depends_on  = [helm_release.tidb-cluster]
+  working_dir = path.cwd
+  program     = ["bash", "-c", "kubectl --kubeconfig ${var.kubeconfig_filename} get svc -n ${var.cluster_name} ${var.cluster_name}-tidb -o json | jq '.status.loadBalancer.ingress[0]'"]


Can this read the right kubeconfig file when in a separated module?

Yes, the kubeconfig_filename should be the relative path to path.cwd by convention, and this is now guaranteed by all the modules we provide

deploy/modules/aws/tidb-cluster/cluster.tf

tennix · 2019-07-16T10:05:41Z

deploy/modules/aws/tidb-cluster/cluster.tf

+  tidb_count                 = var.tidb_count
+  tidb_cluster_chart_version = var.tidb_cluster_chart_version
+  override_values            = var.override_values
+  local_exec_interpreter     = var.local_exec_interpreter


Can this work if a user does not provide local_exec_interpreter?

Yes, there is a reasonable default ["bash", "-c"]

tennix · 2019-07-16T10:13:39Z

deploy/modules/aws/bastion/variables.tf

+
+variable "enable_ssh_to_workers" {
+  description = "Whether enable ssh from bastion to workers, if true, the worker_security_group_id must be provided"
+  default = false


Can we enable this by default?

No, the worker_security_group_id must be provided with this enabled, which can not have a default value. So user will be forced to provide the id or disable this explicitly if we enable this by default.

deploy/modules/aws/bastion/bastion.tf

deploy/.gitignore

Co-Authored-By: Tennix <tennix@users.noreply.github.com>

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei · 2019-07-16T11:11:14Z

@tennix I've addressed some of the review comments and add comments for others, PTAL again

aylei · 2019-07-16T11:54:53Z

/run-e2e-tests

tennix

LGTM

aylei · 2019-07-16T13:28:28Z

@gregwebs I've addressed some comments from tennix, could you please take a look again (again🤣)? Thanks!

onlymellb

LGTM

* Small fixes for terraform aws - fix hardcode chart version - fix unreachable bastion - adjust tidb-cluster no-wait to avoid scheduled backup pvc blocking - add version check - disable pd node public_ip Signed-off-by: Aylei <rayingecho@gmail.com> * Modularization for AWS terraform scripts Signed-off-by: Aylei <rayingecho@gmail.com> * Refine README of AWS terraform Signed-off-by: Aylei <rayingecho@gmail.com> * Fix bastion ssh Signed-off-by: Aylei <rayingecho@gmail.com> * Rephrase section title Signed-off-by: Aylei <rayingecho@gmail.com> * Update deploy/modules/aws/tidb-operator/README.md Co-Authored-By: Tennix <tennix@users.noreply.github.com> * Address review comments Signed-off-by: Aylei <rayingecho@gmail.com> (cherry picked from commit aa62176)

aylei added 2 commits July 10, 2019 02:24

Small fixes for terraform aws

ba9a71d

- fix hardcode chart version - fix unreachable bastion - adjust tidb-cluster no-wait to avoid scheduled backup pvc blocking - add version check - disable pd node public_ip Signed-off-by: Aylei <rayingecho@gmail.com>

Modularization for AWS terraform scripts

f05627c

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei requested a review from gregwebs July 10, 2019 17:14

aylei commented Jul 10, 2019

View reviewed changes

Refine README of AWS terraform

9cc13fb

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei requested a review from tennix July 11, 2019 03:59

Fix bastion ssh

05bac67

Signed-off-by: Aylei <rayingecho@gmail.com>

Merge remote-tracking branch 'upstream/master' into aws-modular

46c4cf5

gregwebs previously approved these changes Jul 12, 2019

View reviewed changes

aylei added 2 commits July 14, 2019 23:03

Merge upstream/master

32196cb

Rephrase section title

bae76bc

Signed-off-by: Aylei <rayingecho@gmail.com>

aylei dismissed gregwebs’s stale review via bae76bc July 14, 2019 15:32

aylei requested a review from gregwebs July 14, 2019 15:39

aylei mentioned this pull request Jul 14, 2019

Alibaba Cloud multiple tidb clusters management support #658

Merged

2 tasks

gregwebs previously approved these changes Jul 14, 2019

View reviewed changes

Merge branch 'master' into aws-modular

3ad7bd0

Merge branch 'master' into aws-modular

95ea107

tennix reviewed Jul 16, 2019

View reviewed changes

Update deploy/modules/aws/tidb-operator/README.md

f48daa1

Co-Authored-By: Tennix <tennix@users.noreply.github.com>

aylei dismissed gregwebs’s stale review via f48daa1 July 16, 2019 10:47

aylei added 2 commits July 16, 2019 19:07

Address review comments

9aef906

Signed-off-by: Aylei <rayingecho@gmail.com>

Merge remote

dc0e5ee

Signed-off-by: Aylei <rayingecho@gmail.com>

tennix approved these changes Jul 16, 2019

View reviewed changes

aylei requested review from gregwebs, tennix and onlymellb July 16, 2019 14:01

onlymellb approved these changes Jul 17, 2019

View reviewed changes

tennix merged commit aa62176 into pingcap:master Jul 17, 2019

This was referenced Jul 17, 2019

dev, 3.0: Update aws deployment document pingcap/docs#1348

Merged

dev, 3.0: Update aws deployment document pingcap/docs-cn#1639

Merged

aylei mentioned this pull request Aug 9, 2019

Separate aws/gcp/alibaba terraform scripts into infrastructure module and tidb-cluster module #494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularization for AWS terraform scripts #650

Modularization for AWS terraform scripts #650

aylei commented Jul 10, 2019 •

edited

Loading

aylei Jul 10, 2019

gregwebs commented Jul 10, 2019

gregwebs commented Jul 10, 2019

gregwebs commented Jul 10, 2019

gregwebs commented Jul 10, 2019

aylei commented Jul 11, 2019

aylei commented Jul 11, 2019 •

edited

Loading

aylei commented Jul 11, 2019

aylei commented Jul 11, 2019

aylei commented Jul 11, 2019

tennix commented Jul 11, 2019 •

edited

Loading

gregwebs commented Jul 12, 2019

aylei commented Jul 14, 2019

aylei commented Jul 14, 2019

aylei commented Jul 15, 2019

aylei commented Jul 16, 2019

tennix Jul 16, 2019

aylei Jul 16, 2019

tennix Jul 16, 2019

aylei Jul 16, 2019

tennix Jul 16, 2019

aylei Jul 16, 2019

tennix Jul 16, 2019

aylei Jul 16, 2019

aylei commented Jul 16, 2019

aylei commented Jul 16, 2019

tennix left a comment

aylei commented Jul 16, 2019

onlymellb left a comment

		@@ -1,5 +1,5 @@
		resource "null_resource" "wait-tiller-ready" {

Modularization for AWS terraform scripts #650

Modularization for AWS terraform scripts #650

Conversation

aylei commented Jul 10, 2019 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

Choose a reason for hiding this comment

gregwebs commented Jul 10, 2019

gregwebs commented Jul 10, 2019

gregwebs commented Jul 10, 2019

gregwebs commented Jul 10, 2019

aylei commented Jul 11, 2019

aylei commented Jul 11, 2019 • edited Loading

aylei commented Jul 11, 2019

aylei commented Jul 11, 2019

aylei commented Jul 11, 2019

tennix commented Jul 11, 2019 • edited Loading

gregwebs commented Jul 12, 2019

aylei commented Jul 14, 2019

aylei commented Jul 14, 2019

aylei commented Jul 15, 2019

aylei commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei commented Jul 16, 2019

aylei commented Jul 16, 2019

tennix left a comment

Choose a reason for hiding this comment

aylei commented Jul 16, 2019

onlymellb left a comment

Choose a reason for hiding this comment

aylei commented Jul 10, 2019 •

edited

Loading

aylei commented Jul 11, 2019 •

edited

Loading

tennix commented Jul 11, 2019 •

edited

Loading