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

[WIP] Optimization for CreateOrUpdateConfigMap #4170

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9ba9166
Optimization for CreateOrUpdateConfigMap
mianhk Aug 31, 2021
c835711
fix: review suggest
mianhk Sep 29, 2021
d3ae640
Optimize: delete origin CreateOrUpdateConfigMap
mianhk Sep 30, 2021
3075100
Merge remote-tracking branch 'upstream/master' into optimize-CreateOr…
mianhk Sep 30, 2021
d3f5476
fix: function name and configMap NotFound check
mianhk Oct 8, 2021
d5f7622
fix: function name and configMap NotFound check
mianhk Oct 8, 2021
96639c0
fix: always create before update
mianhk Oct 25, 2021
5e73f5a
fix: useless code
mianhk Oct 25, 2021
f1b94bb
fix: debug code
mianhk Oct 26, 2021
f6f39ae
fix: update labels and annotations
mianhk Nov 2, 2021
8a013d9
fix: labelFilterKubeInformerFactory of test cases
mianhk Nov 3, 2021
e1de20a
optimized code
mianhk Nov 3, 2021
0f3fa52
fix : logic
mianhk Nov 4, 2021
df12f46
optimize blank line
mianhk Nov 9, 2021
5c00995
Merge branch 'master' into optimize-CreateOrUpdateConfigMap
DanielZhangQD Nov 11, 2021
786ee93
Merge branch 'master' into optimize-CreateOrUpdateConfigMap
ti-chi-bot Nov 12, 2021
8091d54
Merge branch 'master' into optimize-CreateOrUpdateConfigMap
ti-chi-bot Nov 12, 2021
8506b57
Merge branch 'master' into optimize-CreateOrUpdateConfigMap
ti-chi-bot Nov 16, 2021
f5f583b
Merge branch 'master' into optimize-CreateOrUpdateConfigMap
ti-chi-bot Nov 17, 2021
0d4fc84
fix: e2e test error
mianhk Dec 1, 2021
ed1331f
Merge remote-tracking branch 'origin/optimize-CreateOrUpdateConfigMap…
mianhk Dec 1, 2021
5a3e370
Merge remote-tracking branch 'upstream/master' into optimize-CreateOr…
mianhk Dec 1, 2021
1dfae18
Merge branch 'master' into optimize-CreateOrUpdateConfigMap
DanielZhangQD Dec 3, 2021
27a41a9
fix: configmap check dashboards.yaml
mianhk Dec 3, 2021
742d0e1
Merge remote-tracking branch 'origin/optimize-CreateOrUpdateConfigMap…
mianhk Dec 3, 2021
f236c8d
fix: UpdateConfigMap setOwnerFlag
mianhk Dec 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/controller/dependences.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,12 +395,12 @@ func NewDependencies(ns string, cliCfg *CLIConfig, clientset versioned.Interface
return deps, nil
}

