Repository: knative/serving
Pull request #10449 (2020-12-28 20:38:20)

Take 1% time into account
When computing rollout properties, take into account the duration
of the first step. Especially, if the first step is noticeably large
compared to the overall rollout duration, or even larger than it.
Before we'd rollout in possibly much longer than duration time,
with this, we'd be much closer to that time.

/assign @tcnghia mattmoor

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10449#" 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: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | 369f0a2af977c60c9f172f2dd38adb61362eab14 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10449/pull-knative-serving-unit-tests/1343657579863085056) | `/test pull-knative-serving-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10449). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10449?src=pr&el=h1) Report
> Merging [#10449](https://codecov.io/gh/knative/serving/pull/10449?src=pr&el=desc) (0a734f0) into [master](https://codecov.io/gh/knative/serving/commit/5ef6464614bdc71ea992bcb11d8ab55a75f90862?el=desc) (5ef6464) will **decrease** coverage by `0.06%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10449      +/-   ##
==========================================
- Coverage   88.06%   88.00%   -0.07%     
==========================================
  Files         186      186              
  Lines        8799     8800       +1     
==========================================
- Hits         7749     7744       -5     
- Misses        811      815       +4     
- Partials      239      241       +2     
```


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

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10449?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/10449?src=pr&el=footer). Last update [5ef6464...0a734f0](https://codecov.io/gh/knative/serving/pull/10449?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 aa2a2d403..9c068312d 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -438,6 +438,11 @@ func stepConfig(ctx context.Context, goal, prev *ConfigurationRollout, nowTS int
 func (cur *ConfigurationRollout) computeProperties(nowTS, minStepSec, durationSecs float64) {
 	// 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.
+	// Take into account that we already used minStepSecs to move first
+	// 1% so the overall rollout duration is shorter by this amount.
+	// If it took longer than duration it might be negative, so cap it
+	// at 1s, so we just do 1 step.
+	durationSecs = math.Max(1, durationSecs-minStepSec)
 	numSteps := math.Max(1, durationSecs/minStepSec)
 	pf := float64(cur.Percent)
 


Test patch:
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index a6c23e1aa..edd9063f2 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"math"
 	"testing"
 	"time"
 
@@ -450,10 +451,10 @@ func TestReconcile(t *testing.T) {
 					RevisionName: "config-00001", Percent: 1,
 				}}, fakeCurTime.Add(-3*time.Second),
 					func(r *traffic.Rollout) {
-						const numSteps = 120 / 3 // 40
-						// Step duration is 3s (duration / numSteps = 120/40)
-						r.Configurations[0].StepParams.StepDuration = int64(time.Second) * 120 / numSteps // 3s in ns.
-						r.Configurations[0].StepParams.StepSize = (100 - 1) / numSteps                    // 2
+						const numSteps = (120 - 3) / 3 // 39
+						// Step duration is 3s (duration - 3)/ numSteps = 120/40)
+						r.Configurations[0].StepParams.StepDuration = int64(time.Second) * (120 - 3) / numSteps   // 3s in ns.
+						r.Configurations[0].StepParams.StepSize = int(math.Round((100. - 1) / float64(numSteps))) // 3
 						// StepDuration is 3, and so next step is `now` + 3.
 						r.Configurations[0].StepParams.NextStepTime = fakeCurTime.Add(3 * time.Second).UnixNano()
 					},
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 9380455ca..ad6bba478 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -1011,7 +1011,9 @@ func TestObserveReady(t *testing.T) {
 	const (
 		now         = 200620092020 + 1982
 		oldenDays   = 198219841988
+		oldenStep   = float64(now-oldenDays) / float64(time.Second)
 		ancientDays = 14921812
+		ancientStep = float64(now-ancientDays) / float64(time.Second)
 		duration    = 120.
 	)
 	ro := Rollout{
@@ -1050,7 +1052,8 @@ func TestObserveReady(t *testing.T) {
 			Percent: 50,
 		}, {
 			// This covers the case when duration of a step is larger
-			// than duration of the whole rollout.
+			// than duration of the whole rollout. Which means we'll
+			// do the whole rollout in one final step.
 			ConfigurationName: "días antiguos",
 			StepParams: RolloutParams{
 				StartTime: ancientDays,
@@ -1074,9 +1077,9 @@ func TestObserveReady(t *testing.T) {
 			Percent:           100,
 			StepParams: RolloutParams{
 				StartTime:    200620092020,
-				StepDuration: 1212121212, // 120/99
+				StepDuration: 1202020202, // (120s-1984ns)/99
 				StepSize:     1,
-				NextStepTime: now + 1212121212,
+				NextStepTime: now + 1202020202,
 			},
 		}, {
 			ConfigurationName: "step-begin > 1s",
@@ -1084,7 +1087,7 @@ func TestObserveReady(t *testing.T) {
 			StepParams: RolloutParams{
 				StartTime:    oldenDays,
 				StepDuration: int64(3 * time.Second),
-				StepSize:     100 / 40,
+				StepSize:     3, // int(100. / ((duration - oldenStep) / oldenStep)),
 				NextStepTime: now + 3*int64(time.Second),
 			},
 		}, {
@@ -1110,9 +1113,9 @@ func TestObserveReady(t *testing.T) {
 			Percent:           13,
 			StepParams: RolloutParams{
 				StartTime:    ancientDays,
-				StepDuration: int64(duration * time.Second),
+				StepDuration: int64(time.Second), // The minimal possible.
 				StepSize:     12,
-				NextStepTime: now + int64(duration*time.Second),
+				NextStepTime: now + int64(time.Second),
 			},
 		}},
 	}
diff --git a/third_party/kourier-latest/kourier.yaml b/third_party/kourier-latest/kourier.yaml
index 85912d24d..07d0bf0b8 100644
--- a/third_party/kourier-latest/kourier.yaml
+++ b/third_party/kourier-latest/kourier.yaml
@@ -18,7 +18,7 @@ metadata:
   name: kourier-system
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 
 ---
 # Copyright 2020 The Knative Authors
@@ -42,7 +42,7 @@ metadata:
   namespace: knative-serving
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -51,7 +51,7 @@ metadata:
   namespace: knative-serving
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 rules:
   - apiGroups: [""]
     resources: ["events"]
@@ -78,7 +78,7 @@ metadata:
   name: 3scale-kourier
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole
@@ -110,7 +110,7 @@ metadata:
   namespace: knative-serving
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 spec:
   replicas: 1
   selector:
@@ -122,7 +122,7 @@ spec:
         app: 3scale-kourier-control
     spec:
       containers:
-        - image: gcr.io/knative-nightly/knative.dev/net-kourier/cmd/kourier@sha256:0890e5fd952a18d7758724cd81a138b064601b6b80428e0d7a02d28a3853a4de
+        - image: gcr.io/knative-nightly/knative.dev/net-kourier/cmd/kourier@sha256:98248dabcfc70033bd96d3cea0ad3fd69ed668dfa09ae5b39f49c26e814b2d46
           name: kourier-control
           env:
             - name: CERTS_SECRET_NAMESPACE
@@ -158,7 +158,7 @@ metadata:
   namespace: knative-serving
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 spec:
   ports:
     - name: grpc-xds
@@ -191,7 +191,7 @@ metadata:
   namespace: kourier-system
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 spec:
   selector:
     matchLabels:
@@ -257,7 +257,7 @@ metadata:
   namespace: kourier-system
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 spec:
   ports:
     - name: http2
@@ -279,7 +279,7 @@ metadata:
   namespace: kourier-system
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 spec:
   ports:
     - name: http2
@@ -297,7 +297,7 @@ metadata:
   namespace: kourier-system
   labels:
     networking.knative.dev/ingress-provider: kourier
-    serving.knative.dev/release: "v20201223-2dbe9b73"
+    serving.knative.dev/release: "v20201229-2190b2c8"
 data:
   envoy-bootstrap.yaml: |
     dynamic_resources: