support passing raw toml config for tiflash #3355

july2993 · 2020-10-12T04:15:17Z

What problem does this PR solve?

support passing raw toml config for tiflash

follow up #3327

What is changed and how does it work?

origin way to set config:

    tiflash:
      baseImage: pingcap/tiflash
      maxFailoverCount: 3
      replicas: 1
      storageClaims:
        - resources:
            requests:
              storage: 1Gi
--        # storageClassName: local-storage
      # requests:
      #   storage: "1Gi"
      config:
        config:
          logger:
            level: "warn"
            count: 9
        proxy:
          log-level: "warn"
          gc:
            batch-keys: 501

after this pr, support pass config like this:

    tiflash:
      baseImage: pingcap/tiflash
      maxFailoverCount: 3
      replicas: 1
      storageClaims:
        - resources:
            requests:
              storage: 1Gi
--        # storageClassName: local-storage
      # requests:
      #   storage: "1Gi"
      config:
        config: |
          [logger]
            level = "warn"
            count = 9
        proxy: |
          log-level = "warn"
          [gc]
            batch-keys = 501

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

Deploy cluster using the flowing yaml(one use raw, one use origin way),
Check pos normal and config in configmaps as expect

tidb-cluster.yaml CLICK ME

apiVersion: pingcap.com/v1alpha1
kind: TidbCluster
metadata:
  name: basic
spec:
  version: v4.0.6
  timezone: UTC
  pvReclaimPolicy: Delete
  discovery: {}
  configUpdateStrategy: RollingUpdate
  pd:
    baseImage: pingcap/pd
    replicas: 1
    # if storageClassName is not set, the default Storage Class of the Kubernetes cluster will be used
    # storageClassName: local-storage
    requests:
      storage: "1Gi"
    config: |
      [schedule]
        max-merge-region-keys = 22222
        max-merge-region-size = 22
  tikv:
    baseImage: pingcap/tikv
    replicas: 1
    # if storageClassName is not set, the default Storage Class of the Kubernetes cluster will be used
    # storageClassName: local-storage
    requests:
      storage: "1Gi"
    config: |
      [storage]
        # In basic examples, we set this to avoid using too much storage.
        reserve-space = "2MB"
  tidb:
    baseImage: pingcap/tidb
    replicas: 1
    service:
      type: ClusterIP
    config:
      token-limit: 203
      max-server-connections: 51
  pump:
    baseImage: pingcap/tidb-binlog
    replicas: 1
    # storageClassName: local-storage
    requests:
      storage: 20Gi
    # schedulerName: default-scheduler
    # config:
    #   gc: 7
    #   storage:
    #     kv:
    #       compaction-total-size-multiplier: 8.0
    config: |
      gc = 6
      [storage]
        [storage.kv]
          compaction-total-size-multiplier = 8.0


  tiflash:
    baseImage: pingcap/tiflash
    maxFailoverCount: 3
    replicas: 1
    storageClaims:
      - resources:
          requests:
            storage: 1Gi
        # storageClassName: local-storage
    # requests:
    #   storage: "1Gi"
    config:
      config: |
        [logger]
          level = "warn"
          count = 9
      proxy: |
        log-level = "warn"
        [gc]
          batch-keys = 501

tidb-cluster-ym.yamlCLICK ME

apiVersion: pingcap.com/v1alpha1
kind: TidbCluster
metadata:
  name: basic-ym
spec:
  version: v4.0.6
  timezone: UTC
  pvReclaimPolicy: Delete
  discovery: {}
  configUpdateStrategy: RollingUpdate
  pd:
    baseImage: pingcap/pd
    replicas: 1
    # if storageClassName is not set, the default Storage Class of the Kubernetes cluster will be used
    # storageClassName: local-storage
    requests:
      storage: "1Gi"
    config:
      schedule:
        max-merge-region-keys: 22222
        max-merge-region-size: 22
  tikv:
    baseImage: pingcap/tikv
    replicas: 1
    # if storageClassName is not set, the default Storage Class of the Kubernetes cluster will be used
    # storageClassName: local-storage
    requests:
      storage: "1Gi"
    config:
      storage:
        # In basic examples, we set this to avoid using too much storage.
        reserve-space: "2MB"
  tidb:
    baseImage: pingcap/tidb
    replicas: 1
    service:
      type: ClusterIP
    config:
      token-limit: 203
      max-server-connections: 51
  pump:
    baseImage: pingcap/tidb-binlog
    replicas: 1
    # storageClassName: local-storage
    requests:
      storage: 20Gi
    # schedulerName: default-scheduler
    config:
      gc: 6

  tiflash:
    baseImage: pingcap/tiflash
    maxFailoverCount: 3
    replicas: 1
    storageClaims:
      - resources:
          requests:
            storage: 1Gi
        # storageClassName: local-storage
    # requests:
    #   storage: "1Gi"
    config:
      config:
        logger:
          level: "warn"
          count: 9
      proxy:
        log-level: "warn"
        gc:
          batch-keys: 501