func newFakeControl(kubeClientset kubernetes.Interface, informerFactory informers.SharedInformerFactory, kubeInformerFactory kubeinformers.SharedInformerFactory) Controls {
func newFakeControl(kubeClientset kubernetes.Interface, informerFactory informers.SharedInformerFactory, kubeInformerFactory kubeinformers.SharedInformerFactory, labelFilterKubeInformerFactory kubeinformers.SharedInformerFactory) Controls {
genericCtrl := NewFakeGenericControl()
// Shared variables to construct `Dependencies` and some of its fields
return Controls{
JobControl: NewFakeJobControl(kubeInformerFactory.Batch().V1().Jobs()),
ConfigMapControl: NewFakeConfigMapControl(kubeInformerFactory.Core().V1().ConfigMaps()),
ConfigMapControl: NewFakeConfigMapControl(labelFilterKubeInformerFactory.Core().V1().ConfigMaps()),
StatefulSetControl: NewFakeStatefulSetControl(kubeInformerFactory.Apps().V1().StatefulSets()),
ServiceControl: NewFakeServiceControl(kubeInformerFactory.Core().V1().Services(), kubeInformerFactory.Core().V1().Endpoints()),
PVControl: NewFakePVControl(kubeInformerFactory.Core().V1().PersistentVolumes(), kubeInformerFactory.Core().V1().PersistentVolumeClaims()),
Expand Down Expand Up @@ -454,6 +454,6 @@ func NewFakeDependencies() *Dependencies {
if err != nil {
klog.Fatalf("failed to create Dependencies: %s", err)
}
deps.Controls = newFakeControl(kubeCli, informerFactory, kubeInformerFactory)
deps.Controls = newFakeControl(kubeCli, informerFactory, kubeInformerFactory, labelFilterKubeInformerFactory)
return deps
}
32 changes: 32 additions & 0 deletions pkg/manager/member/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ package member

import (
"fmt"
"reflect"

perrors "github.com/pingcap/errors"
"github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1"
"github.com/pingcap/tidb-operator/pkg/apis/util/toml"
"gopkg.in/yaml.v2"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
corelisters "k8s.io/client-go/listers/core/v1"
Expand All @@ -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*/}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed.


for _, k := range tomlField {
oldData, oldOK := old.Data[k]
newData, newOK := new.Data[k]
Expand All @@ -53,6 +57,34 @@ func updateConfigMap(old, new *corev1.ConfigMap) (bool, error) {
}
}

for _, k := range yamlField {
oldData, oldOK := old.Data[k]
newData, newOK := new.Data[k]

if oldOK != newOK {
dataEqual = false
}

if !oldOK || !newOK {
continue
}
m1 := map[string]interface{}{}
m2 := map[string]interface{}{}
err := yaml.Unmarshal([]byte(oldData), &m1)
if err != nil {
dataEqual = false
}
err = yaml.Unmarshal([]byte(newData), &m2)
if err != nil {
dataEqual = false
}
if reflect.DeepEqual(m1, m2) {
new.Data[k] = oldData
} else {
dataEqual = false
}
}

// check startup script
field := "startup-script"
oldScript, oldExist := old.Data[field]
Expand Down
16 changes: 16 additions & 0 deletions pkg/manager/member/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,22 @@ func TestUpdateConfigMap(t *testing.T) {
updateKeys: []string{"pump-config", "config_templ.toml"},
equal: false,
},
{
name: "add some yaml keys",
old: &corev1.ConfigMap{
Data: map[string]string{
"dashboard-config": "foo",
"prometheus-config": "a = \"b\"",
},
},
new: &corev1.ConfigMap{
Data: map[string]string{
"prometheus.yml": "# comment \nc = \"d\"",
},
},
updateKeys: []string{"prometheus.yml"},
equal: false,
},
{
name: "the data of old and new configmaps are the same",
old: &corev1.ConfigMap{
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/member/dm_master_member_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func (m *masterMemberManager) syncMasterConfigMap(dc *v1alpha1.DMCluster, set *a
if err != nil {
return nil, err
}
return m.deps.TypedControl.CreateOrUpdateConfigMap(dc, newCm)
return CreateOrUpdateConfigMap(m.deps.ConfigMapLister, m.deps.ConfigMapControl, newCm, dc)
}

func (m *masterMemberManager) getNewMasterServiceForDMCluster(dc *v1alpha1.DMCluster) *corev1.Service {
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/member/dm_worker_member_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ func (m *workerMemberManager) syncWorkerConfigMap(dc *v1alpha1.DMCluster, set *a
if err != nil {
return nil, err
}
return m.deps.TypedControl.CreateOrUpdateConfigMap(dc, newCm)
return CreateOrUpdateConfigMap(m.deps.ConfigMapLister, m.deps.ConfigMapControl, newCm, dc)
}

func getNewWorkerSetForDMCluster(dc *v1alpha1.DMCluster, cm *corev1.ConfigMap) (*apps.StatefulSet, error) {
Expand Down
48 changes: 23 additions & 25 deletions pkg/manager/member/dm_worker_member_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@
package member

import (
"context"
"fmt"
"strings"
"testing"

"github.com/pingcap/tidb-operator/pkg/apis/label"

"github.com/google/go-cmp/cmp"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/intstr"

. "github.com/onsi/gomega"
Expand All @@ -37,7 +37,6 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/cache"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func TestWorkerMemberManagerSyncCreate(t *testing.T) {
Expand Down Expand Up @@ -80,7 +79,7 @@ func TestWorkerMemberManagerSyncCreate(t *testing.T) {
ctls.svc.SetCreateServiceError(errors.NewInternalError(fmt.Errorf("API server failed")), 0)
}
if test.errOnCreateCm {
ctls.generic.SetCreateOrUpdateError(errors.NewInternalError(fmt.Errorf("API server failed")), 0)
ctls.cm.SetCreateConfigMapError(errors.NewInternalError(fmt.Errorf("API server failed")), 0)
}

syncErr := wmm.SyncDM(dc)
Expand All @@ -94,9 +93,7 @@ func TestWorkerMemberManagerSyncCreate(t *testing.T) {
cmName = cmGen.Name
g.Expect(strings.HasPrefix(cmName, controller.DMWorkerMemberName(dcName))).To(BeTrue())
}
cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: cmName}}
key := client.ObjectKeyFromObject(cm)
getCmErr := ctls.generic.FakeCli.Get(context.TODO(), key, cm)
cm, getCmErr := wmm.deps.ConfigMapLister.ConfigMaps(ns).Get(cmName)
result := result{syncErr, svc, getSvcErr, set, getStsErr, cm, getCmErr}
test.expectFn(g, &result)
}
Expand Down Expand Up @@ -228,7 +225,7 @@ func TestWorkerMemberManagerSyncUpdate(t *testing.T) {
ctls.svc.SetUpdateServiceError(errors.NewInternalError(fmt.Errorf("API server failed")), 0)
}
if test.errOnUpdateCm {
ctls.generic.SetCreateOrUpdateError(errors.NewInternalError(fmt.Errorf("API server failed")), 0)
ctls.cm.SetCreateConfigMapError(errors.NewInternalError(fmt.Errorf("API server failed")), 0)
}

oldCm, err := getWorkerConfigMap(dc)
Expand All @@ -240,8 +237,7 @@ func TestWorkerMemberManagerSyncUpdate(t *testing.T) {

g.Expect(indexers.set.Add(oldSet)).To(Succeed())
g.Expect(indexers.svc.Add(oldSvc)).To(Succeed())

g.Expect(ctls.generic.AddObject(oldCm)).To(Succeed())
g.Expect(indexers.cm.Add(oldCm)).To(Succeed())

if test.prepare != nil {
test.prepare(dc, indexers)
Expand All @@ -258,9 +254,10 @@ func TestWorkerMemberManagerSyncUpdate(t *testing.T) {
cmName = cmGen.Name
g.Expect(strings.HasPrefix(cmName, controller.DMWorkerMemberName(dcName))).To(BeTrue())
}
cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: cmName}}
key := client.ObjectKeyFromObject(cm)
getCmErr := ctls.generic.FakeCli.Get(context.TODO(), key, cm)
cm, getCmErr := mmm.deps.ConfigMapLister.ConfigMaps(ns).Get(cmName)
if getCmErr != nil {
cm = &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: cmName}}
}
result := result{syncErr, oldSvc, svc, getSvcErr, oldSet, set, getStsErr, oldCm, cm, getCmErr, triggerDeleteWorker}
test.expectFn(g, &result)
}
Expand Down Expand Up @@ -593,7 +590,7 @@ func TestWorkerSyncConfigUpdate(t *testing.T) {
set *appsv1.StatefulSet
getSet error
oldCm *corev1.ConfigMap
cms []corev1.ConfigMap
cms []*corev1.ConfigMap
listCm error
}
type testcase struct {
Expand All @@ -610,7 +607,7 @@ func TestWorkerSyncConfigUpdate(t *testing.T) {
ns := dc.Namespace
dcName := dc.Name

mmm, controls, indexers, fakeMasterControl := newFakeWorkerMemberManager()
mmm, _, indexers, fakeMasterControl := newFakeWorkerMemberManager()
masterClient := controller.NewFakeMasterClient(fakeMasterControl, dc)
masterClient.AddReaction(dmapi.GetWorkersActionType, func(action *dmapi.Action) (interface{}, error) {
return test.workerInfos, nil
Expand All @@ -625,18 +622,17 @@ func TestWorkerSyncConfigUpdate(t *testing.T) {

g.Expect(indexers.set.Add(oldSet)).To(Succeed())
g.Expect(indexers.svc.Add(oldSvc)).To(Succeed())
g.Expect(controls.generic.AddObject(oldCm)).To(Succeed())
g.Expect(indexers.cm.Add(oldCm)).To(Succeed())

if test.prepare != nil {
test.prepare(dc, indexers)
}

syncErr := mmm.SyncDM(dc)
set, getStsErr := mmm.deps.StatefulSetLister.StatefulSets(ns).Get(controller.DMWorkerMemberName(dcName))
cmList := &corev1.ConfigMapList{}
cmList, listCmErr := mmm.deps.ConfigMapLister.ConfigMaps(ns).List(labels.Everything())
g.Expect(err).To(Succeed())
listCmErr := controls.generic.FakeCli.List(context.TODO(), cmList)
result := result{syncErr, oldSet, set, getStsErr, oldCm, cmList.Items, listCmErr}
result := result{syncErr, oldSet, set, getStsErr, oldCm, cmList, listCmErr}
test.expectFn(g, &result)
}

Expand All @@ -662,7 +658,7 @@ func TestWorkerSyncConfigUpdate(t *testing.T) {
for i := range r.cms {
cm := r.cms[i]
if cm.Name == using {
usingCm = &cm
usingCm = cm
}
}
g.Expect(usingCm).NotTo(BeNil(), "The configmap used by statefulset must be created")
Expand All @@ -685,13 +681,14 @@ func TestWorkerSyncConfigUpdate(t *testing.T) {
type workerFakeIndexers struct {
svc cache.Indexer
set cache.Indexer
cm cache.Indexer
pod cache.Indexer
}

type workerFakeControls struct {
svc *controller.FakeServiceControl
set *controller.FakeStatefulSetControl
generic *controller.FakeGenericControl
svc *controller.FakeServiceControl
set *controller.FakeStatefulSetControl
cm *controller.FakeConfigMapControl
}

func newFakeWorkerMemberManager() (*workerMemberManager, *workerFakeControls, *workerFakeIndexers, *dmapi.FakeMasterControl) {
Expand All @@ -704,13 +701,14 @@ func newFakeWorkerMemberManager() (*workerMemberManager, *workerFakeControls, *w
failover: NewFakeWorkerFailover(),
}
controls := &workerFakeControls{
svc: fakeDeps.ServiceControl.(*controller.FakeServiceControl),
set: fakeDeps.StatefulSetControl.(*controller.FakeStatefulSetControl),
generic: fakeDeps.GenericControl.(*controller.FakeGenericControl),
svc: fakeDeps.ServiceControl.(*controller.FakeServiceControl),
set: fakeDeps.StatefulSetControl.(*controller.FakeStatefulSetControl),
cm: fakeDeps.ConfigMapControl.(*controller.FakeConfigMapControl),
}
indexers := &workerFakeIndexers{
svc: fakeDeps.KubeInformerFactory.Core().V1().Services().Informer().GetIndexer(),
set: fakeDeps.KubeInformerFactory.Apps().V1().StatefulSets().Informer().GetIndexer(),
cm: fakeDeps.LabelFilterKubeInformerFactory.Core().V1().ConfigMaps().Informer().GetIndexer(),
pod: fakeDeps.KubeInformerFactory.Core().V1().Pods().Informer().GetIndexer(),
}
return pmm, controls, indexers, masterControl
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/member/pd_member_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ func (m *pdMemberManager) syncPDConfigMap(tc *v1alpha1.TidbCluster, set *apps.St
if err != nil {
return nil, err
}
return m.deps.TypedControl.CreateOrUpdateConfigMap(tc, newCm)
return CreateOrUpdateConfigMap(m.deps.ConfigMapLister, m.deps.ConfigMapControl, newCm, tc)
}

func (m *pdMemberManager) getNewPDServiceForTidbCluster(tc *v1alpha1.TidbCluster) *corev1.Service {
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/member/pump_member_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ func (m *pumpMemberManager) syncConfigMap(tc *v1alpha1.TidbCluster, set *appsv1.
if err != nil {
return nil, err
}
return m.deps.TypedControl.CreateOrUpdateConfigMap(tc, newCm)
return CreateOrUpdateConfigMap(m.deps.ConfigMapLister, m.deps.ConfigMapControl, newCm, tc)
}

func getNewPumpHeadlessService(tc *v1alpha1.TidbCluster) *corev1.Service {
Expand Down
Loading