From 9bf61e72b82428ac0d24da46671fec3b9d05ec53 Mon Sep 17 00:00:00 2001 From: John Kelly Date: Tue, 24 Oct 2017 16:54:33 -0400 Subject: [PATCH 1/2] pkg/controller/deployment: unit tests for syncRolloutStatus Created unit tests for syncRolloutStatus function in the deployment package. Tests for syncRolloutStatus have brought the overall test coverage of deployment/progress to 82.1%. Signed-off-by: John Kelly --- pkg/controller/deployment/progress_test.go | 170 +++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/pkg/controller/deployment/progress_test.go b/pkg/controller/deployment/progress_test.go index 41125e7bc23..3ff527398a2 100644 --- a/pkg/controller/deployment/progress_test.go +++ b/pkg/controller/deployment/progress_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/util/workqueue" "k8s.io/kubernetes/pkg/controller/deployment/util" ) @@ -54,6 +55,16 @@ func currentDeployment(pds *int32, replicas, statusReplicas, updatedReplicas, av return d } +// helper to create RS with given availableReplicas +func newRSWithAvailable(name string, specReplicas, statusReplicas, availableReplicas int) *extensions.ReplicaSet { + rs := rs(name, specReplicas, nil, metav1.Time{}) + rs.Status = extensions.ReplicaSetStatus{ + Replicas: int32(statusReplicas), + AvailableReplicas: int32(availableReplicas), + } + return rs +} + func TestRequeueStuckDeployment(t *testing.T) { pds := int32(60) failed := []extensions.DeploymentCondition{ @@ -161,3 +172,162 @@ func TestRequeueStuckDeployment(t *testing.T) { } } } + +func TestSyncRolloutStatus(t *testing.T) { + pds := int32(60) + testTime := metav1.Date(2017, 2, 15, 18, 49, 00, 00, time.UTC) + failedTimedOut := extensions.DeploymentCondition{ + Type: extensions.DeploymentProgressing, + Status: v1.ConditionFalse, + Reason: util.TimedOutReason, + } + newRSAvailable := extensions.DeploymentCondition{ + Type: extensions.DeploymentProgressing, + Status: v1.ConditionTrue, + Reason: util.NewRSAvailableReason, + LastUpdateTime: testTime, + LastTransitionTime: testTime, + } + replicaSetUpdated := extensions.DeploymentCondition{ + Type: extensions.DeploymentProgressing, + Status: v1.ConditionTrue, + Reason: util.ReplicaSetUpdatedReason, + LastUpdateTime: testTime, + LastTransitionTime: testTime, + } + + tests := []struct { + name string + d *extensions.Deployment + allRSs []*extensions.ReplicaSet + newRS *extensions.ReplicaSet + conditionType extensions.DeploymentConditionType + conditionStatus v1.ConditionStatus + conditionReason string + lastUpdate metav1.Time + lastTransition metav1.Time + }{ + { + name: "General: remove Progressing condition and do not estimate progress if deployment has no Progress Deadline", + d: currentDeployment(nil, 3, 2, 2, 2, []extensions.DeploymentCondition{replicaSetUpdated}), + allRSs: []*extensions.ReplicaSet{newRSWithAvailable("bar", 0, 1, 1)}, + newRS: newRSWithAvailable("foo", 3, 2, 2), + }, + { + name: "General: do not estimate progress of deployment with only one active ReplicaSet", + d: currentDeployment(&pds, 3, 3, 3, 3, []extensions.DeploymentCondition{newRSAvailable}), + allRSs: []*extensions.ReplicaSet{newRSWithAvailable("bar", 3, 3, 3)}, + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.NewRSAvailableReason, + lastUpdate: testTime, + lastTransition: testTime, + }, + { + name: "DeploymentProgressing: dont update lastTransitionTime if deployment already has Progressing=True", + d: currentDeployment(&pds, 3, 2, 2, 2, []extensions.DeploymentCondition{replicaSetUpdated}), + allRSs: []*extensions.ReplicaSet{newRSWithAvailable("bar", 0, 1, 1)}, + newRS: newRSWithAvailable("foo", 3, 2, 2), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.ReplicaSetUpdatedReason, + lastTransition: testTime, + }, + { + name: "DeploymentProgressing: update everything if deployment has Progressing=False", + d: currentDeployment(&pds, 3, 2, 2, 2, []extensions.DeploymentCondition{failedTimedOut}), + allRSs: []*extensions.ReplicaSet{newRSWithAvailable("bar", 0, 1, 1)}, + newRS: newRSWithAvailable("foo", 3, 2, 2), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.ReplicaSetUpdatedReason, + }, + { + name: "DeploymentProgressing: create Progressing condition if it does not exist", + d: currentDeployment(&pds, 3, 2, 2, 2, []extensions.DeploymentCondition{}), + allRSs: []*extensions.ReplicaSet{newRSWithAvailable("bar", 0, 1, 1)}, + newRS: newRSWithAvailable("foo", 3, 2, 2), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.ReplicaSetUpdatedReason, + }, + { + name: "DeploymentComplete: dont update lastTransitionTime if deployment already has Progressing=True", + d: currentDeployment(&pds, 3, 3, 3, 3, []extensions.DeploymentCondition{replicaSetUpdated}), + allRSs: []*extensions.ReplicaSet{}, + newRS: newRSWithAvailable("foo", 3, 3, 3), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.NewRSAvailableReason, + lastTransition: testTime, + }, + { + name: "DeploymentComplete: update everything if deployment has Progressing=False", + d: currentDeployment(&pds, 3, 3, 3, 3, []extensions.DeploymentCondition{failedTimedOut}), + allRSs: []*extensions.ReplicaSet{}, + newRS: newRSWithAvailable("foo", 3, 3, 3), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.NewRSAvailableReason, + }, + { + name: "DeploymentComplete: create Progressing condition if it does not exist", + d: currentDeployment(&pds, 3, 3, 3, 3, []extensions.DeploymentCondition{}), + allRSs: []*extensions.ReplicaSet{}, + newRS: newRSWithAvailable("foo", 3, 3, 3), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.NewRSAvailableReason, + }, + { + name: "DeploymentTimedOut: update status if rollout exceeds Progress Deadline", + d: currentDeployment(&pds, 3, 2, 2, 2, []extensions.DeploymentCondition{replicaSetUpdated}), + allRSs: []*extensions.ReplicaSet{}, + newRS: newRSWithAvailable("foo", 3, 2, 2), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionFalse, + conditionReason: util.TimedOutReason, + }, + { + name: "DeploymentTimedOut: do not update status if deployment has existing timedOut condition", + d: currentDeployment(&pds, 3, 2, 2, 2, []extensions.DeploymentCondition{failedTimedOut}), + allRSs: []*extensions.ReplicaSet{}, + newRS: newRSWithAvailable("foo", 3, 2, 2), + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionFalse, + conditionReason: util.TimedOutReason, + lastUpdate: testTime, + lastTransition: testTime, + }, + } + + fake := fake.Clientset{} + dc := &DeploymentController{ + client: &fake, + } + + for _, test := range tests { + if test.newRS != nil { + test.allRSs = append(test.allRSs, test.newRS) + } + + err := dc.syncRolloutStatus(test.allRSs, test.newRS, test.d) + if err != nil { + t.Error(err) + } + + newCond := util.GetDeploymentCondition(test.d.Status, test.conditionType) + switch { + case newCond == nil: + if test.d.Spec.ProgressDeadlineSeconds != nil { + t.Errorf("%s: expected deployment condition: %s", test.name, test.conditionType) + } + case newCond.Status != test.conditionStatus || newCond.Reason != test.conditionReason: + t.Errorf("%s: DeploymentProgressing has status %s with reason %s. Expected %s with %s.", test.name, newCond.Status, newCond.Reason, test.conditionStatus, test.conditionReason) + case !test.lastUpdate.IsZero() && test.lastUpdate != testTime: + t.Errorf("%s: LastUpdateTime was changed to %s but expected %s;", test.name, test.lastUpdate, testTime) + case !test.lastTransition.IsZero() && test.lastTransition != testTime: + t.Errorf("%s: LastTransitionTime was changed to %s but expected %s;", test.name, test.lastTransition, testTime) + } + } +} From cb98157834cc86bdee0d3c7024c110ec9a3050ea Mon Sep 17 00:00:00 2001 From: John Kelly Date: Wed, 1 Nov 2017 14:42:49 -0400 Subject: [PATCH 2/2] pkg/controller/deployment: syncRolloutStatus additional unit test case Added additional test case that exercises fix #53614 or npe when scaling --replicas=0 Moved creation of clientset and deployment controller objects inside test loop since it's mutated every time an API action happens. Signed-off-by: John Kelly --- pkg/controller/deployment/progress_test.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/pkg/controller/deployment/progress_test.go b/pkg/controller/deployment/progress_test.go index 3ff527398a2..9677728e7d0 100644 --- a/pkg/controller/deployment/progress_test.go +++ b/pkg/controller/deployment/progress_test.go @@ -279,6 +279,14 @@ func TestSyncRolloutStatus(t *testing.T) { conditionStatus: v1.ConditionTrue, conditionReason: util.NewRSAvailableReason, }, + { + name: "DeploymentComplete: defend against NPE when newRS=nil", + d: currentDeployment(&pds, 0, 3, 3, 3, []extensions.DeploymentCondition{replicaSetUpdated}), + allRSs: []*extensions.ReplicaSet{newRSWithAvailable("foo", 0, 0, 0)}, + conditionType: extensions.DeploymentProgressing, + conditionStatus: v1.ConditionTrue, + conditionReason: util.NewRSAvailableReason, + }, { name: "DeploymentTimedOut: update status if rollout exceeds Progress Deadline", d: currentDeployment(&pds, 3, 2, 2, 2, []extensions.DeploymentCondition{replicaSetUpdated}), @@ -301,12 +309,12 @@ func TestSyncRolloutStatus(t *testing.T) { }, } - fake := fake.Clientset{} - dc := &DeploymentController{ - client: &fake, - } - for _, test := range tests { + fake := fake.Clientset{} + dc := &DeploymentController{ + client: &fake, + } + if test.newRS != nil { test.allRSs = append(test.allRSs, test.newRS) }