Repository: knative/serving
Pull request #10274 (2020-12-05 01:28:16)

Step revisions when next rollout phase is due.
- when now > next step time
- compute new percentage allocations
- delete 0% revisions

For #9766 

/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/10274#" 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/10274?src=pr&el=h1) Report
> Merging [#10274](https://codecov.io/gh/knative/serving/pull/10274?src=pr&el=desc) (0152426) into [master](https://codecov.io/gh/knative/serving/commit/7ffa92364fdc761d6965d89a5946b65146d9e1ff?el=desc) (7ffa923) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10274      +/-   ##
==========================================
+ Coverage   88.14%   88.19%   +0.04%     
==========================================
  Files         184      184              
  Lines        8630     8649      +19     
==========================================
+ Hits         7607     7628      +21     
+ Misses        787      785       -2     
  Partials      236      236              
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10274?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/rollout.go](https://codecov.io/gh/knative/serving/pull/10274/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy9yb2xsb3V0Lmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10274/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `91.40% <0.00%> (+0.90%)` | :arrow_up: |

------

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

knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-upgrade-tests | 4fb1d51b0b9debd429c151ac3a2455ae8e6b965e | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10274/pull-knative-serving-upgrade-tests/1335033239072411648) | `/test pull-knative-serving-upgrade-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10274). [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 -->

Patch:
diff --git a/config/core/configmaps/logging.yaml b/config/core/configmaps/logging.yaml
index c6acaa972..b8d95c0de 100644
--- a/config/core/configmaps/logging.yaml
+++ b/config/core/configmaps/logging.yaml
@@ -20,7 +20,7 @@ metadata:
   labels:
     serving.knative.dev/release: devel
   annotations:
-    knative.dev/example-checksum: "23eed3d8"
+    knative.dev/example-checksum: "56c59cba"
 data:
   _example: |
     ################################
@@ -47,11 +47,11 @@ data:
         "errorOutputPaths": ["stderr"],
         "encoding": "json",
         "encoderConfig": {
-          "timeKey": "ts",
-          "levelKey": "level",
+          "timeKey": "timestamp",
+          "levelKey": "severity",
           "nameKey": "logger",
           "callerKey": "caller",
-          "messageKey": "msg",
+          "messageKey": "message",
           "stacktraceKey": "stacktrace",
           "lineEnding": "",
           "levelEncoder": "",
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 77de2f5d3..ec32014b4 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -243,6 +243,59 @@ func adjustPercentage(goal int, cr *ConfigurationRollout) {
 	}
 }
 
+// stepRevisions performs re-adjustment of percentages on the revisions
+// to rollout more traffic to the last one.
+func stepRevisions(goal *ConfigurationRollout, nowTS int) {
+	// Not yet ready to adjust the steps or we're done
+	// (shouldn't really be here, but better be defensive).
+	if nowTS < goal.NextStepTime || len(goal.Revisions) < 2 {
+		return
+	}
+
+	revLen := len(goal.Revisions)
+	remaining := goal.StepSize
+	writePos := revLen - 1
+	// readPos is guaranteed to be >= 0, due to the check above.
+	readPos := revLen - 2
+
+	// If step > totalPercent then remaining will always be > 0
+	// even after readPos == -1.
+	// This is the case when config's target is reduced below step size.
+	// E.g. was: R1 = 40 R2 = 10 Step = 10 Total=60
+	// Now = Total = 15;
+	// After adjust percentage: R1 = 5 R2 = 10
+	// Then after first iteration R1 = 0, remaining = 5.
+	// We'll handle this situation below.
+	for remaining > 0 && readPos >= 0 {
+		// If this revision's allocation is strictly larger than the goal,
+		// just subtract the different and we're done.
+		if goal.Revisions[readPos].Percent > remaining {
+			goal.Revisions[readPos].Percent -= remaining
+			break
+		}
+		// Otherwise subtract what is possible and update
+		// write position since this revision will no longer
+		// receive traffic.
+		remaining -= goal.Revisions[readPos].Percent
+		writePos--
+		readPos--
+	}
+	// Copy the last one to the write pos
+	goal.Revisions[writePos] = goal.Revisions[revLen-1]
+
+	goal.Revisions[writePos].Percent += goal.StepSize
+	// This can happen if step is now larger than total allocation, see the
+	// note above.
+	// E.g. with example above R2 = 20, and ro we have to cap it at 15.
+	if goal.Revisions[writePos].Percent > goal.Percent {
+		goal.Revisions[writePos].Percent = goal.Percent
+	}
+	// And cull the tail portion of it.
+	goal.Revisions = goal.Revisions[:writePos+1]
+	// Also set the next time.
+	goal.NextStepTime = nowTS + goal.StepDuration
+}
+
 // stepConfig takes previous and goal configuration shapes and returns a new
 // config rollout, after computing the percetage allocations.
 func stepConfig(goal, prev *ConfigurationRollout, nowTS int) *ConfigurationRollout {
@@ -275,6 +328,10 @@ func stepConfig(goal, prev *ConfigurationRollout, nowTS int) *ConfigurationRollo
 			ret.NextStepTime = prev.NextStepTime
 			ret.StepDuration = prev.StepDuration
 			ret.StartTime = prev.StartTime
+			// adjustPercentage above would've already accounted if target for the
+			// whole Configuration changed up or down. So here we should just redistribute
+			// the existing values.
+			stepRevisions(ret, nowTS)
 		}
 		return ret
 	}


Test patch:
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 0d8ff6d4a..b79a4aa62 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -1050,3 +1050,144 @@ func TestJSONRoundtrip(t *testing.T) {
 			cmp.Diff(orig, deserialized, cmpopts.EquateEmpty()))
 	}
 }
+
+func TestStepRevisions(t *testing.T) {
+	tests := []struct {
+		name string
+		now  int
+		cfg  *ConfigurationRollout
+		want *ConfigurationRollout
+	}{{
+		name: "noop (1): too soon",
+		now:  1982,
+		cfg: &ConfigurationRollout{
+			NextStepTime: 1984,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 10,
+			}, {
+				Percent: 5,
+			}, {
+				Percent: 60,
+			}},
+		},
+		want: &ConfigurationRollout{
+			NextStepTime: 1984,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 10,
+			}, {
+				Percent: 5,
+			}, {
+				Percent: 60,
+			}},
+		},
+	}, {
+		name: "noop (2): done",
+		now:  1982,
+		cfg: &ConfigurationRollout{
+			NextStepTime: 1984,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 10,
+			}},
+		},
+		want: &ConfigurationRollout{
+			NextStepTime: 1984,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 10,
+			}},
+		},
+	}, {
+		name: "step last, exact time",
+		now:  1984,
+		cfg: &ConfigurationRollout{
+			NextStepTime: 1984,
+			StepSize:     10,
+			StepDuration: 55,
+			Percent:      90,
+			Revisions: []RevisionRollout{{
+				Percent: 30,
+			}, {
+				Percent: 60,
+			}},
+		},
+		want: &ConfigurationRollout{
+			Percent:      90,
+			NextStepTime: 1984 + 55,
+			StepDuration: 55,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 20,
+			}, {
+				Percent: 70,
+			}},
+		},
+	}, {
+		name: "step last, delete some",
+		now:  1988,
+		cfg: &ConfigurationRollout{
+			Percent:      90,
+			NextStepTime: 1984,
+			StepSize:     10,
+			StepDuration: 55,
+			Revisions: []RevisionRollout{{
+				Percent: 22,
+			}, {
+				Percent: 5,
+			}, {
+				Percent: 3,
+			}, {
+				Percent: 60,
+			}},
+		},
+		want: &ConfigurationRollout{
+			Percent:      90,
+			NextStepTime: 1988 + 55,
+			StepDuration: 55,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 20,
+			}, {
+				Percent: 70,
+			}},
+		},
+	}, {
+		name: "over target now",
+		now:  1988,
+		cfg: &ConfigurationRollout{
+			Percent:      15,
+			NextStepTime: 1984,
+			StepSize:     10,
+			StepDuration: 77,
+			Revisions: []RevisionRollout{{
+				Percent: 5,
+			}, {
+				Percent: 1,
+			}, {
+				Percent: 1,
+			}, {
+				Percent: 8,
+			}},
+		},
+		want: &ConfigurationRollout{
+			Percent:      15,
+			NextStepTime: 1988 + 77,
+			StepDuration: 77,
+			StepSize:     10,
+			Revisions: []RevisionRollout{{
+				Percent: 15,
+			}},
+		},
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			stepRevisions(tc.cfg, tc.now)
+			if got, want := tc.cfg, tc.want; !cmp.Equal(got, want) {
+				t.Errorf("Incorrect revision stepping: diff(-want,+got):\n%s", cmp.Diff(want, got))
+			}
+		})
+	}
+}
diff --git a/test/config/config-logging.yaml b/test/config/config-logging.yaml
index 188368cd3..cd9239452 100644
--- a/test/config/config-logging.yaml
+++ b/test/config/config-logging.yaml
@@ -29,11 +29,11 @@ data:
       "errorOutputPaths": ["stderr"],
       "encoding": "json",
       "encoderConfig": {
-        "timeKey": "ts",
-        "levelKey": "level",
+        "timeKey": "timestamp",
+        "levelKey": "severity",
         "nameKey": "logger",
         "callerKey": "caller",
-        "messageKey": "msg",
+        "messageKey": "message",
         "stacktraceKey": "stacktrace",
         "lineEnding": "",
         "levelEncoder": "",