From af0f5dcc88ef7fd5090994ed4602119606964c83 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Fri, 8 Sep 2017 01:26:48 +0200 Subject: [PATCH] Fix deployment timeout reporting If the previous condition has been a successful rollout then we shouldn't try to estimate any progress. Scenario: * progressDeadlineSeconds is smaller than the difference between now and the time the last rollout finished in the past. * the creation of a new ReplicaSet triggers a resync of the Deployment prior to the cached copy of the Deployment getting updated with the status.condition that indicates the creation of the new ReplicaSet. The Deployment will be resynced and eventually its Progressing condition will catch up with the state of the world. Signed-off-by: Michail Kargakis --- pkg/controller/deployment/util/deployment_util.go | 14 ++++++++++++++ .../deployment/util/deployment_util_test.go | 15 +++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 4800497fda3..39872bd17b5 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -861,6 +861,20 @@ func DeploymentTimedOut(deployment *extensions.Deployment, newStatus *extensions if condition == nil { return false } + // If the previous condition has been a successful rollout then we shouldn't try to + // estimate any progress. Scenario: + // + // * progressDeadlineSeconds is smaller than the difference between now and the time + // the last rollout finished in the past. + // * the creation of a new ReplicaSet triggers a resync of the Deployment prior to the + // cached copy of the Deployment getting updated with the status.condition that indicates + // the creation of the new ReplicaSet. + // + // The Deployment will be resynced and eventually its Progressing condition will catch + // up with the state of the world. + if condition.Reason == NewRSAvailableReason { + return false + } if condition.Reason == TimedOutReason { return true } diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 7c27d8f771a..115ae9148a1 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -1127,7 +1127,7 @@ func TestDeploymentTimedOut(t *testing.T) { timeFn := func(min, sec int) time.Time { return time.Date(2016, 1, 1, 0, min, sec, 0, time.UTC) } - deployment := func(condType extensions.DeploymentConditionType, status v1.ConditionStatus, pds *int32, from time.Time) extensions.Deployment { + deployment := func(condType extensions.DeploymentConditionType, status v1.ConditionStatus, reason string, pds *int32, from time.Time) extensions.Deployment { return extensions.Deployment{ Spec: extensions.DeploymentSpec{ ProgressDeadlineSeconds: pds, @@ -1137,6 +1137,7 @@ func TestDeploymentTimedOut(t *testing.T) { { Type: condType, Status: status, + Reason: reason, LastUpdateTime: metav1.Time{Time: from}, }, }, @@ -1155,24 +1156,30 @@ func TestDeploymentTimedOut(t *testing.T) { { name: "no progressDeadlineSeconds specified - no timeout", - d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, null, timeFn(1, 9)), + d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, "", null, 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", - d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, &ten, timeFn(1, 9)), + d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, "", &ten, timeFn(1, 9)), nowFn: func() time.Time { return timeFn(1, 20) }, expected: true, }, { name: "progressDeadlineSeconds: 10s, now - started => 00:01:20 - 00:01:11 => 9s", - d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, &ten, timeFn(1, 11)), + d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, "", &ten, timeFn(1, 11)), nowFn: func() time.Time { return timeFn(1, 20) }, expected: false, }, + { + name: "previous status was a complete deployment", + + d: deployment(extensions.DeploymentProgressing, v1.ConditionTrue, NewRSAvailableReason, nil, time.Time{}), + expected: false, + }, } for _, test := range tests {