From a4f85c8dd0aadcf6d49e71cf4b2e9488f26fea11 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Tue, 24 Jul 2018 13:20:39 -0700 Subject: [PATCH] Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32. 1. MaxInt32 has the same meaning as unset, for compatibility 2. Deployment controller treats MaxInt32 the same as unset (nil) --- pkg/apis/extensions/v1beta1/defaults.go | 8 ++++++++ pkg/apis/extensions/v1beta1/defaults_test.go | 13 +++++++++---- pkg/controller/deployment/progress.go | 6 +++--- pkg/controller/deployment/progress_test.go | 12 ++++++++++-- pkg/controller/deployment/sync.go | 8 ++++---- pkg/controller/deployment/util/deployment_util.go | 7 ++++++- .../deployment/util/deployment_util_test.go | 15 ++++++++++++--- 7 files changed, 52 insertions(+), 17 deletions(-) diff --git a/pkg/apis/extensions/v1beta1/defaults.go b/pkg/apis/extensions/v1beta1/defaults.go index d817c2f0f44..3138696a518 100644 --- a/pkg/apis/extensions/v1beta1/defaults.go +++ b/pkg/apis/extensions/v1beta1/defaults.go @@ -17,6 +17,8 @@ limitations under the License. package v1beta1 import ( + "math" + "k8s.io/api/core/v1" extensionsv1beta1 "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -110,6 +112,12 @@ func SetDefaults_Deployment(obj *extensionsv1beta1.Deployment) { strategy.RollingUpdate.MaxSurge = &maxSurge } } + // Set extensionsv1beta1.DeploymentSpec.ProgressDeadlineSeconds to MaxInt, + // which has the same meaning as unset. + if obj.Spec.ProgressDeadlineSeconds == nil { + obj.Spec.ProgressDeadlineSeconds = new(int32) + *obj.Spec.ProgressDeadlineSeconds = math.MaxInt32 + } } func SetDefaults_ReplicaSet(obj *extensionsv1beta1.ReplicaSet) { diff --git a/pkg/apis/extensions/v1beta1/defaults_test.go b/pkg/apis/extensions/v1beta1/defaults_test.go index a3baf2f3143..722a16af321 100644 --- a/pkg/apis/extensions/v1beta1/defaults_test.go +++ b/pkg/apis/extensions/v1beta1/defaults_test.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta1_test import ( + "math" "reflect" "testing" @@ -188,7 +189,8 @@ func TestSetDefaultDeployment(t *testing.T) { MaxUnavailable: &defaultIntOrString, }, }, - Template: defaultTemplate, + Template: defaultTemplate, + ProgressDeadlineSeconds: utilpointer.Int32Ptr(math.MaxInt32), }, }, }, @@ -213,7 +215,8 @@ func TestSetDefaultDeployment(t *testing.T) { MaxUnavailable: &defaultIntOrString, }, }, - Template: defaultTemplate, + Template: defaultTemplate, + ProgressDeadlineSeconds: utilpointer.Int32Ptr(math.MaxInt32), }, }, }, @@ -237,7 +240,8 @@ func TestSetDefaultDeployment(t *testing.T) { MaxUnavailable: &defaultIntOrString, }, }, - Template: defaultTemplate, + Template: defaultTemplate, + ProgressDeadlineSeconds: utilpointer.Int32Ptr(math.MaxInt32), }, }, }, @@ -256,7 +260,8 @@ func TestSetDefaultDeployment(t *testing.T) { Strategy: extensionsv1beta1.DeploymentStrategy{ Type: extensionsv1beta1.RecreateDeploymentStrategyType, }, - Template: defaultTemplate, + Template: defaultTemplate, + ProgressDeadlineSeconds: utilpointer.Int32Ptr(math.MaxInt32), }, }, }, diff --git a/pkg/controller/deployment/progress.go b/pkg/controller/deployment/progress.go index cfe35ebab4d..601359c79c4 100644 --- a/pkg/controller/deployment/progress.go +++ b/pkg/controller/deployment/progress.go @@ -36,7 +36,7 @@ func (dc *DeploymentController) syncRolloutStatus(allRSs []*apps.ReplicaSet, new newStatus := calculateStatus(allRSs, newRS, d) // If there is no progressDeadlineSeconds set, remove any Progressing condition. - if d.Spec.ProgressDeadlineSeconds == nil { + if !util.HasProgressDeadline(d) { util.RemoveDeploymentCondition(&newStatus, apps.DeploymentProgressing) } @@ -47,7 +47,7 @@ func (dc *DeploymentController) syncRolloutStatus(allRSs []*apps.ReplicaSet, new isCompleteDeployment := newStatus.Replicas == newStatus.UpdatedReplicas && currentCond != nil && currentCond.Reason == util.NewRSAvailableReason // Check for progress only if there is a progress deadline set and the latest rollout // hasn't completed yet. - if d.Spec.ProgressDeadlineSeconds != nil && !isCompleteDeployment { + if util.HasProgressDeadline(d) && !isCompleteDeployment { switch { case util.DeploymentComplete(d, &newStatus): // Update the deployment conditions with a message for the new replica set that @@ -159,7 +159,7 @@ var nowFn = func() time.Time { return time.Now() } func (dc *DeploymentController) requeueStuckDeployment(d *apps.Deployment, newStatus apps.DeploymentStatus) time.Duration { currentCond := util.GetDeploymentCondition(d.Status, apps.DeploymentProgressing) // Can't estimate progress if there is no deadline in the spec or progressing condition in the current status. - if d.Spec.ProgressDeadlineSeconds == nil || currentCond == nil { + if !util.HasProgressDeadline(d) || currentCond == nil { return time.Duration(-1) } // No need to estimate progress if the rollout is complete or already timed out. diff --git a/pkg/controller/deployment/progress_test.go b/pkg/controller/deployment/progress_test.go index da0aa2d67df..444bda998e7 100644 --- a/pkg/controller/deployment/progress_test.go +++ b/pkg/controller/deployment/progress_test.go @@ -17,6 +17,7 @@ limitations under the License. package deployment import ( + "math" "testing" "time" @@ -67,6 +68,7 @@ func newRSWithAvailable(name string, specReplicas, statusReplicas, availableRepl func TestRequeueStuckDeployment(t *testing.T) { pds := int32(60) + infinite := int32(math.MaxInt32) failed := []apps.DeploymentCondition{ { Type: apps.DeploymentProgressing, @@ -90,11 +92,17 @@ func TestRequeueStuckDeployment(t *testing.T) { expected time.Duration }{ { - name: "no progressDeadlineSeconds specified", + name: "nil progressDeadlineSeconds specified", d: currentDeployment(nil, 4, 3, 3, 2, nil), status: newDeploymentStatus(3, 3, 2), expected: time.Duration(-1), }, + { + name: "infinite progressDeadlineSeconds specified", + d: currentDeployment(&infinite, 4, 3, 3, 2, nil), + status: newDeploymentStatus(3, 3, 2), + expected: time.Duration(-1), + }, { name: "no progressing condition found", d: currentDeployment(&pds, 4, 3, 3, 2, nil), @@ -330,7 +338,7 @@ func TestSyncRolloutStatus(t *testing.T) { newCond := util.GetDeploymentCondition(test.d.Status, test.conditionType) switch { case newCond == nil: - if test.d.Spec.ProgressDeadlineSeconds != nil { + if test.d.Spec.ProgressDeadlineSeconds != nil && *test.d.Spec.ProgressDeadlineSeconds != math.MaxInt32 { t.Errorf("%s: expected deployment condition: %s", test.name, test.conditionType) } case newCond.Status != test.conditionStatus || newCond.Reason != test.conditionReason: diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 63bd28ab18b..734def26894 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -71,7 +71,7 @@ func (dc *DeploymentController) sync(d *apps.Deployment, rsList []*apps.ReplicaS // These conditions are needed so that we won't accidentally report lack of progress for resumed deployments // that were paused for longer than progressDeadlineSeconds. func (dc *DeploymentController) checkPausedConditions(d *apps.Deployment) error { - if d.Spec.ProgressDeadlineSeconds == nil { + if !deploymentutil.HasProgressDeadline(d) { return nil } cond := deploymentutil.GetDeploymentCondition(d.Status, apps.DeploymentProgressing) @@ -158,7 +158,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *apps.Deployment, rsList, old // of this deployment then it is likely that old users started caring about progress. In that // case we need to take into account the first time we noticed their new replica set. cond := deploymentutil.GetDeploymentCondition(d.Status, apps.DeploymentProgressing) - if d.Spec.ProgressDeadlineSeconds != nil && cond == nil { + if deploymentutil.HasProgressDeadline(d) && cond == nil { msg := fmt.Sprintf("Found new replica set %q", rsCopy.Name) condition := deploymentutil.NewDeploymentCondition(apps.DeploymentProgressing, v1.ConditionTrue, deploymentutil.FoundNewRSReason, msg) deploymentutil.SetDeploymentCondition(&d.Status, *condition) @@ -253,7 +253,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *apps.Deployment, rsList, old return nil, err case err != nil: msg := fmt.Sprintf("Failed to create new replica set %q: %v", newRS.Name, err) - if d.Spec.ProgressDeadlineSeconds != nil { + if deploymentutil.HasProgressDeadline(d) { cond := deploymentutil.NewDeploymentCondition(apps.DeploymentProgressing, v1.ConditionFalse, deploymentutil.FailedRSCreateReason, msg) deploymentutil.SetDeploymentCondition(&d.Status, *cond) // We don't really care about this error at this point, since we have a bigger issue to report. @@ -269,7 +269,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *apps.Deployment, rsList, old } needsUpdate := deploymentutil.SetDeploymentRevision(d, newRevision) - if !alreadyExists && d.Spec.ProgressDeadlineSeconds != nil { + if !alreadyExists && deploymentutil.HasProgressDeadline(d) { msg := fmt.Sprintf("Created new replica set %q", createdRS.Name) condition := deploymentutil.NewDeploymentCondition(apps.DeploymentProgressing, v1.ConditionTrue, deploymentutil.NewReplicaSetReason, msg) deploymentutil.SetDeploymentCondition(&d.Status, *condition) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index a435e5eacef..2ea5d22e120 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -18,6 +18,7 @@ package util import ( "fmt" + "math" "sort" "strconv" "strings" @@ -773,7 +774,7 @@ var nowFn = func() time.Time { return time.Now() } // is older than progressDeadlineSeconds or a Progressing condition with a TimedOutReason reason already // exists. func DeploymentTimedOut(deployment *apps.Deployment, newStatus *apps.DeploymentStatus) bool { - if deployment.Spec.ProgressDeadlineSeconds == nil { + if !HasProgressDeadline(deployment) { return false } @@ -918,3 +919,7 @@ func ResolveFenceposts(maxSurge, maxUnavailable *intstrutil.IntOrString, desired return int32(surge), int32(unavailable), nil } + +func HasProgressDeadline(d *apps.Deployment) bool { + return d.Spec.ProgressDeadlineSeconds != nil && *d.Spec.ProgressDeadlineSeconds != math.MaxInt32 +} diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 1d90e848d0d..5bf5b1486cb 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -18,6 +18,7 @@ package util import ( "fmt" + "math" "math/rand" "reflect" "sort" @@ -1068,8 +1069,9 @@ func TestDeploymentProgressing(t *testing.T) { func TestDeploymentTimedOut(t *testing.T) { var ( - null *int32 - ten = int32(10) + null *int32 + ten = int32(10) + infinite = int32(math.MaxInt32) ) timeFn := func(min, sec int) time.Time { @@ -1102,12 +1104,19 @@ func TestDeploymentTimedOut(t *testing.T) { expected bool }{ { - name: "no progressDeadlineSeconds specified - no timeout", + name: "nil progressDeadlineSeconds specified - no timeout", d: deployment(apps.DeploymentProgressing, v1.ConditionTrue, "", null, timeFn(1, 9)), nowFn: func() time.Time { return timeFn(1, 20) }, expected: false, }, + { + name: "infinite progressDeadlineSeconds specified - no timeout", + + d: deployment(apps.DeploymentProgressing, v1.ConditionTrue, "", &infinite, timeFn(1, 9)), + nowFn: func() time.Time { return timeFn(1, 20) }, + expected: false, + }, { name: "progressDeadlineSeconds: 10s, now - started => 00:01:20 - 00:01:09 => 11s",