Related changes

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

Does this PR introduce a user-facing change?:

support passing raw toml config for tiflash

codecov-io · 2020-10-12T04:22:32Z

Codecov Report

Merging #3355 into master will decrease coverage by 0.38%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3355      +/-   ##
==========================================
- Coverage   50.28%   49.90%   -0.39%     
==========================================
  Files         163      163              
  Lines       16517    16408     -109     
==========================================
- Hits         8306     8188     -118     
+ Misses       7411     7410       -1     
- Partials      800      810      +10

Flag	Coverage Δ
#unittest	`49.90% <66.66%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

july2993 · 2020-10-12T06:25:20Z

pkg/manager/member/tiflash_util_test.go

-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			setTiFlashConfigDefault(&test.config, "", "test", "test")


Note the previous code for custom config case
config and expect both setted as customTiFlashLogConfig
and no deep copy(CommonConfing and ProxyConfig is the same object for config and expect)

so config will always equal expect after calling this.

pkg/apis/pingcap/v1alpha1/validation/validation.go

DanielZhangQD

LGTM

pkg/apis/pingcap/v1alpha1/validation/validation.go

july2993 · 2020-10-13T02:44:17Z

@DanielZhangQD @lonng PTAL
recheck&test and align code about update configmap in dff61e7

pkg/apis/pingcap/v1alpha1/validation/validation.go

pkg/manager/member/pump_member_manager.go

pkg/manager/member/tiflash_member_manager.go

pkg/manager/member/tikv_member_manager.go

DanielZhangQD

LGTM

july2993 · 2020-10-13T08:22:57Z

@lonng PTAL

lonng

LGTM

lonng · 2020-10-13T14:22:29Z

/merge

ti-srebot · 2020-10-13T14:22:48Z

@lonng Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1

lonng · 2020-10-14T00:32:56Z

/test pull-e2e-kind

DanielZhangQD · 2020-10-14T01:48:30Z

/test pull-e2e-kind

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-10-14T02:42:41Z

cherry pick to release-1.1 in PR #3368

follow up pingcap#3327 origin way to set config: ```yaml tiflash: baseImage: pingcap/tiflash maxFailoverCount: 3 replicas: 1 storageClaims: - resources: requests: storage: 1Gi -- # storageClassName: local-storage # requests: # storage: "1Gi" config: config: logger: level: "warn" count: 9 proxy: log-level: "warn" gc: batch-keys: 501 ``` after this pr, support pass config like this: ```yaml tiflash: baseImage: pingcap/tiflash maxFailoverCount: 3 replicas: 1 storageClaims: - resources: requests: storage: 1Gi -- # storageClassName: local-storage # requests: # storage: "1Gi" config: config: | [logger] level = "warn" count = 9 proxy: | log-level = "warn" [gc] batch-keys = 501 ```

support passing raw toml config for tiflash Co-authored-by: july2993 <july2993@gmail.com> Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>

follow up pingcap#3327 origin way to set config: ```yaml tiflash: baseImage: pingcap/tiflash maxFailoverCount: 3 replicas: 1 storageClaims: - resources: requests: storage: 1Gi -- # storageClassName: local-storage # requests: # storage: "1Gi" config: config: logger: level: "warn" count: 9 proxy: log-level: "warn" gc: batch-keys: 501 ``` after this pr, support pass config like this: ```yaml tiflash: baseImage: pingcap/tiflash maxFailoverCount: 3 replicas: 1 storageClaims: - resources: requests: storage: 1Gi -- # storageClassName: local-storage # requests: # storage: "1Gi" config: config: | [logger] level = "warn" count = 9 proxy: | log-level = "warn" [gc] batch-keys = 501 ```

july2993 commented Oct 12, 2020

View reviewed changes

july2993 force-pushed the toml_config_flash branch 2 times, most recently from 73b672d to 26eca0a Compare October 12, 2020 08:02

