[WIP] Optimization for CreateOrUpdateConfigMap #4170

mianhk · 2021-09-01T04:07:22Z

What problem does this PR solve?

Close #3737

What is changed and how does it work?

Code changes

Has Go code change
Has CI related scripts change

Tests

Unit test
E2E test
Manual test
No code

Side effects

Breaking backward compatibility
Other side effects:

Related changes

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

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.

Optimization for CreateOrUpdateConfigMap

ti-chi-bot · 2021-09-01T04:07:23Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

DanielZhangQD
july2993

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

DanielZhangQD · 2021-09-13T04:03:25Z

@mianhk Even with this PR, the function here https://github.com/pingcap/tidb-operator/blob/master/pkg/controller/generic_control.go#L425 will also call the Create API first, so it does not fix this issue thoroughly.

…UpdateConfigMap Conflicts: pkg/controller/generic_control.go

codecov-commenter · 2021-09-30T03:40:51Z

Codecov Report

Merging #4170 (5b0e271) into master (dc276de) will increase coverage by 0.01%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master    #4170      +/-   ##
==========================================
+ Coverage   61.65%   61.67%   +0.01%     
==========================================
  Files         181      181              
  Lines       19482    19520      +38     
==========================================
+ Hits        12011    12038      +27     
- Misses       6313     6315       +2     
- Partials     1158     1167       +9

Flag	Coverage Δ
unittest	`61.67% <70.00%> (+0.01%)`	⬆️

mianhk · 2021-09-30T03:58:37Z

@mianhk Even with this PR, the function here https://github.com/pingcap/tidb-operator/blob/master/pkg/controller/generic_control.go#L425 will also call the Create API first, so it does not fix this issue thoroughly.

I fixed this by CheckAndUpdateConfigMap and check the configMap by ConfigMapLister. And if is it necessary to create or upgrade other resources before after checking by Listers.

pkg/controller/generic_control.go

DanielZhangQD · 2021-10-12T08:55:28Z

/run-all-tests

DanielZhangQD · 2021-10-12T09:58:19Z

pkg/controller/generic_control.go

