Repository: knative/serving
Pull request #9935 (2020-10-23 21:57:17)

Add percentage to the main config object.
When writing the actual rollout logic I found that it'll be much easier to have the target % provided,
especially if it can change. Rather than summing and comparing the individual revision rollout % values.

For #9765

/assign @tcnghia mattmoor

codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/9935?src=pr&el=h1) Report
> Merging [#9935](https://codecov.io/gh/knative/serving/pull/9935?src=pr&el=desc) into [master](https://codecov.io/gh/knative/serving/commit/d6e77f77bb9d637f8284d8f37904a6d4758d00a7?el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

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

```diff
@@           Coverage Diff           @@
##           master    #9935   +/-   ##
=======================================
  Coverage   87.85%   87.85%           
=======================================
  Files         183      183           
  Lines        8630     8631    +1     
=======================================
+ Hits         7582     7583    +1     
  Misses        799      799           
  Partials      249      249           
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/9935?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/traffic/traffic.go](https://codecov.io/gh/knative/serving/pull/9935/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy90cmFmZmljLmdv) | `94.35% <100.00%> (+0.03%)` | :arrow_up: |

------

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


Patch:
diff --git a/pkg/apis/serving/v1/route_defaults.go b/pkg/apis/serving/v1/route_defaults.go
index 55bcc825f..652eef568 100644
--- a/pkg/apis/serving/v1/route_defaults.go
+++ b/pkg/apis/serving/v1/route_defaults.go
@@ -55,4 +55,11 @@ func (tt *TrafficTarget) SetDefaults(ctx context.Context) {
 	if tt.LatestRevision == nil {
 		tt.LatestRevision = ptr.Bool(tt.RevisionName == "")
 	}
+	// Despite the fact that we have the field percent
+	// as required, historically we were lenient about checking this.
+	// But by setting explicit `0` we can eliminate lots of checking
+	// downstream in validation and controllers.
+	if tt.Percent == nil {
+		tt.Percent = ptr.Int64(0)
+	}
 }
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index aef33dcde..32e4afcde 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -43,6 +43,11 @@ type ConfigurationRollout struct {
 	ConfigurationName string `json:"configurationName"`
 	Tag               string `json:"tag,omitempty"`
 
+	// Percent denotes the total percentage for this configuration.
+	// The individual percentages of the Revisions below will sum to this
+	// number.
+	Percent int `json:"percent"`
+
 	// The revisions in the rollout. In steady state this should
 	// contain 0 (no revision is ready) or 1 (rollout done).
 	// During the actual rollout it will contain N revisions
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 88906cf1b..e7c440111 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -167,6 +167,8 @@ func newBuilder(
 
 // BuildRollout builds the current rollout state.
 // It is expected to be invoked after applySpecTraffic.
+// Returned Rollout will be sorted by tag and within tag by configuration
+// (only default tag can have more than configuration object attached).
 // TODO(vagababov): actually deal with rollouts, vs just report desired state.
 func (cfg *Config) BuildRollout() *Rollout {
 	rollout := &Rollout{}
@@ -205,9 +207,12 @@ func buildRolloutForTag(r *Rollout, tag string, rts RevisionTargets) {
 		r.Configurations = append(r.Configurations, ConfigurationRollout{
 			ConfigurationName: rt.ConfigurationName,
 			Tag:               tag,
+			Percent:           int(zeroIfNil(rt.Percent)),
 			Revisions: []RevisionRollout{{
 				RevisionName: rt.RevisionName,
-				Percent:      int(valIfNil(0, rt.Percent)),
+				// Note: this will match config value in steady state, but
+				// during rollout it will be overridden by the rollout logic.
+				Percent: int(zeroIfNil(rt.Percent)),
 			}},
 		})
 	}
@@ -340,10 +345,10 @@ func (cb *configBuilder) addRevisionTarget(tt *v1.TrafficTarget) error {
 	return nil
 }
 
-// valIfNil returns `val` if `ptr==nil`, or `*ptr` otherwise.
-func valIfNil(val int64, ptr *int64) int64 {
+// zeroIfNil returns `0` if `ptr==nil`, or `*ptr` otherwise.
+func zeroIfNil(ptr *int64) int64 {
 	if ptr == nil {
-		return val
+		return 0
 	}
 	return *ptr
 }
@@ -354,7 +359,7 @@ func mergeIfNecessary(rts RevisionTargets, rt RevisionTarget) RevisionTargets {
 	for i := range rts {
 		if rts[i].Tag == rt.Tag && rts[i].RevisionName == rt.RevisionName &&
 			*rt.LatestRevision == *rts[i].LatestRevision {
-			rts[i].Percent = ptr.Int64(valIfNil(0, rts[i].Percent) + valIfNil(0, rt.Percent))
+			rts[i].Percent = ptr.Int64(zeroIfNil(rts[i].Percent) + zeroIfNil(rt.Percent))
 			return rts
 		}
 	}


Test patch:
diff --git a/pkg/apis/serving/v1/route_defaults_test.go b/pkg/apis/serving/v1/route_defaults_test.go
index 7773c305e..9f409cb43 100644
--- a/pkg/apis/serving/v1/route_defaults_test.go
+++ b/pkg/apis/serving/v1/route_defaults_test.go
@@ -51,7 +51,7 @@ func TestRouteDefaulting(t *testing.T) {
 		},
 		wc: WithDefaultConfigurationName,
 	}, {
-		// Make sure it keeps a 'nil' as a 'nil' and not 'zero'
+		// Make sure it replaces a 'nil' as a '0'.
 		name: "implied zero percent",
 		in: &Route{
 			Spec: RouteSpec{
@@ -70,8 +70,8 @@ func TestRouteDefaulting(t *testing.T) {
 					LatestRevision: ptr.Bool(true),
 				}, {
 					RevisionName:   "bar",
-					Percent:        nil,
 					LatestRevision: ptr.Bool(false),
+					Percent:        ptr.Int64(0),
 				}},
 			},
 		},
diff --git a/pkg/apis/serving/v1/service_defaults_test.go b/pkg/apis/serving/v1/service_defaults_test.go
index 31b88e5ba..01547042a 100644
--- a/pkg/apis/serving/v1/service_defaults_test.go
+++ b/pkg/apis/serving/v1/service_defaults_test.go
@@ -253,6 +253,7 @@ func TestServiceDefaulting(t *testing.T) {
 						LatestRevision: ptr.Bool(false),
 					}, {
 						Tag:            "latest",
+						Percent:        ptr.Int64(0),
 						LatestRevision: ptr.Bool(true),
 					}},
 				},
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index d2656f55a..28e4cfafe 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -179,6 +179,7 @@ func TestBuildTrafficConfigurationVanilla(t *testing.T) {
 	wantR := &Rollout{
 		Configurations: []ConfigurationRollout{{
 			ConfigurationName: goodConfig.Name,
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: goodNewRev.Name,
 				Percent:      100,
@@ -282,6 +283,7 @@ func TestBuildTrafficConfigurationVanillaScaledToZero(t *testing.T) {
 	wantR := &Rollout{
 		Configurations: []ConfigurationRollout{{
 			ConfigurationName: inactiveConfig.Name,
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: inactiveRev.Name,
 				Percent:      100,
@@ -432,6 +434,7 @@ func TestBuildTrafficConfigurationTwoEntriesSameConfig(t *testing.T) {
 	wantR := &Rollout{
 		Configurations: []ConfigurationRollout{{
 			ConfigurationName: niceConfig.Name,
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: niceNewRev.Name,
 				Percent:      100,
@@ -566,18 +569,21 @@ func TestBuildTrafficConfigThreeConfigs(t *testing.T) {
 		// Default tag configs are sorted as well.
 		Configurations: []ConfigurationRollout{{
 			ConfigurationName: goodConfig.Name,
+			Percent:           35,
 			Revisions: []RevisionRollout{{
 				RevisionName: goodNewRev.Name,
 				Percent:      35,
 			}},
 		}, {
 			ConfigurationName: inactiveConfig.Name,
+			Percent:           35,
 			Revisions: []RevisionRollout{{
 				RevisionName: inactiveRev.Name,
 				Percent:      35,
 			}},
 		}, {
 			ConfigurationName: niceConfig.Name,
+			Percent:           30,
 			Revisions: []RevisionRollout{{
 				RevisionName: niceNewRev.Name,
 				Percent:      30,
@@ -586,6 +592,7 @@ func TestBuildTrafficConfigThreeConfigs(t *testing.T) {
 			// Note sorting flipped these two.
 			ConfigurationName: inactiveConfig.Name,
 			Tag:               "jackson",
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: inactiveRev.Name,
 				Percent:      100,
@@ -593,6 +600,7 @@ func TestBuildTrafficConfigThreeConfigs(t *testing.T) {
 		}, {
 			ConfigurationName: goodConfig.Name,
 			Tag:               "robert",
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: goodNewRev.Name,
 				Percent:      100,
@@ -675,6 +683,7 @@ func TestBuildTrafficConfigurationTwoEntriesSameConfigDifferentTags(t *testing.T
 	wantR := &Rollout{
 		Configurations: []ConfigurationRollout{{
 			ConfigurationName: niceConfig.Name,
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: niceNewRev.Name,
 				Percent:      100,
@@ -682,6 +691,7 @@ func TestBuildTrafficConfigurationTwoEntriesSameConfigDifferentTags(t *testing.T
 		}, {
 			ConfigurationName: niceConfig.Name,
 			Tag:               "robert",
+			Percent:           100,
 			Revisions: []RevisionRollout{{
 				RevisionName: niceNewRev.Name,
 				Percent:      100,
@@ -1030,6 +1040,7 @@ func TestBuildTrafficConfigurationPreliminary(t *testing.T) {
 					ConfigurationName: goodConfig.Name,
 					RevisionName:      goodNewRev.Name,
 					LatestRevision:    ptr.Bool(false),
+					Percent:           ptr.Int64(0),
 				},
 				Active:   true,
 				Protocol: net.ProtocolH2C,
@@ -1038,6 +1049,7 @@ func TestBuildTrafficConfigurationPreliminary(t *testing.T) {
 					Tag:               "alpha",
 					ConfigurationName: niceConfig.Name,
 					RevisionName:      niceNewRev.Name,
+					Percent:           ptr.Int64(0),
 					LatestRevision:    ptr.Bool(true),
 				},
 				Active:   true,
@@ -1081,6 +1093,7 @@ func TestBuildTrafficConfigurationPreliminary(t *testing.T) {
 				ConfigurationName: goodConfig.Name,
 				RevisionName:      goodNewRev.Name,
 				LatestRevision:    ptr.Bool(false),
+				Percent:           ptr.Int64(0),
 			},
 			Active:   true,
 			Protocol: net.ProtocolH2C,
@@ -1090,6 +1103,7 @@ func TestBuildTrafficConfigurationPreliminary(t *testing.T) {
 				ConfigurationName: niceConfig.Name,
 				RevisionName:      niceNewRev.Name,
 				LatestRevision:    ptr.Bool(true),
+				Percent:           ptr.Int64(0),
 			},
 			Active:   true,
 			Protocol: net.ProtocolH2C,
@@ -1318,11 +1332,13 @@ func TestRoundTripping(t *testing.T) {
 		RevisionName:   goodNewRev.Name,
 		URL:            domains.URL(domains.HTTPScheme, "beta-test-route.test.example.com"),
 		LatestRevision: ptr.Bool(false),
+		Percent:        ptr.Int64(0),
 	}, {
 		Tag:            "alpha",
 		RevisionName:   niceNewRev.Name,
 		URL:            domains.URL(domains.HTTPScheme, "alpha-test-route.test.example.com"),
 		LatestRevision: ptr.Bool(true),
+		Percent:        ptr.Int64(0),
 	}}
 	route := testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
 		RevisionName: goodOldRev.Name,
diff --git a/test/conformance/api/v1/service_test.go b/test/conformance/api/v1/service_test.go
index 718ba3aeb..2d9b8f5df 100644
--- a/test/conformance/api/v1/service_test.go
+++ b/test/conformance/api/v1/service_test.go
@@ -24,6 +24,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/ptr"
 	pkgTest "knative.dev/pkg/test"
@@ -305,6 +306,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 			Tag:            "latest",
 			RevisionName:   objects.Config.Status.LatestReadyRevisionName,
 			LatestRevision: ptr.Bool(true),
+			Percent:        ptr.Int64(0),
 		},
 	}
 	t.Log("Waiting for Service to become ready with the new shape.")
@@ -338,6 +340,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 		Tag:            "latest",
 		RevisionName:   secondRevision,
 		LatestRevision: ptr.Bool(true),
+		Percent:        ptr.Int64(0),
 	}
 	t.Log("Waiting for Service to become ready with the new shape.")
 	if err := waitForDesiredTrafficShape(t, names.Service, desiredTrafficShape, clients); err != nil {
@@ -365,8 +368,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 			RevisionName: secondRevision,
 			Percent:      ptr.Int64(50),
 		}, {
-			Tag:     "latest",
-			Percent: nil,
+			Tag: "latest",
 		}},
 	})
 	if err != nil {
@@ -390,6 +392,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 			Tag:            "latest",
 			RevisionName:   secondRevision,
 			LatestRevision: ptr.Bool(true),
+			Percent:        ptr.Int64(0),
 		},
 	}
 	t.Log("Waiting for Service to become ready with the new shape.")
@@ -421,6 +424,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 		Tag:            "latest",
 		RevisionName:   thirdRevision,
 		LatestRevision: ptr.Bool(true),
+		Percent:        ptr.Int64(0),
 	}
 	t.Log("Waiting for Service to become ready with the new shape.")
 	if err := waitForDesiredTrafficShape(t, names.Service, desiredTrafficShape, clients); err != nil {