From ab1be9380922b64a82b2e2ac7ede265ebafb5578 Mon Sep 17 00:00:00 2001 From: waynepeking348 Date: Sun, 20 Dec 2020 13:37:00 +0800 Subject: [PATCH 1/3] clean rs by revision instead of creat timestamp --- pkg/controller/deployment/sync.go | 2 +- pkg/controller/deployment/util/deployment_util.go | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 3c7e808b24b..c1e3d18c05b 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -448,7 +448,7 @@ func (dc *DeploymentController) cleanupDeployment(oldRSs []*apps.ReplicaSet, dep return nil } - sort.Sort(controller.ReplicaSetsByCreationTimestamp(cleanableRSes)) + sort.Sort(deploymentutil.ReplicaSetsByRevision(cleanableRSes)) klog.V(4).Infof("Looking to cleanup old replica sets for deployment %q", deployment.Name) for i := int32(0); i < diff; i++ { diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index fb1af676d4c..be7d98789ab 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -949,3 +949,18 @@ func GetDeploymentsForReplicaSet(deploymentLister appslisters.DeploymentLister, return deployments, nil } + +// ReplicaSetsByRevision sorts a list of ReplicaSet by revision, using their creation timestamp or name as a tie breaker. +// By using the creation timestamp, this sorts from old to new replica sets. +type ReplicaSetsByRevision []*apps.ReplicaSet + +func (o ReplicaSetsByRevision) Len() int { return len(o) } +func (o ReplicaSetsByRevision) Swap(i, j int) { o[i], o[j] = o[j], o[i] } +func (o ReplicaSetsByRevision) Less(i, j int) bool { + revision1, err1 := Revision(o[i]) + revision2, err2 := Revision(o[j]) + if err1 != nil || err2 != nil || revision1 == revision2 { + return controller.ReplicaSetsByCreationTimestamp(o).Less(i, j) + } + return revision1 < revision2 +} From c8f98589208e8c5a0d9262ebd90985acc55d2d5a Mon Sep 17 00:00:00 2001 From: waynepeking348 Date: Sun, 20 Dec 2020 16:54:31 +0800 Subject: [PATCH 2/3] add unit test cases --- pkg/controller/deployment/sync_test.go | 144 +++++++++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/pkg/controller/deployment/sync_test.go b/pkg/controller/deployment/sync_test.go index 942b93aac2c..d441341b593 100644 --- a/pkg/controller/deployment/sync_test.go +++ b/pkg/controller/deployment/sync_test.go @@ -24,8 +24,10 @@ import ( apps "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" + clienttesting "k8s.io/client-go/testing" testclient "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/controller" @@ -446,3 +448,145 @@ func TestDeploymentController_cleanupDeployment(t *testing.T) { } } } + +func TestDeploymentController_cleanupDeploymentOrder(t *testing.T) { + selector := map[string]string{"foo": "bar"} + now := metav1.Now() + duration := time.Minute + + newRSWithRevisionAndCreationTimestamp := func(name string, replicas int, selector map[string]string, timestamp time.Time, revision string) *apps.ReplicaSet { + rs := rs(name, replicas, selector, metav1.NewTime(timestamp)) + if revision != "" { + rs.Annotations = map[string]string{ + deploymentutil.RevisionAnnotation: revision, + } + } + rs.Status = apps.ReplicaSetStatus{ + Replicas: int32(replicas), + } + return rs + } + + // for all test cases, creationTimestamp order keeps as: rs1 < rs2 < rs3 < r4 + tests := []struct { + oldRSs []*apps.ReplicaSet + revisionHistoryLimit int32 + expectedDeletedRSs sets.String + }{ + { + // revision order: rs1 < rs2, delete rs1 + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 0, selector, now.Add(-1*duration), "1"), + newRSWithRevisionAndCreationTimestamp("foo-2", 0, selector, now.Time, "2"), + }, + revisionHistoryLimit: 1, + expectedDeletedRSs: sets.NewString("foo-1"), + }, + { + // revision order: rs2 < rs1, delete rs2 + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 0, selector, now.Add(-1*duration), "2"), + newRSWithRevisionAndCreationTimestamp("foo-2", 0, selector, now.Time, "1"), + }, + revisionHistoryLimit: 1, + expectedDeletedRSs: sets.NewString("foo-2"), + }, + { + // rs1 has revision but rs2 doesn't have revision, delete rs2 + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 0, selector, now.Add(-1*duration), "1"), + newRSWithRevisionAndCreationTimestamp("foo-2", 0, selector, now.Time, ""), + }, + revisionHistoryLimit: 1, + expectedDeletedRSs: sets.NewString("foo-2"), + }, + { + // rs1 doesn't have revision while rs2 has revision, delete rs1 + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 0, selector, now.Add(-1*duration), ""), + newRSWithRevisionAndCreationTimestamp("foo-2", 0, selector, now.Time, "2"), + }, + revisionHistoryLimit: 1, + expectedDeletedRSs: sets.NewString("foo-1"), + }, + { + // revision order: rs1 < rs2 < r3, but rs1 has replicas, delete rs2 + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 1, selector, now.Add(-1*duration), "1"), + newRSWithRevisionAndCreationTimestamp("foo-2", 0, selector, now.Time, "2"), + newRSWithRevisionAndCreationTimestamp("foo-3", 0, selector, now.Add(duration), "3"), + }, + revisionHistoryLimit: 1, + expectedDeletedRSs: sets.NewString("foo-2"), + }, + { + // revision order: rs1 < rs2 < r3, both rs1 && rs2 have replicas, don't delete + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 1, selector, now.Add(-1*duration), "1"), + newRSWithRevisionAndCreationTimestamp("foo-2", 1, selector, now.Time, "2"), + newRSWithRevisionAndCreationTimestamp("foo-3", 0, selector, now.Add(duration), "3"), + }, + revisionHistoryLimit: 1, + expectedDeletedRSs: sets.NewString(), + }, + { + // revision order: rs2 < rs4 < rs1 < rs3, delete rs2 && rs4 + oldRSs: []*apps.ReplicaSet{ + newRSWithRevisionAndCreationTimestamp("foo-1", 0, selector, now.Add(-1*duration), "3"), + newRSWithRevisionAndCreationTimestamp("foo-2", 0, selector, now.Time, "1"), + newRSWithRevisionAndCreationTimestamp("foo-3", 0, selector, now.Add(duration), "4"), + newRSWithRevisionAndCreationTimestamp("foo-4", 0, selector, now.Add(2*duration), "2"), + }, + revisionHistoryLimit: 2, + expectedDeletedRSs: sets.NewString("foo-2", "foo-4"), + }, + } + + for i := range tests { + test := tests[i] + t.Logf("scenario %d", i) + + fake := &fake.Clientset{} + informers := informers.NewSharedInformerFactory(fake, controller.NoResyncPeriodFunc()) + controller, err := NewDeploymentController(informers.Apps().V1().Deployments(), informers.Apps().V1().ReplicaSets(), informers.Core().V1().Pods(), fake) + if err != nil { + t.Fatalf("error creating Deployment controller: %v", err) + } + + controller.eventRecorder = &record.FakeRecorder{} + controller.dListerSynced = alwaysReady + controller.rsListerSynced = alwaysReady + controller.podListerSynced = alwaysReady + for _, rs := range test.oldRSs { + informers.Apps().V1().ReplicaSets().Informer().GetIndexer().Add(rs) + } + + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + d := newDeployment("foo", 1, &test.revisionHistoryLimit, nil, nil, map[string]string{"foo": "bar"}) + controller.cleanupDeployment(test.oldRSs, d) + + deletedRSs := sets.String{} + for _, action := range fake.Actions() { + deleteAction, ok := action.(clienttesting.DeleteActionImpl) + if !ok { + t.Logf("Found not-delete action with verb %v. Ignoring.", action.GetVerb()) + continue + } + + if deleteAction.GetResource().Resource != "replicasets" { + continue + } + + deletedRSs.Insert(deleteAction.GetName()) + } + t.Logf("&test.revisionHistoryLimit: %d, &test.deletedReplicaSets: %v", test.revisionHistoryLimit, deletedRSs) + + if !test.expectedDeletedRSs.Equal(deletedRSs) { + t.Errorf("expect to delete old replica sets %v, but got %v", test.expectedDeletedRSs, deletedRSs) + continue + } + } +} From b2de3507d0f3793031efc0e42aeb57b995b84d96 Mon Sep 17 00:00:00 2001 From: waynepeking348 Date: Sat, 16 Jan 2021 22:20:53 +0800 Subject: [PATCH 3/3] add dependency in bazel BUILD file --- pkg/controller/deployment/BUILD | 1 + pkg/controller/deployment/sync_test.go | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/deployment/BUILD b/pkg/controller/deployment/BUILD index 9b783e71259..ccf9b7f3176 100644 --- a/pkg/controller/deployment/BUILD +++ b/pkg/controller/deployment/BUILD @@ -78,6 +78,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/client-go/informers:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library", diff --git a/pkg/controller/deployment/sync_test.go b/pkg/controller/deployment/sync_test.go index d441341b593..3405f8c5e60 100644 --- a/pkg/controller/deployment/sync_test.go +++ b/pkg/controller/deployment/sync_test.go @@ -27,7 +27,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" - clienttesting "k8s.io/client-go/testing" testclient "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/controller" @@ -570,7 +569,7 @@ func TestDeploymentController_cleanupDeploymentOrder(t *testing.T) { deletedRSs := sets.String{} for _, action := range fake.Actions() { - deleteAction, ok := action.(clienttesting.DeleteActionImpl) + deleteAction, ok := action.(testclient.DeleteActionImpl) if !ok { t.Logf("Found not-delete action with verb %v. Ignoring.", action.GetVerb()) continue