Repository: knative/serving
Pull request #10418 (2020-12-21 21:41:48)

Remove status validation for route
and propagate up.
No reason to validate something that we write ourselves.

/assign mattmoor @tcnghia 

knative-prow-robot: @vagababov: GitHub didn't allow me to assign the following users: tcn.

Note that only [knative members](https://github.com/orgs/knative/people), repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see [the contributor guide](https://git.k8s.io/community/contributors/guide/first-contribution.md#issue-assignment-in-github)

<details>

In response to [this](https://github.com/knative/serving/pull/10418):

>and propagate up.
>No reason to validate something that we write ourselves.
>
>/assign mattmoor @tcn


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.
</details>
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10418#" 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/apis/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/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":[]} -->
vagababov: /assign @tcnghia

codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10418?src=pr&el=h1) Report
> Merging [#10418](https://codecov.io/gh/knative/serving/pull/10418?src=pr&el=desc) (a1e7058) into [master](https://codecov.io/gh/knative/serving/commit/534e987e922ddbdbfc6c25a805901979d28d0228?el=desc) (534e987) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #10418      +/-   ##
==========================================
- Coverage   88.09%   88.05%   -0.04%     
==========================================
  Files         186      186              
  Lines        8771     8760      -11     
==========================================
- Hits         7727     7714      -13     
- Misses        807      810       +3     
+ Partials      237      236       -1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/serving/v1/configuration\_validation.go](https://codecov.io/gh/knative/serving/pull/10418/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc2VydmluZy92MS9jb25maWd1cmF0aW9uX3ZhbGlkYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/apis/serving/v1/route\_validation.go](https://codecov.io/gh/knative/serving/pull/10418/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc2VydmluZy92MS9yb3V0ZV92YWxpZGF0aW9uLmdv) | `97.87% <ø> (-2.13%)` | :arrow_down: |
| [pkg/apis/serving/v1/service\_validation.go](https://codecov.io/gh/knative/serving/pull/10418/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc2VydmluZy92MS9zZXJ2aWNlX3ZhbGlkYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10418/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `90.41% <0.00%> (-0.92%)` | :arrow_down: |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/10418/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `90.00% <0.00%> (+1.42%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10418?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/10418?src=pr&el=footer). Last update [534e987...0d14289](https://codecov.io/gh/knative/serving/pull/10418?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-istio-stable-no-mesh | 0d14289d568359bbabd25ab107ad558746b82694 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10418/pull-knative-serving-istio-stable-no-mesh/1341136838496096256) | `/test pull-knative-serving-istio-stable-no-mesh`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10418). [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 -->
vagababov: Unrelated
/test pull-knative-serving-istio-stable-no-mesh

Patch:
diff --git a/pkg/apis/serving/v1/configuration_validation.go b/pkg/apis/serving/v1/configuration_validation.go
index 2509a3094..76b7d7922 100644
--- a/pkg/apis/serving/v1/configuration_validation.go
+++ b/pkg/apis/serving/v1/configuration_validation.go
@@ -41,8 +41,6 @@ func (c *Configuration) Validate(ctx context.Context) (errs *apis.FieldError) {
 		errs = errs.Also(c.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 	}
 
-	errs = errs.Also(c.Status.Validate(apis.WithinStatus(ctx)).ViaField("status"))
-
 	if apis.IsInUpdate(ctx) {
 		original := apis.GetBaseline(ctx).(*Configuration)
 		// Don't validate annotations(creator and lastModifier) when configuration owned by service
@@ -63,16 +61,6 @@ func (cs *ConfigurationSpec) Validate(ctx context.Context) *apis.FieldError {
 	return cs.Template.Validate(ctx).ViaField("template")
 }
 
-// Validate implements apis.Validatable
-func (cs *ConfigurationStatus) Validate(ctx context.Context) *apis.FieldError {
-	return cs.ConfigurationStatusFields.Validate(ctx)
-}
-
-// Validate implements apis.Validatable
-func (csf *ConfigurationStatusFields) Validate(ctx context.Context) *apis.FieldError {
-	return nil
-}
-
 // validateLabels function validates configuration labels
 func (c *Configuration) validateLabels() (errs *apis.FieldError) {
 	for key, val := range c.GetLabels() {
diff --git a/pkg/apis/serving/v1/route_validation.go b/pkg/apis/serving/v1/route_validation.go
index 936d5f411..bb79ec71d 100644
--- a/pkg/apis/serving/v1/route_validation.go
+++ b/pkg/apis/serving/v1/route_validation.go
@@ -32,7 +32,6 @@ func (r *Route) Validate(ctx context.Context) *apis.FieldError {
 	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta()).Also(
 		r.validateLabels().ViaField("labels")).ViaField("metadata")
 	errs = errs.Also(r.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
-	errs = errs.Also(r.Status.Validate(apis.WithinStatus(ctx)).ViaField("status"))
 
 	if apis.IsInUpdate(ctx) {
 		original := apis.GetBaseline(ctx).(*Route)
@@ -192,21 +191,6 @@ func (tt *TrafficTarget) validateURL(ctx context.Context, errs *apis.FieldError)
 	return errs
 }
 
-// Validate implements apis.Validatable.
-func (rs *RouteStatus) Validate(ctx context.Context) *apis.FieldError {
-	return rs.RouteStatusFields.Validate(ctx)
-}
-
-// Validate implements apis.Validatable.
-func (rsf *RouteStatusFields) Validate(ctx context.Context) *apis.FieldError {
-	// TODO(mattmoor): Validate other status fields.
-
-	if len(rsf.Traffic) != 0 {
-		return validateTrafficList(ctx, rsf.Traffic).ViaField("traffic")
-	}
-	return nil
-}
-
 func validateClusterVisibilityLabel(label string) (errs *apis.FieldError) {
 	if label != serving.VisibilityClusterLocal {
 		errs = apis.ErrInvalidValue(label, network.VisibilityLabelKey)
diff --git a/pkg/apis/serving/v1/service_validation.go b/pkg/apis/serving/v1/service_validation.go
index deab3be5c..7e0cd9525 100644
--- a/pkg/apis/serving/v1/service_validation.go
+++ b/pkg/apis/serving/v1/service_validation.go
@@ -41,8 +41,6 @@ func (s *Service) Validate(ctx context.Context) (errs *apis.FieldError) {
 		errs = errs.Also(s.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 	}
 
-	errs = errs.Also(s.Status.Validate(apis.WithinStatus(ctx)).ViaField("status"))
-
 	if apis.IsInUpdate(ctx) {
 		original := apis.GetBaseline(ctx).(*Service)
 		errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, s.Spec, original.GetAnnotations(),
@@ -62,12 +60,6 @@ func (ss *ServiceSpec) Validate(ctx context.Context) *apis.FieldError {
 		ss.RouteSpec.Validate(WithDefaultConfigurationName(ctx)))
 }
 
-// Validate implements apis.Validatable
-func (ss *ServiceStatus) Validate(ctx context.Context) *apis.FieldError {
-	return ss.ConfigurationStatusFields.Validate(ctx).Also(
-		ss.RouteStatusFields.Validate(ctx))
-}
-
 // validateLabels function validates service labels
 func (s *Service) validateLabels() (errs *apis.FieldError) {
 	for key, val := range s.GetLabels() {
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/apis/serving/v1/route_validation_test.go b/pkg/apis/serving/v1/route_validation_test.go
index fd11b664a..aa31383e0 100644
--- a/pkg/apis/serving/v1/route_validation_test.go
+++ b/pkg/apis/serving/v1/route_validation_test.go
@@ -191,14 +191,6 @@ func TestTrafficTargetValidation(t *testing.T) {
 			return WithDefaultConfigurationName(apis.WithinSpec(ctx))
 		},
 		want: apis.ErrGeneric(`may not set revisionName "" when latestRevision is false`, "latestRevision"),
-	}, {
-		name: "invalid without revisionName in status",
-		tt: &TrafficTarget{
-			ConfigurationName: "blah",
-			Percent:           ptr.Int64(37),
-		},
-		wc:   apis.WithinStatus,
-		want: apis.ErrMissingField("revisionName"),
 	}, {
 		name: "valid with revisionName and default configurationName",
 		tt: &TrafficTarget{
@@ -364,35 +356,6 @@ func TestRouteValidation(t *testing.T) {
 			},
 		},
 		want: nil,
-	}, {
-		name: "missing url in status",
-		r: &Route{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: RouteSpec{
-				Traffic: []TrafficTarget{{
-					Tag:          "bar",
-					RevisionName: "foo",
-					Percent:      ptr.Int64(100),
-				}},
-			},
-			Status: RouteStatus{
-				RouteStatusFields: RouteStatusFields{
-					Traffic: []TrafficTarget{{
-						Tag:          "bar",
-						RevisionName: "foo",
-						Percent:      ptr.Int64(100),
-					}},
-				},
-			},
-		},
-		want: &apis.FieldError{
-			Message: "missing field(s)",
-			Paths: []string{
-				"status.traffic[0].url",
-			},
-		},
 	}, {
 		name: "invalid traffic entry (missing oneof)",
 		r: &Route{
diff --git a/pkg/apis/serving/v1/service_validation_test.go b/pkg/apis/serving/v1/service_validation_test.go
index db90b92ba..38616c2b5 100644
--- a/pkg/apis/serving/v1/service_validation_test.go
+++ b/pkg/apis/serving/v1/service_validation_test.go
@@ -581,50 +581,6 @@ func TestServiceSubresourceUpdate(t *testing.T) {
 		},
 		subresource: "status",
 		want:        nil,
-	}, {
-		name: "status update with invalid status",
-		service: &Service{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: ServiceSpec{
-				ConfigurationSpec: ConfigurationSpec{
-					Template: RevisionTemplateSpec{
-						Spec: RevisionSpec{
-							PodSpec: corev1.PodSpec{
-								Containers: []corev1.Container{{
-									Image: "helloworld:foo",
-								}},
-							},
-							TimeoutSeconds: ptr.Int64(config.DefaultMaxRevisionTimeoutSeconds + 1),
-						},
-					},
-				},
-				RouteSpec: RouteSpec{
-					Traffic: []TrafficTarget{{
-						LatestRevision: ptr.Bool(true),
-						Percent:        ptr.Int64(100),
-					}},
-				},
-			},
-			Status: ServiceStatus{
-				RouteStatusFields: RouteStatusFields{
-					Traffic: []TrafficTarget{{
-						Tag:          "bar",
-						RevisionName: "foo",
-						Percent:      ptr.Int64(50), URL: &apis.URL{
-							Scheme: "http",
-							Host:   "foo.bar.com",
-						},
-					}},
-				},
-			},
-		},
-		subresource: "status",
-		want: &apis.FieldError{
-			Message: "Traffic targets sum to 50, want 100",
-			Paths:   []string{"status.traffic"},
-		},
 	}, {
 		name: "non-status sub resource update with valid revision template",
 		service: &Service{
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)
 		}