+	configMapNeedUpdate := false
+
+	if !configMapNotExist {
+		if err := mergeFn(cm, oldCm); err != nil {


It seems that mergeFn will never return err!=nil.
BTW, why set configMapNeedUpdate = true when err!=nil?

DanielZhangQD · 2021-10-12T10:04:26Z

pkg/controller/generic_control.go

+	}
+
+	if configMapNotExist || configMapNeedUpdate {
+		result, err := w.GenericControlInterface.CreateOrUpdate(controller, cm, mergeFn, true)


If you call CreateOrUpdate, it will always call create first before the update, which is a useless call to the kube-apiserver.
Could you please refer to the StatefulSet sync function https://github.com/pingcap/tidb-operator/blob/master/pkg/manager/member/tidb_member_manager.go#L177, just create if not exist and update if necessary?
Refer to https://github.com/pingcap/tidb-operator/blob/master/pkg/manager/member/config.go#L27 for configmap comparison. And you have to consider the components in the other files of this PR.

DanielZhangQD

Please check the comments.

pkg/controller/configmap_control.go

pkg/manager/member/utils.go

pkg/controller/dependences.go

pkg/manager/member/utils.go

pkg/manager/member/tikv_member_manager.go

DanielZhangQD · 2021-12-03T04:20:14Z

/run-all-tests

DanielZhangQD · 2021-12-03T07:10:21Z

pkg/manager/member/config.go

@@ -29,6 +31,8 @@ func updateConfigMap(old, new *corev1.ConfigMap) (bool, error) {

 	// check config
 	tomlField := []string{"config-file" /*pd,tikv,tidb */, "pump-config", "config_templ.toml" /*tiflash*/, "proxy_templ.toml" /*tiflash*/}
+	yamlField := []string{"prometheus-config", "prometheus.yml" /*prometheus*/}


prometheus-config is used as a key for external configmap that can be mounted for Prometheus which is not managed by TiDB Operator, so we can ignore this.
However, dashboards.yaml should be considered.

…' into optimize-CreateOrUpdateConfigMap

DanielZhangQD · 2021-12-06T02:26:57Z

/run-all-tests

DanielZhangQD · 2021-12-06T02:28:49Z

/merge

ti-chi-bot · 2021-12-06T02:28:52Z

This pull request has been accepted and is ready to merge.

Commit hash: 742d0e1

DanielZhangQD · 2021-12-06T13:45:30Z

/run-all-tests

DanielZhangQD · 2021-12-07T00:06:37Z

/run-all-tests

DanielZhangQD · 2021-12-07T01:45:47Z

/test pull-e2e-kind-br

DanielZhangQD · 2021-12-07T02:13:35Z

/test pull-e2e-kind

DanielZhangQD · 2021-12-07T06:29:58Z

/run-all-tests

DanielZhangQD · 2021-12-07T09:15:55Z

/test pull-e2e-kind

DanielZhangQD · 2021-12-07T10:27:22Z

/run-all-tests

DanielZhangQD · 2021-12-08T00:48:40Z

/run-all-tests

DanielZhangQD · 2021-12-08T07:19:20Z

/test pull-e2e-kind

DanielZhangQD · 2021-12-08T08:39:12Z

/test pull-e2e-kind

DanielZhangQD · 2021-12-08T12:10:30Z

/test pull-e2e-kind

DanielZhangQD · 2021-12-09T00:57:31Z

/test pull-e2e-kind

mianhk · 2021-12-10T02:14:09Z

Some e2e tests can't pass caused by the labelFilterKubeInformerFactory :

tidb-operator/pkg/controller/dependences.go

Line 336 in ff10892

    
           ConfigMapLister:             labelFilterKubeInformerFactory.Core().V1().ConfigMaps().Lister(),

This will always filter configmap with the label app.kubernetes.io/managed-by=tidb-operator.

But at the same time, some e2e tests install tidbcluster by Helm with chats:

tidb-operator/charts/tidb-cluster/templates/pump-configmap.yaml

Lines 7 to 12 in ff10892

    
           labels: 
        
             app.kubernetes.io/name: {{ template "chart.name" . }} 
        
             app.kubernetes.io/managed-by: {{ .Release.Service }} 
        
             app.kubernetes.io/instance: {{ .Release.Name }} 
        
             app.kubernetes.io/component: pump 
        
             helm.sh/chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+"  "_" }}

Which may create configmaps with the label app.kubernetes.io/managed-by=Helm which will not be cached by the informer.

There are three ways to fix this:

Reset tidbcluster's default labels to app.kubernetes.io/managed-by=tidb-operator in charts. Don't know if there are other effects.
Modify labelFilterKubeInformerFactory to kubeInformerFactory of ConfigMapLister. This will save all ConfigMap locally refer to [WIP] Optimization for CreateOrUpdateConfigMap #4170 (comment).

Replace the way to create tidbcluster by Helm to kubeCli, maybe we can do this in another PR, refer to:

tidb-operator/tests/e2e/tidbcluster/serial.go

Lines 159 to 162 in ff10892

    
           tc, err := cli.PingcapV1alpha1().TidbClusters(tc.Namespace).Create(context.TODO(), tc, metav1.CreateOptions{}) 
        
           framework.ExpectNoError(err, "failed to create TidbCluster: %v", tc) 
        
           err = oa.WaitForTidbClusterReady(tc, 30*time.Minute, 5*time.Second) 
        
           framework.ExpectNoError(err, "failed to wait for TidbCluster ready: %v", tc)

@DanielZhangQD PTAL, which way can be taken?

ti-chi-bot · 2021-12-13T11:48:45Z

@mianhk: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot · 2021-12-15T09:57:06Z

Merge canceled because a new commit is pushed.

Optimization for CreateOrUpdateConfigMap

9ba9166

ti-chi-bot requested review from DanielZhangQD and july2993 September 1, 2021 04:07

DanielZhangQD changed the title ~~Optimization for CreateOrUpdateConfigMap~~ [WIP] Optimization for CreateOrUpdateConfigMap Sep 24, 2021

mianhk added 2 commits September 29, 2021 22:13

fix: review suggest

c835711

Optimize: delete origin CreateOrUpdateConfigMap

d3ae640

ti-chi-bot added the needs-rebase label Sep 30, 2021

Merge remote-tracking branch 'upstream/master' into optimize-CreateOr…

3075100

…UpdateConfigMap Conflicts: pkg/controller/generic_control.go

ti-chi-bot removed the needs-rebase label Sep 30, 2021

mianhk changed the title ~~[WIP] Optimization for CreateOrUpdateConfigMap~~ Optimization for CreateOrUpdateConfigMap Sep 30, 2021

csuzhangxc reviewed Oct 8, 2021

View reviewed changes

pkg/controller/generic_control.go Outdated Show resolved Hide resolved

pkg/controller/generic_control.go Outdated Show resolved Hide resolved

mianhk added 2 commits October 8, 2021 19:13

fix: function name and configMap NotFound check

d3f5476

fix: function name and configMap NotFound check

d5f7622

csuzhangxc approved these changes Oct 9, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 label Oct 9, 2021

DanielZhangQD reviewed Oct 12, 2021

View reviewed changes

DanielZhangQD suggested changes Oct 12, 2021

View reviewed changes

ti-chi-bot removed the status/LGT1 label Oct 12, 2021

mianhk added 2 commits October 25, 2021 22:29

fix: always create before update

96639c0

fix: useless code

5e73f5a

csuzhangxc reviewed Oct 26, 2021

View reviewed changes

pkg/controller/configmap_control.go Outdated Show resolved Hide resolved

pkg/manager/member/utils.go Outdated Show resolved Hide resolved

pkg/manager/member/utils.go Outdated Show resolved Hide resolved

fix: debug code

f1b94bb

july2993 reviewed Nov 2, 2021

View reviewed changes

pkg/controller/dependences.go Outdated Show resolved Hide resolved

pkg/manager/member/utils.go Outdated Show resolved Hide resolved

pkg/manager/member/utils.go Outdated Show resolved Hide resolved

pkg/manager/member/tikv_member_manager.go Show resolved Hide resolved

fix: update labels and annotations

f6f39ae

Merge branch 'master' into optimize-CreateOrUpdateConfigMap

1dfae18

DanielZhangQD reviewed Dec 3, 2021

View reviewed changes

mianhk added 2 commits December 3, 2021 16:15

fix: configmap check dashboards.yaml

27a41a9

Merge remote-tracking branch 'origin/optimize-CreateOrUpdateConfigMap…

742d0e1

…' into optimize-CreateOrUpdateConfigMap

ti-chi-bot added the status/can-merge label Dec 6, 2021

fix: UpdateConfigMap setOwnerFlag

f236c8d

ti-chi-bot added the needs-rebase label Dec 13, 2021

mianhk force-pushed the optimize-CreateOrUpdateConfigMap branch from 14a2357 to f236c8d Compare December 15, 2021 09:57

ti-chi-bot removed the status/can-merge label Dec 15, 2021

mianhk mentioned this pull request Dec 22, 2021

[WIP]Deprecate unnecessary Helm Chart deployment #4319

Open

10 tasks

DanielZhangQD changed the title ~~Optimization for CreateOrUpdateConfigMap~~ [WIP] Optimization for CreateOrUpdateConfigMap Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Optimization for CreateOrUpdateConfigMap #4170

[WIP] Optimization for CreateOrUpdateConfigMap #4170

mianhk commented Sep 1, 2021

ti-chi-bot commented Sep 1, 2021 •

edited

Loading

DanielZhangQD commented Sep 13, 2021

codecov-commenter commented Sep 30, 2021 •

edited

Loading

mianhk commented Sep 30, 2021

DanielZhangQD commented Oct 12, 2021

DanielZhangQD Oct 12, 2021

DanielZhangQD Oct 12, 2021

DanielZhangQD left a comment

DanielZhangQD commented Dec 3, 2021

DanielZhangQD Dec 3, 2021

mianhk Dec 3, 2021

DanielZhangQD commented Dec 6, 2021

DanielZhangQD commented Dec 6, 2021

ti-chi-bot commented Dec 6, 2021

DanielZhangQD commented Dec 6, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 9, 2021

mianhk commented Dec 10, 2021

ti-chi-bot commented Dec 13, 2021

ti-chi-bot commented Dec 15, 2021

[WIP] Optimization for CreateOrUpdateConfigMap #4170

Are you sure you want to change the base?

[WIP] Optimization for CreateOrUpdateConfigMap #4170

Conversation

mianhk commented Sep 1, 2021

What problem does this PR solve?

What is changed and how does it work?

Code changes

Tests

Side effects

Related changes

Release Notes

ti-chi-bot commented Sep 1, 2021 • edited Loading

DanielZhangQD commented Sep 13, 2021

codecov-commenter commented Sep 30, 2021 • edited Loading

Codecov Report

mianhk commented Sep 30, 2021

DanielZhangQD commented Oct 12, 2021

DanielZhangQD Oct 12, 2021

Choose a reason for hiding this comment

DanielZhangQD Oct 12, 2021

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

DanielZhangQD commented Dec 3, 2021

DanielZhangQD Dec 3, 2021

Choose a reason for hiding this comment

mianhk Dec 3, 2021

Choose a reason for hiding this comment

DanielZhangQD commented Dec 6, 2021

DanielZhangQD commented Dec 6, 2021

ti-chi-bot commented Dec 6, 2021

DanielZhangQD commented Dec 6, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 7, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 8, 2021

DanielZhangQD commented Dec 9, 2021

mianhk commented Dec 10, 2021

ti-chi-bot commented Dec 13, 2021

ti-chi-bot commented Dec 15, 2021

ti-chi-bot commented Sep 1, 2021 •

edited

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading