Skip to content

Commit

Permalink
avoid unnecessary updates of discovery service and deployment (#1499) (…
Browse files Browse the repository at this point in the history 
…#1531)
  • Loading branch information
@sre-bot
sre-bot authored Jan 11, 2020
1 parent 3b259e6 commit e7138da
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 44 deletions.
14 changes: 0 additions & 14 deletions pkg/controller/equality.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
26 changes: 19 additions & 7 deletions pkg/controller/generic_control.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ package controller

import (
"context"
"encoding/json"
"fmt"
"strings"

Expand Down Expand Up @@ -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
}
Expand All @@ -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
})
Expand Down Expand Up @@ -245,22 +252,27 @@ 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)
if err != nil {
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
})
Expand Down
191 changes: 168 additions & 23 deletions pkg/controller/generic_control_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package controller

import (
"context"
"testing"

"github.com/pingcap/tidb-operator/pkg/scheme"
Expand All @@ -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)
Expand Down Expand Up @@ -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))
},
},
{
Expand Down Expand Up @@ -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))
},
},
{
Expand Down Expand Up @@ -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))
},
},
}
Expand All @@ -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{
{
Expand All @@ -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))
},
},
{
Expand Down Expand Up @@ -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))
},
},
}
Expand All @@ -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...)
}

0 comments on commit e7138da

Please sign in to comment.