From e7138dac9c620d6b42cf3577b702982a58cb2153 Mon Sep 17 00:00:00 2001 From: pingcap-github-bot Date: Sat, 11 Jan 2020 12:21:42 +0800 Subject: [PATCH] avoid unnecessary updates of discovery service and deployment (#1499) (#1531) --- pkg/controller/equality.go | 14 -- pkg/controller/generic_control.go | 26 +++- pkg/controller/generic_control_test.go | 191 ++++++++++++++++++++++--- 3 files changed, 187 insertions(+), 44 deletions(-) diff --git a/pkg/controller/equality.go b/pkg/controller/equality.go index ce42aec9b0..4c3d5ee17e 100644 --- a/pkg/controller/equality.go +++ b/pkg/controller/equality.go @@ -31,20 +31,6 @@ const ( LastAppliedConfigAnnotation = "pingcap.com/last-applied-configuration" ) -// SetDeploymentLastAppliedPodTemplate set last pod template to Deployment's annotation -func SetDeploymentLastAppliedPodTemplate(dep *appsv1.Deployment) error { - b, err := json.Marshal(dep.Spec.Template.Spec) - if err != nil { - return err - } - applied := string(b) - if dep.Annotations == nil { - dep.Annotations = map[string]string{} - } - dep.Annotations[LastAppliedPodTemplate] = applied - return nil -} - // GetDeploymentLastAppliedPodTemplate set last applied pod template from Deployment's annotation func GetDeploymentLastAppliedPodTemplate(dep *appsv1.Deployment) (*corev1.PodSpec, error) { applied, ok := dep.Annotations[LastAppliedPodTemplate] diff --git a/pkg/controller/generic_control.go b/pkg/controller/generic_control.go index 1cf8f1f5e3..01820e9eff 100644 --- a/pkg/controller/generic_control.go +++ b/pkg/controller/generic_control.go @@ -15,6 +15,7 @@ package controller import ( "context" + "encoding/json" "fmt" "strings" @@ -147,6 +148,10 @@ func (w *typedWrapper) CreateOrUpdateDeployment(controller runtime.Object, deplo existingDep.Spec.Replicas = desiredDep.Spec.Replicas existingDep.Labels = desiredDep.Labels + + if existingDep.Annotations == nil { + existingDep.Annotations = map[string]string{} + } for k, v := range desiredDep.Annotations { existingDep.Annotations[k] = v } @@ -163,11 +168,13 @@ func (w *typedWrapper) CreateOrUpdateDeployment(controller runtime.Object, deplo } // podSpec of deployment is hard to merge, use an annotation to assist if DeploymentPodSpecChanged(desiredDep, existingDep) { - existingDep.Spec.Template.Spec = desiredDep.Spec.Template.Spec - err := SetDeploymentLastAppliedPodTemplate(existingDep) + // Record last applied spec in favor of future equality check + b, err := json.Marshal(desiredDep.Spec.Template.Spec) if err != nil { return err } + existingDep.Annotations[LastAppliedConfigAnnotation] = string(b) + existingDep.Spec.Template.Spec = desiredDep.Spec.Template.Spec } return nil }) @@ -245,8 +252,11 @@ func (w *typedWrapper) CreateOrUpdateService(controller runtime.Object, svc *cor existingSvc := existing.(*corev1.Service) desiredSvc := desired.(*corev1.Service) + if existingSvc.Annotations == nil { + existingSvc.Annotations = map[string]string{} + } for k, v := range desiredSvc.Annotations { - desiredSvc.Annotations[k] = v + existingSvc.Annotations[k] = v } existingSvc.Labels = desiredSvc.Labels equal, err := ServiceEqual(desiredSvc, existingSvc) @@ -254,13 +264,15 @@ func (w *typedWrapper) CreateOrUpdateService(controller runtime.Object, svc *cor return err } if !equal { - clusterIp := existingSvc.Spec.ClusterIP - existingSvc.Spec = desiredSvc.Spec - existingSvc.Spec.ClusterIP = clusterIp - err := SetServiceLastAppliedConfigAnnotation(existingSvc) + // record desiredSvc Spec in annotations in favor of future equality checks + b, err := json.Marshal(desiredSvc.Spec) if err != nil { return err } + existingSvc.Annotations[LastAppliedConfigAnnotation] = string(b) + clusterIp := existingSvc.Spec.ClusterIP + existingSvc.Spec = desiredSvc.Spec + existingSvc.Spec.ClusterIP = clusterIp } return nil }) diff --git a/pkg/controller/generic_control_test.go b/pkg/controller/generic_control_test.go index d0525c8e5d..b199e8c53e 100644 --- a/pkg/controller/generic_control_test.go +++ b/pkg/controller/generic_control_test.go @@ -14,6 +14,7 @@ package controller import ( + "context" "testing" "github.com/pingcap/tidb-operator/pkg/scheme" @@ -37,22 +38,25 @@ func TestGenericControlInterface_CreateOrUpdate(t *testing.T) { existing *appsv1.Deployment desired *appsv1.Deployment mergeFn MergeFn - expectFn func(*GomegaWithT, client.Client, *appsv1.Deployment, error) + expectFn func(*GomegaWithT, *FakeClientWithTracker, *appsv1.Deployment, error) } testFn := func(tt *testCase) { t.Log(tt.name) var c client.Client - if tt.existing != nil { - c = fake.NewFakeClientWithScheme(scheme.Scheme, tt.existing) - } else { - c = fake.NewFakeClientWithScheme(scheme.Scheme) - } + c = fake.NewFakeClientWithScheme(scheme.Scheme) + withTracker := NewFakeClientWithTracker(c) recorder := record.NewFakeRecorder(10) - control := NewRealGenericControl(c, recorder) + control := NewRealGenericControl(withTracker, recorder) controller := newTidbCluster() + if tt.existing != nil { + _, err := control.CreateOrUpdate(controller, tt.existing, tt.mergeFn) + g.Expect(err).To(Succeed()) + } + withTracker.UpdateTracker.SetRequests(0) + withTracker.CreateTracker.SetRequests(0) result, err := control.CreateOrUpdate(controller, tt.desired, tt.mergeFn) - tt.expectFn(g, c, result.(*appsv1.Deployment), err) + tt.expectFn(g, withTracker, result.(*appsv1.Deployment), err) } mergeFn := func(existing, desired runtime.Object) error { e := existing.(*appsv1.Deployment) @@ -80,9 +84,11 @@ func TestGenericControlInterface_CreateOrUpdate(t *testing.T) { }, }, mergeFn: mergeFn, - expectFn: func(g *GomegaWithT, c client.Client, result *appsv1.Deployment, err error) { + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, result *appsv1.Deployment, err error) { g.Expect(err).To(Succeed()) g.Expect(result.Spec.Replicas).To(Equal(Int32Ptr(1))) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(0)) }, }, { @@ -116,10 +122,12 @@ func TestGenericControlInterface_CreateOrUpdate(t *testing.T) { }, }, mergeFn: mergeFn, - expectFn: func(g *GomegaWithT, c client.Client, result *appsv1.Deployment, err error) { + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, result *appsv1.Deployment, err error) { g.Expect(err).To(Succeed()) g.Expect(result.Spec.Replicas).To(Equal(Int32Ptr(2))) g.Expect(result.Spec.Template.Spec.DNSPolicy).To(Equal(corev1.DNSClusterFirstWithHostNet)) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(1)) }, }, { @@ -155,11 +163,13 @@ func TestGenericControlInterface_CreateOrUpdate(t *testing.T) { }, }, mergeFn: mergeFn, - expectFn: func(g *GomegaWithT, c client.Client, result *appsv1.Deployment, err error) { + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, result *appsv1.Deployment, err error) { g.Expect(err).To(Succeed()) g.Expect(result.Spec.Replicas).To(Equal(Int32Ptr(2))) g.Expect(result.Spec.Template.Spec.DNSPolicy).To(Equal(corev1.DNSClusterFirstWithHostNet)) g.Expect(result.Spec.Paused).To(BeTrue()) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(1)) }, }, } @@ -174,25 +184,55 @@ func TestCreateOrUpdateDeployment(t *testing.T) { type testCase struct { name string + initial *appsv1.Deployment existing *appsv1.Deployment desired *appsv1.Deployment - expectFn func(*GomegaWithT, client.Client, *appsv1.Deployment, error) + expectFn func(*GomegaWithT, *FakeClientWithTracker, *appsv1.Deployment, error) } testFn := func(tt *testCase) { t.Log(tt.name) var c client.Client - if tt.existing != nil { - c = fake.NewFakeClientWithScheme(scheme.Scheme, tt.existing) - } else { - c = fake.NewFakeClientWithScheme(scheme.Scheme) - } + c = fake.NewFakeClientWithScheme(scheme.Scheme) + withTracker := NewFakeClientWithTracker(c) recorder := record.NewFakeRecorder(10) - control := NewRealGenericControl(c, recorder) + control := NewRealGenericControl(withTracker, recorder) typed := NewTypedControl(control) controller := newTidbCluster() + if tt.initial != nil { + err := typed.Create(controller, tt.initial) + g.Expect(err).To(Succeed()) + } + if tt.existing != nil { + _, err := typed.CreateOrUpdateDeployment(controller, tt.existing) + g.Expect(err).To(Succeed()) + } + withTracker.UpdateTracker.SetRequests(0) + withTracker.CreateTracker.SetRequests(0) result, createErr := typed.CreateOrUpdateDeployment(controller, tt.desired) - tt.expectFn(g, c, result, createErr) + tt.expectFn(g, withTracker, result, createErr) + } + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{ + "k": "v", + }}, + Replicas: Int32Ptr(1), + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "k": "v", + }, + }, + Spec: corev1.PodSpec{ + DNSPolicy: corev1.DNSClusterFirst, + }, + }, + }, } cases := []*testCase{ { @@ -212,10 +252,12 @@ func TestCreateOrUpdateDeployment(t *testing.T) { }, }, }, - expectFn: func(g *GomegaWithT, c client.Client, result *appsv1.Deployment, err error) { + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, result *appsv1.Deployment, err error) { g.Expect(err).To(Succeed()) - g.Eventually(result.OwnerReferences).Should(HaveLen(1)) - g.Eventually(result.OwnerReferences[0].Kind).Should(Equal("TidbCluster")) + g.Expect(result.OwnerReferences).Should(HaveLen(1)) + g.Expect(result.OwnerReferences[0].Kind).Should(Equal("TidbCluster")) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(0)) }, }, { @@ -264,12 +306,25 @@ func TestCreateOrUpdateDeployment(t *testing.T) { }, }, }, - expectFn: func(g *GomegaWithT, c client.Client, result *appsv1.Deployment, err error) { + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, result *appsv1.Deployment, err error) { g.Expect(err).To(Succeed()) g.Expect(result.Spec.Replicas).To(Equal(Int32Ptr(2))) g.Expect(result.Spec.Template.Spec.DNSPolicy).To(Equal(corev1.DNSClusterFirstWithHostNet)) g.Expect(result.Spec.Selector.MatchLabels["k"]).To(Equal("v")) g.Expect(result.Spec.Template.Labels["k"]).To(Equal("v")) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(1)) + }, + }, + { + name: "CreateOrUpdate same desired state twice will skip real updating", + initial: dep, + existing: dep, + desired: dep, + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, result *appsv1.Deployment, err error) { + g.Expect(err).To(Succeed()) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(0)) }, }, } @@ -278,3 +333,93 @@ func TestCreateOrUpdateDeployment(t *testing.T) { testFn(tt) } } + +func TestCreateOrUpdateService(t *testing.T) { + g := NewGomegaWithT(t) + + type testCase struct { + name string + initial *corev1.Service + existing *corev1.Service + desired *corev1.Service + expectFn func(*GomegaWithT, *FakeClientWithTracker, error) + } + testFn := func(tt *testCase) { + t.Log(tt.name) + + var c client.Client + c = fake.NewFakeClientWithScheme(scheme.Scheme) + withTracker := NewFakeClientWithTracker(c) + recorder := record.NewFakeRecorder(10) + control := NewRealGenericControl(withTracker, recorder) + typed := NewTypedControl(control) + controller := newTidbCluster() + if tt.initial != nil { + err := typed.Create(controller, tt.initial) + g.Expect(err).To(Succeed()) + } + if tt.existing != nil { + _, err := typed.CreateOrUpdateService(controller, tt.existing) + g.Expect(err).To(Succeed()) + } + withTracker.UpdateTracker.SetRequests(0) + withTracker.CreateTracker.SetRequests(0) + _, createErr := typed.CreateOrUpdateService(controller, tt.desired) + tt.expectFn(g, withTracker, createErr) + } + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "k": "v", + }, + Ports: []corev1.ServicePort{{ + Port: 9090, + }}, + }, + } + cases := []*testCase{ + { + name: "CreateOrUpdate same desired state twice will skip real updating", + initial: svc, + existing: svc, + desired: svc, + expectFn: func(g *GomegaWithT, c *FakeClientWithTracker, err error) { + g.Expect(err).To(Succeed()) + g.Expect(c.CreateTracker.GetRequests()).To(Equal(1)) + g.Expect(c.UpdateTracker.GetRequests()).To(Equal(0)) + }, + }, + } + + for _, tt := range cases { + testFn(tt) + } +} + +type FakeClientWithTracker struct { + client.Client + CreateTracker RequestTracker + UpdateTracker RequestTracker +} + +func NewFakeClientWithTracker(cli client.Client) *FakeClientWithTracker { + return &FakeClientWithTracker{ + Client: cli, + UpdateTracker: RequestTracker{}, + CreateTracker: RequestTracker{}, + } +} + +func (c *FakeClientWithTracker) Create(ctx context.Context, obj runtime.Object, opts ...client.CreateOption) error { + c.CreateTracker.Inc() + return c.Client.Create(ctx, obj, opts...) +} + +func (c *FakeClientWithTracker) Update(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error { + c.UpdateTracker.Inc() + return c.Client.Update(ctx, obj, opts...) +}