july2993 marked this pull request as ready for review October 12, 2020 08:02

july2993 added the status/PTAL PR needs to be reviewed label Oct 12, 2020

july2993 requested review from lonng and DanielZhangQD October 12, 2020 08:02

Support pass raw toml config for tiflash

5416168

july2993 force-pushed the toml_config_flash branch from 26eca0a to 5416168 Compare October 12, 2020 08:04

DanielZhangQD reviewed Oct 12, 2020

View reviewed changes

pkg/apis/pingcap/v1alpha1/validation/validation.go Show resolved Hide resolved

july2993 added 2 commits October 12, 2020 18:02

Add not-expect type validation

ae21405

Merge branch 'master' into toml_config_flash

4572e15

DanielZhangQD previously approved these changes Oct 12, 2020

View reviewed changes

ti-srebot added the status/LGT1 label Oct 12, 2020

lonng reviewed Oct 12, 2020

View reviewed changes

pkg/apis/pingcap/v1alpha1/validation/validation.go Outdated Show resolved Hide resolved

july2993 added 2 commits October 12, 2020 20:43

simplify validata

1d8257d

Align code style about update map

dff61e7

july2993 dismissed DanielZhangQD’s stale review via dff61e7 October 13, 2020 02:40

DanielZhangQD reviewed Oct 13, 2020

View reviewed changes

pkg/apis/pingcap/v1alpha1/validation/validation.go Outdated Show resolved Hide resolved

july2993 added 2 commits October 13, 2020 11:22

remove literal

11716e3

Merge branch 'master' into toml_config_flash

ba9b8ef

DanielZhangQD reviewed Oct 13, 2020

View reviewed changes

fix path

a46492a

july2993 requested review from DanielZhangQD and lonng October 13, 2020 06:13

DanielZhangQD approved these changes Oct 13, 2020

View reviewed changes

DanielZhangQD added the needs-cherry-pick-1.1 label Oct 13, 2020

lonng approved these changes Oct 13, 2020

View reviewed changes

Merge branch 'master' into toml_config_flash

74cc4f9

ti-srebot removed the status/LGT1 label Oct 13, 2020

ti-srebot approved these changes Oct 13, 2020

View reviewed changes

ti-srebot added the status/LGT2 label Oct 13, 2020

july2993 merged commit 6aa94f7 into pingcap:master Oct 14, 2020

july2993 deleted the toml_config_flash branch October 14, 2020 02:41

ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Oct 14, 2020

cherry pick pingcap#3355 to release-1.1

f57e305

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Oct 14, 2020

support passing raw toml config for tiflash (#3355) #3368

Merged

july2993 mentioned this pull request Oct 15, 2020

en, zh: Add docs about passing raw TOML config pingcap/docs-tidb-operator#765

Merged

7 tasks

ti-srebot mentioned this pull request Oct 15, 2020

en, zh: Add docs about passing raw TOML config (#765) pingcap/docs-tidb-operator#767

Merged

7 tasks

DanielZhangQD mentioned this pull request Oct 20, 2020

Some configuration parameters are missed in the TidbCluster CR #3212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support passing raw toml config for tiflash #3355

support passing raw toml config for tiflash #3355

july2993 commented Oct 12, 2020 •

edited

codecov-io commented Oct 12, 2020 •

edited

july2993 Oct 12, 2020

DanielZhangQD left a comment

july2993 commented Oct 13, 2020

DanielZhangQD left a comment

july2993 commented Oct 13, 2020

lonng left a comment

lonng commented Oct 13, 2020

ti-srebot commented Oct 13, 2020

lonng commented Oct 14, 2020

DanielZhangQD commented Oct 14, 2020

ti-srebot commented Oct 14, 2020

support passing raw toml config for tiflash #3355

support passing raw toml config for tiflash #3355

Conversation

july2993 commented Oct 12, 2020 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

codecov-io commented Oct 12, 2020 • edited

Codecov Report

july2993 Oct 12, 2020

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

july2993 commented Oct 13, 2020

DanielZhangQD left a comment

Choose a reason for hiding this comment

july2993 commented Oct 13, 2020

lonng left a comment

Choose a reason for hiding this comment

lonng commented Oct 13, 2020

ti-srebot commented Oct 13, 2020

lonng commented Oct 14, 2020

DanielZhangQD commented Oct 14, 2020

ti-srebot commented Oct 14, 2020

july2993 commented Oct 12, 2020 •

edited

codecov-io commented Oct 12, 2020 •

edited