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

change the way to config tidb/tikv/pd in charts #638

Merged
merged 14 commits into from
Jul 17, 2019

Conversation

DanielZhangQD
Copy link
Contributor

@DanielZhangQD DanielZhangQD commented Jul 5, 2019

What problem does this PR solve?

Do not provide config parameters for tidb/tikv/pd in values.yaml of charts any more to avoid the config mismatch between different releases of tidb/tikv/pd and also tidb-operator does not need to maintain the config templates for tidb/tikv/pd any more.
Instead, users should follow the config template in tidb/tikv/pd repo and provide customized configs by themselves.

What is changed and how does it work?

Update the way to config tidb/tikv/pd in values.yaml of chart.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    • Install TiDB cluster with new values.yaml and cluster runs normally, TiDB service is available to access.

Code changes

  • Has Helm charts change
  • Has documents change

Side effects

  • Breaking backward compatibility
    • Users should migrate the configs in values.yaml of previous chart releases to the new values.yaml of the new chart, otherwise, the configurations will be ignored when upgrading TiDB cluster with the new chart.

Related changes

  • Need to update the documentation

Does this PR introduce a user-facing change?:

Action required: Users should migrate the configs in values.yaml of previous chart releases to the new values.yaml of the new chart, otherwise, the configurations will be ignored when upgrading TiDB cluster with the new chart.
For example, configs in old values.yaml:
pd:
 logLevel: info
 maxStoreDownTime: 30m
 maxReplicas: 3
tikv:
 logLevel: info
 syncLog: true
 readpoolStorageConcurrency: 4
 readpoolCoprocessorConcurrency: 8
 storageSchedulerWorkerPoolSize: 4
tidb:
 logLevel: info
 preparedPlanCacheEnabled: false
 preparedPlanCacheCapacity: 100
 txnLocalLatchesEnabled: false
 txnLocalLatchesCapacity: "10240000"
 tokenLimit: "1000"
 memQuotaQuery: "34359738368"
 txnEntryCountLimit: "300000"
 txnTotalSizeLimit: "104857600"
 checkMb4ValueInUtf8: true
 treatOldVersionUtf8AsUtf8mb4: true
 lease: 45s
 maxProcs: 0

After migration, new values.yaml should be as below:
pd:
 config: |
   [log]
   level = "info"
   [schedule]
   max-store-down-time = "30m"
   [replication]
   max-replicas = 3
tikv:
 config: |
   log-level = "info"
   [raftstore]
   sync-log = true
   [readpool.storage]
   high-concurrency = 4
   normal-concurrency = 4
   low-concurrency = 4
   [readpool.coprocessor]
   high-concurrency = 8
   normal-concurrency = 8
   low-concurrency = 8
   [storage]
   scheduler-worker-pool-size = 4
tidb:
 config: |
   token-limit = 1000
   mem-quota-query = 34359738368
   check-mb4-value-in-utf8 = true
   treat-old-version-utf8-as-utf8mb4 = true
   lease = "45s"
   [log]
   level = "info"
   [prepared-plan-cache]
   enabled = false
   capacity = 100
   [txn-local-latches]
   enabled = false
   capacity = 10240000
   [performance]
   txn-entry-count-limit = 300000
   txn-total-size-limit = 104857600
   max-procs = 0

Please check [cluster configuration](https://pingcap.com/docs-cn/v3.0/reference/configuration/tidb-in-kubernetes/cluster-configuration/) for detailed configuration.

@gregwebs
Copy link
Contributor

gregwebs commented Jul 5, 2019

I like this. What about _pd-config.tpl, etc?

@DanielZhangQD
Copy link
Contributor Author

I like this. What about _pd-config.tpl, etc?

Actually, we can remove the config.tpl for tidb/tikv/pd in charts, but for now, I prefer to keep them as they are just in case, but no need to maintain or update them anymore.

@DanielZhangQD
Copy link
Contributor Author

DanielZhangQD commented Jul 8, 2019

@aylei Since GCP and Aliyun are under development, I just updated the config for AWS, please sync the change for GCP and Aliyun later.

@gregwebs
Copy link
Contributor

gregwebs commented Jul 8, 2019

@DanielZhangQD it is really confusing to have config.tpl if they don't do anything. I don't know what the current behavior is when both are present from looking at this PR.

@DanielZhangQD
Copy link
Contributor Author

DanielZhangQD commented Jul 9, 2019

@DanielZhangQD it is really confusing to have config.tpl if they don't do anything. I don't know what the current behavior is when both are present from looking at this PR.
@gregwebs
It's in the _helper.tpl:
https://github.com/pingcap/tidb-operator/blob/master/charts/tidb-cluster/templates/_helpers.tpl#L33
It will first check if Values.pd.config is configured, if yes, take it as configmap data, else, take the file config/_pd-config.tpl.

Discussed with @tennix @weekface @xiaojingchen, *-config.tpl and the related code will be remoted.

xiaojingchen
xiaojingchen previously approved these changes Jul 10, 2019
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@weekface weekface added this to the v1.0.0 milestone Jul 15, 2019
aylei
aylei previously approved these changes Jul 15, 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

@DanielZhangQD
Copy link
Contributor Author

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

1 similar comment
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

1 similar comment
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

weekface
weekface previously approved these changes Jul 17, 2019
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

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

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@weekface weekface merged commit 9252057 into pingcap:master Jul 17, 2019
@DanielZhangQD DanielZhangQD deleted the config-change branch July 18, 2019 07:39
weekface pushed a commit that referenced this pull request Jul 29, 2019
yahonda pushed a commit that referenced this pull request Dec 27, 2021
* add single page to describe enterprise edition deployment

* add more descriptions

* address comments

* address comments

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/doc enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants