Repository: knative/serving
Pull request #10420 (2020-12-21 23:02:07)

Compute and set the Route.Status to match rollout status
Basically apply similar logic when computing the splits in the ingress,
but slightly simpler.

If there's a matching rollout, then pass its revisions to the code that
existed already for a single target (now it's a loop -- but otherwise
basically the same).

For tagged routes during rollout it will display the split over the
tagged route (it feels more natural that way).

/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/10420#" 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/10420?src=pr&el=h1) Report
> Merging [#10420](https://codecov.io/gh/knative/serving/pull/10420?src=pr&el=desc) (ee70f55) into [master](https://codecov.io/gh/knative/serving/commit/534e987e922ddbdbfc6c25a805901979d28d0228?el=desc) (534e987) will **decrease** coverage by `0.06%`.
> The diff coverage is `79.24%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10420      +/-   ##
==========================================
- Coverage   88.09%   88.02%   -0.07%     
==========================================
  Files         186      186              
  Lines        8771     8805      +34     
==========================================
+ Hits         7727     7751      +24     
- Misses        807      813       +6     
- Partials      237      241       +4     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10420?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/route.go](https://codecov.io/gh/knative/serving/pull/10420/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcm91dGUuZ28=) | `76.31% <62.50%> (-0.32%)` | :arrow_down: |
| [pkg/reconciler/route/traffic/traffic.go](https://codecov.io/gh/knative/serving/pull/10420/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy90cmFmZmljLmdv) | `93.03% <82.22%> (-1.19%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/leases.go](https://codecov.io/gh/knative/serving/pull/10420/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9sZWFzZXMuZ28=) | `76.97% <0.00%> (-1.44%)` | :arrow_down: |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10420/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/10420?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/10420?src=pr&el=footer). Last update [534e987...0c40b93](https://codecov.io/gh/knative/serving/pull/10420?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index 654cbc67a..c6906de36 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -152,11 +152,12 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil
 		return err
 	}
 
+	roInProgress := !effectiveRO.Done()
 	if ingress.GetObjectMeta().GetGeneration() != ingress.Status.ObservedGeneration {
 		r.Status.MarkIngressNotConfigured()
-	} else if !effectiveRO.Done() {
+	} else if roInProgress {
 		logger.Info("Rollout is in progress")
-		// Rollout in progress
+		// Rollout in progress, so mark the status as such.
 		r.Status.MarkIngressRolloutInProgress()
 	} else {
 		r.Status.PropagateIngressStatus(ingress.Status)
@@ -167,6 +168,20 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil
 		return err
 	}
 
+	// We do it here, rather than in the similar check above,
+	// since we might be inside a rollout and Ingress
+	// is not yet ready and that takes priority, so the `roInProgress` branch
+	// wil not be triggered.
+	if roInProgress {
+		// Update the route.Status.Traffic to contain correct traffic
+		// distribution based on rollout status.
+		r.Status.Traffic, err = traffic.GetRevisionTrafficTargets(ctx, r, effectiveRO)
+		if err != nil {
+			return err
+		}
+		return nil
+	}
+
 	logger.Info("Route successfully synced")
 	return nil
 }
@@ -324,8 +339,8 @@ func (c *Reconciler) configureTraffic(ctx context.Context, r *v1.Route) (*traffi
 
 	logger.Info("All referred targets are routable, marking AllTrafficAssigned with traffic information.")
 
-	// Domain should already be present
-	r.Status.Traffic, err = t.GetRevisionTrafficTargets(ctx, r)
+	// Pass empty rollout here. We'll recompute this if there is a rollout in progress.
+	r.Status.Traffic, err = t.GetRevisionTrafficTargets(ctx, r, &traffic.Rollout{})
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index 0a481f404..0b8066360 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -432,8 +432,9 @@ func stepConfig(ctx context.Context, goal, prev *ConfigurationRollout, nowTS int
 // 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
+	// 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.
+	numSteps := math.Max(1, durationSecs/minStepSec)
 	pf := float64(cur.Percent)
 
 	// The smallest step is 1%, so if we can fit more steps
@@ -447,7 +448,9 @@ func (cur *ConfigurationRollout) computeProperties(nowTS, minStepSec, durationSe
 	// 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)
+	// In addition, ensure that we don't have step size larger than total
+	//  percentage for the configuration.
+	stepSize := math.Min(pf-1, math.Round((pf-1)/numSteps))
 
 	// The time we sleep between the steps.
 	stepDuration := durationSecs / numSteps * float64(time.Second)
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 4ab7c19ce..1af1c348e 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -25,6 +25,7 @@ import (
 
 	net "knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
+	"knative.dev/pkg/apis"
 	"knative.dev/pkg/ptr"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
@@ -87,39 +88,93 @@ func BuildTrafficConfiguration(configLister listers.ConfigurationLister, revList
 	return builder.build()
 }
 
-// GetRevisionTrafficTargets returns a list of TrafficTarget flattened to the RevisionName, and having ConfigurationName cleared out.
-func (cfg *Config) GetRevisionTrafficTargets(ctx context.Context, r *v1.Route) ([]v1.TrafficTarget, error) {
-	results := make([]v1.TrafficTarget, len(cfg.revisionTargets))
-	for i, tt := range cfg.revisionTargets {
-		var pp *int64
-		if tt.Percent != nil {
-			pp = ptr.Int64(*tt.Percent)
+func rolloutConfig(cfgName string, ros []*ConfigurationRollout) *ConfigurationRollout {
+	for _, ro := range ros {
+		if ro.ConfigurationName == cfgName {
+			return ro
 		}
+	}
+	// Technically impossible with valid inputs.
+	return nil
+}
+
+func (cfg *Config) computeURL(ctx context.Context, r *v1.Route, tt *RevisionTarget) (*apis.URL, error) {
+	meta := r.ObjectMeta.DeepCopy()
+
+	hostname, err := domains.HostnameFromTemplate(ctx, meta.Name, tt.Tag)
+	if err != nil {
+		return nil, err
+	}
+
+	labels.SetVisibility(meta, cfg.Visibility[tt.Tag] == netv1alpha1.IngressVisibilityClusterLocal)
+
+	// HTTP is currently the only supported scheme.
+	fullDomain, err := domains.DomainNameFromTemplate(ctx, *meta, hostname)
+	if err != nil {
+		return nil, err
+	}
+	return domains.URL(domains.HTTPScheme, fullDomain), nil
+}
 
+func (cfg *Config) targetToStatus(ctx context.Context, r *v1.Route, tt *RevisionTarget,
+	revs []RevisionRollout, results []v1.TrafficTarget) (_ []v1.TrafficTarget, err error) {
+	var url *apis.URL
+	// Do this once per tag.
+	if tt.Tag != "" {
+		url, err = cfg.computeURL(ctx, r, tt)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	for i := range revs {
+		rr := &revs[i]
 		// We cannot `DeepCopy` here, since tt.TrafficTarget might contain both
 		// configuration and revision.
-		results[i] = v1.TrafficTarget{
+		result := v1.TrafficTarget{
 			Tag:            tt.Tag,
-			RevisionName:   tt.RevisionName,
-			Percent:        pp,
+			RevisionName:   rr.RevisionName,
+			Percent:        ptr.Int64(int64(rr.Percent)),
 			LatestRevision: tt.LatestRevision,
 		}
-		if tt.Tag != "" {
-			meta := r.ObjectMeta.DeepCopy()
 
-			hostname, err := domains.HostnameFromTemplate(ctx, meta.Name, tt.Tag)
-			if err != nil {
-				return nil, err
-			}
+		if tt.Tag != "" {
+			result.URL = url
+		}
+		results = append(results, result)
+	}
+	return results, nil
+}
 
-			labels.SetVisibility(meta, cfg.Visibility[tt.Tag] == netv1alpha1.IngressVisibilityClusterLocal)
+// GetRevisionTrafficTargets returns a list of TrafficTarget flattened to the RevisionName, and having ConfigurationName cleared out.
+func (cfg *Config) GetRevisionTrafficTargets(ctx context.Context, r *v1.Route, ro *Rollout) ([]v1.TrafficTarget, error) {
+	results := make([]v1.TrafficTarget, 0, len(cfg.revisionTargets))
+	for i := range cfg.revisionTargets {
+		tt := &cfg.revisionTargets[i]
+		var (
+			roCfg *ConfigurationRollout
+			revs  []RevisionRollout
+			err   error
+		)
+
+		if tt.LatestRevision != nil && *tt.LatestRevision {
+			cfgs := ro.RolloutsByTag(tt.Tag)
+			roCfg = rolloutConfig(tt.ConfigurationName, cfgs)
+		}
 
-			// HTTP is currently the only supported scheme.
-			fullDomain, err := domains.DomainNameFromTemplate(ctx, *meta, hostname)
-			if err != nil {
-				return nil, err
-			}
-			results[i].URL = domains.URL(domains.HTTPScheme, fullDomain)
+		// Not a rollout, or just a revision target. Setup mock revision rollout
+		// with a single element.
+		if roCfg == nil {
+			revs = []RevisionRollout{{
+				RevisionName: tt.RevisionName,
+				Percent:      int(*tt.Percent),
+			}}
+		} else {
+			revs = roCfg.Revisions
+		}
+		results, err = cfg.targetToStatus(ctx, r, tt, revs, results)
+		if err != nil {
+			return nil, err
 		}
 	}
 	return results, nil


Test patch:
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 80c106a32..a6c23e1aa 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -469,9 +469,14 @@ func TestReconcile(t *testing.T) {
 				WithURL, WithAddress, WithRouteConditionsAutoTLSDisabled,
 				WithRouteGeneration(2009), WithRouteObservedGeneration,
 				MarkTrafficAssigned, MarkInRollout, WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00000",
+						Percent:        ptr.Int64(99),
+						LatestRevision: ptr.Bool(true),
+					},
 					v1.TrafficTarget{
 						RevisionName:   "config-00001",
-						Percent:        ptr.Int64(100),
+						Percent:        ptr.Int64(1),
 						LatestRevision: ptr.Bool(true),
 					})),
 		}},
@@ -910,9 +915,14 @@ func TestReconcile(t *testing.T) {
 			Object: Route("default", "new-latest-ready", WithConfigTarget("config"),
 				WithURL, WithAddress, WithRouteConditionsAutoTLSDisabled, WithRouteGeneration(1),
 				MarkTrafficAssigned, MarkInRollout, WithRouteObservedGeneration, WithRouteFinalizer, WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00001",
+						Percent:        ptr.Int64(99),
+						LatestRevision: ptr.Bool(true),
+					},
 					v1.TrafficTarget{
 						RevisionName:   "config-00002",
-						Percent:        ptr.Int64(100),
+						Percent:        ptr.Int64(1),
 						LatestRevision: ptr.Bool(true),
 					})),
 		}},
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index 9383825f9..9380455ca 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -1009,9 +1009,10 @@ func TestStep(t *testing.T) {
 
 func TestObserveReady(t *testing.T) {
 	const (
-		now       = 200620092020 + 1982
-		oldenDays = 198219841988
-		duration  = 120.
+		now         = 200620092020 + 1982
+		oldenDays   = 198219841988
+		ancientDays = 14921812
+		duration    = 120.
 	)
 	ro := Rollout{
 		Configurations: []ConfigurationRollout{{
@@ -1047,6 +1048,14 @@ func TestObserveReady(t *testing.T) {
 				StartTime: oldenDays,
 			},
 			Percent: 50,
+		}, {
+			// This covers the case when duration of a step is larger
+			// than duration of the whole rollout.
+			ConfigurationName: "días antiguos",
+			StepParams: RolloutParams{
+				StartTime: ancientDays,
+			},
+			Percent: 13,
 		}},
 	}
 
@@ -1096,6 +1105,15 @@ func TestObserveReady(t *testing.T) {
 				StepSize:     50 / 40,
 				NextStepTime: now + 3*int64(time.Second),
 			},
+		}, {
+			ConfigurationName: "días antiguos",
+			Percent:           13,
+			StepParams: RolloutParams{
+				StartTime:    ancientDays,
+				StepDuration: int64(duration * time.Second),
+				StepSize:     12,
+				NextStepTime: now + int64(duration*time.Second),
+			},
 		}},
 	}
 
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index 7dba88dcc..2c9ef4371 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -1381,7 +1381,81 @@ func TestRoundTripping(t *testing.T) {
 	if tc, err := BuildTrafficConfiguration(configLister, revLister, route); err != nil {
 		t.Error("Unexpected error", err)
 	} else {
-		targets, err := tc.GetRevisionTrafficTargets(getContext(), route)
+		targets, err := tc.GetRevisionTrafficTargets(getContext(), route, &Rollout{})
+		if err != nil {
+			t.Error("Unexpected error:", err)
+		}
+		if got, want := targets, expected; !cmp.Equal(want, got) {
+			t.Errorf("Unexpected traffic diff (-want +got):\n%s", cmp.Diff(want, got))
+		}
+	}
+}
+
+func TestRoundTrippingWithRollout(t *testing.T) {
+	expected := []v1.TrafficTarget{{
+		RevisionName:   "older-rev",
+		Percent:        ptr.Int64(56),
+		LatestRevision: ptr.Bool(true),
+	}, {
+		RevisionName:   goodOldRev.Name,
+		Percent:        ptr.Int64(44),
+		LatestRevision: ptr.Bool(true),
+	}, {
+		Tag:            "beta",
+		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:   niceOldRev.Name,
+		URL:            domains.URL(domains.HTTPScheme, "alpha-test-route.test.example.com"),
+		LatestRevision: ptr.Bool(true),
+		Percent:        ptr.Int64(81),
+	}, {
+		Tag:            "alpha",
+		RevisionName:   niceNewRev.Name,
+		URL:            domains.URL(domains.HTTPScheme, "alpha-test-route.test.example.com"),
+		LatestRevision: ptr.Bool(true),
+		Percent:        ptr.Int64(19),
+	}}
+	route := testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
+		ConfigurationName: goodConfig.Name,
+		LatestRevision:    ptr.Bool(true),
+		Percent:           ptr.Int64(100),
+	}, v1.TrafficTarget{
+		Tag:          "beta",
+		RevisionName: goodNewRev.Name,
+	}, v1.TrafficTarget{
+		Tag:               "alpha",
+		ConfigurationName: niceConfig.Name,
+	}))
+	ro := &Rollout{
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: goodConfig.Name,
+			Revisions: []RevisionRollout{{
+				RevisionName: "older-rev",
+				Percent:      56,
+			}, {
+				RevisionName: goodOldRev.Name,
+				Percent:      44,
+			}},
+		}, {
+			ConfigurationName: niceConfig.Name,
+			Tag:               "alpha",
+			Revisions: []RevisionRollout{{
+				RevisionName: niceOldRev.Name,
+				Percent:      81,
+			}, {
+				RevisionName: niceNewRev.Name,
+				Percent:      19,
+			}},
+		}},
+	}
+	if tc, err := BuildTrafficConfiguration(configLister, revLister, route); err != nil {
+		t.Error("Unexpected error", err)
+	} else {
+		targets, err := tc.GetRevisionTrafficTargets(getContext(), route, ro)
 		if err != nil {
 			t.Error("Unexpected error:", err)
 		}