Repository: knative/serving
Pull request #10251 (2020-12-03 03:49:18)

Set initial computations for the rollout
- step size
- step duration
- next step size

Unit test in the rollout level.

/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/10251#" 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 | bdddbd0635230742cf1f7d4b29f948cafbb38140 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10251/pull-knative-serving-unit-tests/1334343957529759744) | `/test pull-knative-serving-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10251). [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/10251?src=pr&el=h1) Report
> Merging [#10251](https://codecov.io/gh/knative/serving/pull/10251?src=pr&el=desc) (bdddbd0) into [master](https://codecov.io/gh/knative/serving/commit/0a8bcbf34abe5dead0c32284a40c55acbdb8c9de?el=desc) (0a8bcbf) will **increase** coverage by `0.10%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10251      +/-   ##
==========================================
+ Coverage   88.10%   88.21%   +0.10%     
==========================================
  Files         184      184              
  Lines        8619     8630      +11     
==========================================
+ Hits         7594     7613      +19     
+ Misses        787      783       -4     
+ Partials      238      234       -4     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10251?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10251/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (+3.40%)` | :arrow_up: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10251/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `88.28% <0.00%> (+3.90%)` | :arrow_up: |

------

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


Patch:
diff --git a/OWNERS_ALIASES b/OWNERS_ALIASES
index 5ac75d31a..5ab36ed92 100644
--- a/OWNERS_ALIASES
+++ b/OWNERS_ALIASES
@@ -24,7 +24,6 @@ aliases:
   - dprotaso        # API
   - markusthoemmes  # Scaling
   - mattmoor        # API
-  - mdemirhan       # Network
   - tcnghia         # Network
   - vagababov       # Scaling
 
@@ -35,7 +34,6 @@ aliases:
   - grantr          # TOC
   - markusthoemmes  # Scaling/TOC
   - mattmoor        # API/TOC
-  - mdemirhan       # Network
   - nak3            # Network
   - tcnghia         # Network/TOC
   - ZhiminXiang     # Network
@@ -43,13 +41,11 @@ aliases:
 
   autoscaling-approvers:
   - markusthoemmes
-  - mdemirhan
   - vagababov
   - julz
   autoscaling-reviewers:
   - julz
   - markusthoemmes
-  - mdemirhan
   - taragu
   - vagababov
   - yanweiguo
@@ -74,7 +70,6 @@ aliases:
   networking-approvers:
   - JRBANCEL
   - markusthoemmes
-  - mdemirhan
   - nak3
   - tcnghia
   - vagababov
@@ -83,7 +78,6 @@ aliases:
   - andrew-su
   - JRBANCEL
   - markusthoemmes
-  - mdemirhan
   - nak3
   - shashwathi
   - tcnghia
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 3616159af..4900729ea 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -67,14 +67,17 @@ type ConfigurationRollout struct {
 	// This is required to compute step time and deadline.
 	StartTime int `json:"starttime,omitempty"`
 
-	// LastStepTimeStamp is the Unix timestamp when the last
-	// rollout step was performed.
-	LastStepTime int `json:"lastStep,omitempty"`
+	// NextStepTime is the Unix timestamp when the next
+	// rollout step should performed.
+	NextStepTime int `json:"lastStep,omitempty"`
 
 	// StepDuration is a rounded up number of seconds how long it took
 	// for ingress to successfully move first 1% of traffic to the new revision.
 	// Note, that his number does not include any coldstart, etc timing.
 	StepDuration int `json:"stepDuration,omitempty"`
+
+	// How much traffic to move in a single step.
+	StepSize int `json:"stepSize,omitempty"`
 }
 
 // RevisionRollout describes the revision in the config rollout.
@@ -89,9 +92,9 @@ type RevisionRollout struct {
 
 // Done returns true if there is no active rollout going on
 // for the configuration.
-func (cr *ConfigurationRollout) Done() bool {
+func (cur *ConfigurationRollout) Done() bool {
 	// Zero or just one revision.
-	return len(cr.Revisions) < 2
+	return len(cur.Revisions) < 2
 }
 
 // Validate validates current rollout for inconsistencies.
@@ -115,15 +118,21 @@ func (cur *Rollout) Validate() bool {
 	return true
 }
 
+// TODO(vagababov): default fake rollout duration in use, while we
+// only modify the annotation and do not actually modify the traffic.
+const durationSecs = 120.0
+
 // ObserveReady traverses the configs and the ones that are in rollout
 // but have not observed step time yet, will have it set, to
-// max(1, nowTS-cfg.StartTime). nowTS is unix timestamp in ns.
+// max(1, nowTS-cfg.StartTime).
 func (cur *Rollout) ObserveReady(nowTS int) {
 	for i := range cur.Configurations {
 		c := &cur.Configurations[i]
 		if c.StepDuration == 0 && c.StartTime > 0 {
-			c.StepDuration = int(math.Max(1,
-				math.Ceil(time.Duration(nowTS-c.StartTime).Seconds())))
+			// In really ceil(nowTS-c.StartTime) should always give 1s, but
+			// given possible time drift, we'll ensure that at least 1s is returned.
+			minStepSec := math.Max(1, math.Ceil(time.Duration(nowTS-c.StartTime).Seconds()))
+			c.computeProperties(float64(nowTS), minStepSec, durationSecs)
 		}
 	}
 }
@@ -133,7 +142,6 @@ func (cur *Rollout) ObserveReady(nowTS int) {
 // At the end of the call the returned object will contain the
 // desired traffic shape.
 // Step will return cur if no previous state was available.
-// nowTS is unix timestamp in ns.
 func (cur *Rollout) Step(prev *Rollout, nowTS int) *Rollout {
 	if prev == nil || len(prev.Configurations) == 0 {
 		return cur
@@ -214,6 +222,8 @@ func (cur *Rollout) Step(prev *Rollout, nowTS int) *Rollout {
 // the older revisions.
 func adjustPercentage(goal int, cr *ConfigurationRollout) {
 	switch diff := goal - cr.Percent; {
+	case goal == 0:
+		cr.Revisions = nil // No traffic, no rollout.
 	case diff > 0:
 		cr.Revisions[len(cr.Revisions)-1].Percent += diff
 	case diff < 0:
@@ -262,7 +272,7 @@ func stepConfig(goal, prev *ConfigurationRollout, nowTS int) *ConfigurationRollo
 			// Copy the duration info from the previous when no new revision
 			// has been created.
 			ret.Deadline = prev.Deadline
-			ret.LastStepTime = prev.LastStepTime
+			ret.NextStepTime = prev.NextStepTime
 			ret.StepDuration = prev.StepDuration
 			ret.StartTime = prev.StartTime
 		}
@@ -304,6 +314,40 @@ func stepConfig(goal, prev *ConfigurationRollout, nowTS int) *ConfigurationRollo
 	return ret
 }
 
+// computeProperties computes the time between steps, each step size
+// and next reconcile time. This is invoked when the rollout just starts.
+// nowTS current unix timestamp in ns.
+// 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
+	pf := float64(cur.Percent)
+
+	// The smallest step is 1%, so if we can fit more steps
+	// than we have percents, we'll make c.Percent-1 steps
+	// each equal to 1%. -1, since we already moved 1% of the traffic.
+	if pf < numSteps {
+		numSteps = pf - 1
+	}
+
+	// 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)
+
+	// The time we sleep between the steps.
+	// We round up here since it's better to roll slightly longer,
+	// than slightly shorter.
+	stepDuration := math.Ceil(durationSecs / numSteps)
+
+	cur.StepDuration = int(stepDuration)
+	cur.StepSize = int(stepSize)
+	cur.NextStepTime = int(nowTS + stepDuration*float64(time.Second))
+}
+
 // sortRollout sorts the rollout based on tag so it's consistent
 // from run to run, since input to the process is map iterator.
 func sortRollout(r *Rollout) {


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 6ee8baaec..0d8ff6d4a 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -79,7 +79,7 @@ func TestStep(t *testing.T) {
 					Percent:      100,
 				}},
 				Deadline:     2006, // <- Those should be ignored.
-				LastStepTime: 2009,
+				NextStepTime: 2009,
 				StepDuration: 2020,
 				StartTime:    2004,
 			}},
@@ -93,7 +93,7 @@ func TestStep(t *testing.T) {
 					Percent:      100,
 				}},
 				Deadline:     1982, // <- Those should be copied.
-				LastStepTime: 1984,
+				NextStepTime: 1984,
 				StepDuration: 1988,
 				StartTime:    1955,
 			}},
@@ -107,7 +107,7 @@ func TestStep(t *testing.T) {
 					Percent:      100,
 				}},
 				Deadline:     1982,
-				LastStepTime: 1984,
+				NextStepTime: 1984,
 				StepDuration: 1988,
 				StartTime:    1955,
 			}},
@@ -195,7 +195,7 @@ func TestStep(t *testing.T) {
 					Percent:      100,
 				}},
 				Deadline:     1982, // <- Those should be thrown out.
-				LastStepTime: 1984,
+				NextStepTime: 1984,
 				StepDuration: 1988,
 			}},
 		},
@@ -706,22 +706,58 @@ func TestObserveReady(t *testing.T) {
 	)
 	ro := Rollout{
 		Configurations: []ConfigurationRollout{{
+			Percent:           100,
 			ConfigurationName: "has-step",
 			StepDuration:      11,
 		}, {
 			ConfigurationName: "no-step-no-begin",
+			Percent:           100,
 		}, {
 			ConfigurationName: "step-begin < 1s",
 			StartTime:         200620092020,
+			Percent:           100,
 		}, {
 			ConfigurationName: "step-begin > 1s",
 			StartTime:         oldenDays,
+			Percent:           100,
+		}, {
+			ConfigurationName: "Percent not 100%",
+			StartTime:         oldenDays,
+			Percent:           50,
 		}},
 	}
 
-	want := ro
-	want.Configurations[2].StepDuration = 1
-	want.Configurations[3].StepDuration = 3 // 2.4 rounded up.
+	want := Rollout{
+		Configurations: []ConfigurationRollout{{
+			Percent:           100,
+			ConfigurationName: "has-step",
+			StepDuration:      11,
+		}, {
+			ConfigurationName: "no-step-no-begin",
+			Percent:           100,
+		}, {
+			ConfigurationName: "step-begin < 1s",
+			Percent:           100,
+			StartTime:         200620092020,
+			StepDuration:      2,
+			StepSize:          1,
+			NextStepTime:      now + int(2*time.Second),
+		}, {
+			ConfigurationName: "step-begin > 1s",
+			Percent:           100,
+			StartTime:         oldenDays,
+			StepDuration:      3,
+			StepSize:          100 / 40,
+			NextStepTime:      now + 3*int(time.Second),
+		}, {
+			ConfigurationName: "Percent not 100%",
+			Percent:           50,
+			StartTime:         oldenDays,
+			StepDuration:      3,
+			StepSize:          50 / 40,
+			NextStepTime:      now + 3*int(time.Second),
+		}},
+	}
 
 	// This works in place.
 	ro.ObserveReady(now)
@@ -981,7 +1017,7 @@ func TestJSONRoundtrip(t *testing.T) {
 			}},
 			StartTime:    1955,
 			Deadline:     2006,
-			LastStepTime: 1988,
+			NextStepTime: 1988,
 			StepDuration: 1984,
 		}, {
 			ConfigurationName: "no",