Repository: knative/serving
Pull request #10419 (2020-12-21 22:07:46)

Fix the bug when the duration of the rollout < step size
Otherwise we endup with step size > 100% and duration longer than duration, and by a lot, possibly.

Change-Id: Idca5dc47a6ecdb83a9e5342a1760debc763bf2f5

/assign @tcnghia 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10419#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=h1) Report
> Merging [#10419](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=desc) (51ff125) into [master](https://codecov.io/gh/knative/serving/commit/534e987e922ddbdbfc6c25a805901979d28d0228?el=desc) (534e987) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/10419/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui)](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #10419   +/-   ##
=======================================
  Coverage   88.09%   88.09%           
=======================================
  Files         186      186           
  Lines        8771     8771           
=======================================
  Hits         7727     7727           
- Misses        807      808    +1     
+ Partials      237      236    -1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10419/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10419/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `90.41% <0.00%> (-0.92%)` | :arrow_down: |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/10419/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `90.00% <0.00%> (+1.42%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=footer). Last update [534e987...51ff125](https://codecov.io/gh/knative/serving/pull/10419?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10419#pullrequestreview-556721877" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10419#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [tcnghia,vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 0a481f404..0b8066360 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -432,8 +432,9 @@ func stepConfig(ctx context.Context, goal, prev *ConfigurationRollout, nowTS int
 // Pre: minStepSec >= 1, in seconds.
 // Pre: durationSecs > 1, in seconds.
 func (cur *ConfigurationRollout) computeProperties(nowTS, minStepSec, durationSecs float64) {
-	// First compute number of steps.
-	numSteps := durationSecs / minStepSec
+	// First compute number of steps. If it takes more time to step 1% than the
+	// whole allotted time for the rollout, do it in 1 step.
+	numSteps := math.Max(1, durationSecs/minStepSec)
 	pf := float64(cur.Percent)
 
 	// The smallest step is 1%, so if we can fit more steps
@@ -447,7 +448,9 @@ func (cur *ConfigurationRollout) computeProperties(nowTS, minStepSec, durationSe
 	// For bigger jumps this might yield slightly bigger moves
 	// but rounding down makes 1.9 => 1, which basically doubles the rollout time.
 	// E.g. 100% in 4 steps. 1% -> 26% -> 51% -> 76% -> 100%.
-	stepSize := math.Round((pf - 1) / numSteps)
+	// In addition, ensure that we don't have step size larger than total
+	//  percentage for the configuration.
+	stepSize := math.Min(pf-1, math.Round((pf-1)/numSteps))
 
 	// The time we sleep between the steps.
 	stepDuration := durationSecs / numSteps * float64(time.Second)


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 9383825f9..9380455ca 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -1009,9 +1009,10 @@ func TestStep(t *testing.T) {
 
 func TestObserveReady(t *testing.T) {
 	const (
-		now       = 200620092020 + 1982
-		oldenDays = 198219841988
-		duration  = 120.
+		now         = 200620092020 + 1982
+		oldenDays   = 198219841988
+		ancientDays = 14921812
+		duration    = 120.
 	)
 	ro := Rollout{
 		Configurations: []ConfigurationRollout{{
@@ -1047,6 +1048,14 @@ func TestObserveReady(t *testing.T) {
 				StartTime: oldenDays,
 			},
 			Percent: 50,
+		}, {
+			// This covers the case when duration of a step is larger
+			// than duration of the whole rollout.
+			ConfigurationName: "días antiguos",
+			StepParams: RolloutParams{
+				StartTime: ancientDays,
+			},
+			Percent: 13,
 		}},
 	}
 
@@ -1096,6 +1105,15 @@ func TestObserveReady(t *testing.T) {
 				StepSize:     50 / 40,
 				NextStepTime: now + 3*int64(time.Second),
 			},
+		}, {
+			ConfigurationName: "días antiguos",
+			Percent:           13,
+			StepParams: RolloutParams{
+				StartTime:    ancientDays,
+				StepDuration: int64(duration * time.Second),
+				StepSize:     12,
+				NextStepTime: now + int64(duration*time.Second),
+			},
 		}},
 	}