Repository: knative/serving
Pull request #10350 (2020-12-11 05:20:42)

Rather than `ceil` do `round` on the step size.
Current floor is too conservative, with 1.9 -> 1 basically doubling
the lenght of the rollout.
So rather round to make those more reasonable.

Update the tests that are affected and add a new one.

/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/10350#" 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":[]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10350#pullrequestreview-549809018" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10350#" 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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10350?src=pr&el=h1) Report
> Merging [#10350](https://codecov.io/gh/knative/serving/pull/10350?src=pr&el=desc) (d586083) into [master](https://codecov.io/gh/knative/serving/commit/d339141c7ce5ae05c8ddc111d860a7a56738ae29?el=desc) (d339141) will **decrease** coverage by `0.02%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10350      +/-   ##
==========================================
- Coverage   88.10%   88.08%   -0.03%     
==========================================
  Files         186      186              
  Lines        8711     8711              
==========================================
- Hits         7675     7673       -2     
- Misses        803      804       +1     
- Partials      233      234       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10350?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10350/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/10350/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `88.57% <0.00%> (-1.43%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10350?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/10350?src=pr&el=footer). Last update [d339141...d586083](https://codecov.io/gh/knative/serving/pull/10350?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 1ac95376b..2aa0949e3 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -416,11 +416,10 @@ func (cur *ConfigurationRollout) computeProperties(nowTS, minStepSec, durationSe
 	}
 
 	// We're moving traffic in equal steps.
-	// The last step can be larger, since it's more likely to
-	// easily accommodate the traffic jump, due to built in
-	// spare capacity.
-	// E.g. 100% in 4 steps. 1% -> 25% -> 49% -> 74% -> 100%, last jump being 26%.
-	stepSize := math.Floor((pf - 1) / numSteps)
+	// 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)
 
 	// The time we sleep between the steps.
 	stepDuration := durationSecs / numSteps * float64(time.Second)


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index f9c5d9476..914ccfd00 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -162,7 +162,8 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 		t.Errorf("NextStepTime = %d, want: %d", got, want)
 	}
 
-	if got, want := ro.Configurations[0].StepParams.StepSize, 1; got != want {
+	// Rounding up, so step size should be 2.
+	if got, want := ro.Configurations[0].StepParams.StepSize, 47/24+1; got != want {
 		t.Errorf("StepSize = %d, want: %d", got, want)
 	}
 	if !wasReenqueued {
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 91d186924..ebe010e49 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -1031,6 +1031,12 @@ func TestObserveReady(t *testing.T) {
 				StartTime: oldenDays,
 			},
 			Percent: 100,
+		}, {
+			ConfigurationName: "step-begin > 1s, step size round up",
+			StepParams: RolloutParams{
+				StartTime: oldenDays,
+			},
+			Percent: 75,
 		}, {
 			ConfigurationName: "Percent not 100%",
 			StepParams: RolloutParams{
@@ -1068,6 +1074,15 @@ func TestObserveReady(t *testing.T) {
 				StepSize:     100 / 40,
 				NextStepTime: now + 3*int64(time.Second),
 			},
+		}, {
+			ConfigurationName: "step-begin > 1s, step size round up",
+			Percent:           75,
+			StepParams: RolloutParams{
+				StartTime:    oldenDays,
+				StepDuration: int64(3 * time.Second),
+				StepSize:     75/40 + 1,
+				NextStepTime: now + 3*int64(time.Second),
+			},
 		}, {
 			ConfigurationName: "Percent not 100%",
 			Percent:           50,