Repository: knative/serving
Pull request #10106 (2020-11-13 04:37:16)

Start executing rollout steps in the ingress reconcile
- This applies rolledout reconcile to the annotaion (no ingress spec changes yet)
- Moved the appllication from the createingress to route controller — since we need to compute diffs
  which makes things easier this way, because to do rollout we need to compare previous with the new one.

/assign @tcnghia mattmoor

For #9766

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10106#" 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/10106?src=pr&el=h1) Report
> Merging [#10106](https://codecov.io/gh/knative/serving/pull/10106?src=pr&el=desc) (a92cd20) into [master](https://codecov.io/gh/knative/serving/commit/1dd43e9d8b03356507cc125b00c1f5925391f66f?el=desc) (1dd43e9) will **decrease** coverage by `0.02%`.
> The diff coverage is `68.57%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10106      +/-   ##
==========================================
- Coverage   87.98%   87.96%   -0.03%     
==========================================
  Files         185      185              
  Lines        8717     8733      +16     
==========================================
+ Hits         7670     7682      +12     
- Misses        797      800       +3     
- Partials      250      251       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10106?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/resources/ingress.go](https://codecov.io/gh/knative/serving/pull/10106/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVzb3VyY2VzL2luZ3Jlc3MuZ28=) | `100.00% <ø> (+3.25%)` | :arrow_up: |
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10106/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `76.19% <67.64%> (-4.40%)` | :arrow_down: |
| [pkg/reconciler/route/route.go](https://codecov.io/gh/knative/serving/pull/10106/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcm91dGUuZ28=) | `75.52% <100.00%> (ø)` | |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10106/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `88.28% <0.00%> (+1.56%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10106?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/10106?src=pr&el=footer). Last update [1dd43e9...a92cd20](https://codecov.io/gh/knative/serving/pull/10106?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 | a92cd2096e9de58cf21ebded3df2b27f7b3bea6e | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10106/pull-knative-serving-upgrade-tests/1327108268648042496) | `/test pull-knative-serving-upgrade-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10106). [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/docs/runtime-contract.md b/docs/runtime-contract.md
index edf550f12..7cc7b0ea7 100644
--- a/docs/runtime-contract.md
+++ b/docs/runtime-contract.md
@@ -504,7 +504,8 @@ changes.
 From the OCI spec:
 
 > `rootfsPropagation` (string, OPTIONAL) sets the rootfs's mount propagation.
-> Its value is either slave, private, shared or unbindable. The <!-- wokeignore:rule=slave -->
+> Its value is either slave,<!-- wokeignore:rule=slave --> private, shared or
+> unbindable. The
 > [Shared Subtrees](https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt)
 > article in the kernel documentation has more information about mount
 > propagation.
diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index d8751ab3f..227ce7135 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -18,9 +18,11 @@ package route
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 
+	"github.com/davecgh/go-spew/spew"
 	"go.uber.org/zap"
 	"golang.org/x/sync/errgroup"
 	corev1 "k8s.io/api/core/v1"
@@ -30,9 +32,11 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/apis/duck"
 	"knative.dev/pkg/controller"
+	"knative.dev/pkg/kmeta"
 	"knative.dev/pkg/logging"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/reconciler/route/config"
@@ -40,10 +44,18 @@ import (
 	"knative.dev/serving/pkg/reconciler/route/traffic"
 )
 
-func (c *Reconciler) reconcileIngress(ctx context.Context, r *v1.Route, desired *netv1alpha1.Ingress) (*netv1alpha1.Ingress, error) {
+func (c *Reconciler) reconcileIngress(ctx context.Context, r *v1.Route, desired *netv1alpha1.Ingress, tc *traffic.Config) (*netv1alpha1.Ingress, error) {
 	recorder := controller.GetEventRecorder(ctx)
 	ingress, err := c.ingressLister.Ingresses(desired.Namespace).Get(desired.Name)
+
+	// Get the current rollout state as described by the traffic.
+	curRO := tc.BuildRollout()
+
 	if apierrs.IsNotFound(err) {
+		// If there is no exisiting Ingress, then current rollout is _the_ rollout.
+		desired.Annotations = kmeta.UnionMaps(desired.Annotations, map[string]string{
+			networking.RolloutAnnotationKey: serializeRollout(ctx, curRO),
+		})
 		ingress, err = c.netclient.NetworkingV1alpha1().Ingresses(desired.Namespace).Create(ctx, desired, metav1.CreateOptions{})
 		if err != nil {
 			recorder.Eventf(r, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress: %v", err)
@@ -54,25 +66,34 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, r *v1.Route, desired
 		return ingress, nil
 	} else if err != nil {
 		return nil, err
-	} else if !equality.Semantic.DeepEqual(ingress.Spec, desired.Spec) ||
-		!equality.Semantic.DeepEqual(ingress.Annotations, desired.Annotations) ||
-		!equality.Semantic.DeepEqual(ingress.Labels, desired.Labels) {
-		// It is notable that one reason for differences here may be defaulting.
-		// When that is the case, the Update will end up being a nop because the
-		// webhook will bring them into alignment and no new reconciliation will occur.
-		// Also, compare annotation and label in case ingress.Class or parent route's labels
-		// is updated.
-
-		// Don't modify the informers copy
-		origin := ingress.DeepCopy()
-		origin.Spec = desired.Spec
-		origin.Annotations = desired.Annotations
-		origin.Labels = desired.Labels
-		updated, err := c.netclient.NetworkingV1alpha1().Ingresses(origin.Namespace).Update(ctx, origin, metav1.UpdateOptions{})
-		if err != nil {
-			return nil, fmt.Errorf("failed to update Ingress: %w", err)
+	} else {
+		// Ingress exists. We need to compute the rollout spec diff.
+		prevRO := deserializeRollout(ctx, ingress.Annotations[networking.RolloutAnnotationKey])
+		effectiveRO := curRO.Step(prevRO)
+		// Update the annotation.
+		desired.Annotations[networking.RolloutAnnotationKey] = serializeRollout(ctx, effectiveRO)
+		// TODO(vagababov): apply the Rollout to the ingress spec here.
+		if !equality.Semantic.DeepEqual(ingress.Spec, desired.Spec) ||
+			!equality.Semantic.DeepEqual(ingress.Annotations, desired.Annotations) ||
+			!equality.Semantic.DeepEqual(ingress.Labels, desired.Labels) {
+			// It is notable that one reason for differences here may be defaulting.
+			// When that is the case, the Update will end up being a nop because the
+			// webhook will bring them into alignment and no new reconciliation will occur.
+			// Also, compare annotation and label in case ingress.Class or parent route's labels
+			// is updated.
+
+			// Don't modify the informers copy
+			origin := ingress.DeepCopy()
+			origin.Spec = desired.Spec
+			origin.Annotations = desired.Annotations
+			origin.Labels = desired.Labels
+			updated, err := c.netclient.NetworkingV1alpha1().Ingresses(origin.Namespace).Update(
+				ctx, origin, metav1.UpdateOptions{})
+			if err != nil {
+				return nil, fmt.Errorf("failed to update Ingress: %w", err)
+			}
+			return updated, nil
 		}
-		return updated, nil
 	}
 
 	return ingress, err
@@ -227,3 +248,30 @@ func (c *Reconciler) reconcileTargetRevisions(ctx context.Context, t *traffic.Co
 	}
 	return eg.Wait()
 }
+
+func serializeRollout(ctx context.Context, r *traffic.Rollout) string {
+	sr, err := json.Marshal(r)
+	if err != nil {
+		// This must never happen in the normal course of things.
+		logging.FromContext(ctx).Warnw("Error serializing Rollout: "+spew.Sprint(r),
+			zap.Error(err))
+		return ""
+	}
+	return string(sr)
+}
+
+func deserializeRollout(ctx context.Context, ro string) *traffic.Rollout {
+	if ro == "" {
+		return nil
+	}
+	r := &traffic.Rollout{}
+	// Failure can happen if users manually tweaked the
+	// annotation or there's etcd corruption. Just log, rollouts
+	// are not mission critical.
+	if err := json.Unmarshal([]byte(ro), r); err != nil {
+		logging.FromContext(ctx).Warnw("Error deserializing Rollout: "+ro,
+			zap.Error(err))
+		return nil
+	}
+	return r
+}
diff --git a/pkg/reconciler/route/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go
index b73590396..ad72e686d 100644
--- a/pkg/reconciler/route/resources/ingress.go
+++ b/pkg/reconciler/route/resources/ingress.go
@@ -18,11 +18,8 @@ package resources
 
 import (
 	"context"
-	"encoding/json"
 	"sort"
 
-	"github.com/davecgh/go-spew/spew"
-	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -33,7 +30,6 @@ import (
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	ingress "knative.dev/networking/pkg/ingress"
 	"knative.dev/pkg/kmeta"
-	"knative.dev/pkg/logging"
 	"knative.dev/serving/pkg/activator"
 	apicfg "knative.dev/serving/pkg/apis/config"
 	"knative.dev/serving/pkg/apis/serving"
@@ -78,7 +74,6 @@ func MakeIngress(
 			}),
 			Annotations: kmeta.FilterMap(kmeta.UnionMaps(map[string]string{
 				networking.IngressClassAnnotationKey: ingressClass,
-				networking.RolloutAnnotationKey:      serializeRollout(ctx, tc.BuildRollout()),
 			}, r.GetAnnotations()), func(key string) bool {
 				return key == corev1.LastAppliedConfigAnnotation
 			}),
@@ -88,17 +83,6 @@ func MakeIngress(
 	}, nil
 }
 
-func serializeRollout(ctx context.Context, r *traffic.Rollout) string {
-	sr, err := json.Marshal(r)
-	if err != nil {
-		// This must not never happen in the normal course of things.
-		logging.FromContext(ctx).Warnw("Error serializing Rollout: "+spew.Sprint(r),
-			zap.Error(err))
-		return ""
-	}
-	return string(sr)
-}
-
 // makeIngressSpec builds a new IngressSpec from inputs.
 func makeIngressSpec(
 	ctx context.Context,
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index a6ddb89b1..b158b3d0e 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -183,7 +183,7 @@ func (c *Reconciler) reconcileIngressResources(ctx context.Context, r *v1.Route,
 		return nil, err
 	}
 
-	ingress, err := c.reconcileIngress(ctx, r, desired)
+	ingress, err := c.reconcileIngress(ctx, r, desired, tc)
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index 24fecf20a..b04be03e7 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -49,9 +49,9 @@ func TestReconcileIngressInsert(t *testing.T) {
 	defer cancel()
 
 	r := Route("test-ns", "test-route")
-	ci := newTestIngress(t, r)
+	ci, tc := newTestIngress(t, r)
 
-	if _, err := reconciler.reconcileIngress(ctx, r, ci); err != nil {
+	if _, err := reconciler.reconcileIngress(ctx, r, ci, tc); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 }
@@ -65,15 +65,15 @@ func TestReconcileIngressUpdate(t *testing.T) {
 
 	r := Route("test-ns", "test-route")
 
-	ci := newTestIngress(t, r)
-	if _, err := reconciler.reconcileIngress(ctx, r, ci); err != nil {
+	ci, tc := newTestIngress(t, r)
+	if _, err := reconciler.reconcileIngress(ctx, r, ci, tc); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
 	updated := getRouteIngressFromClient(ctx, t, r)
 	fakeciinformer.Get(ctx).Informer().GetIndexer().Add(updated)
 
-	ci2 := newTestIngress(t, r, func(tc *traffic.Config) {
+	ci2, tc := newTestIngress(t, r, func(tc *traffic.Config) {
 		tc.Targets[traffic.DefaultTarget][0].TrafficTarget.Percent = ptr.Int64(50)
 		tc.Targets[traffic.DefaultTarget] = append(tc.Targets[traffic.DefaultTarget], traffic.RevisionTarget{
 			TrafficTarget: v1.TrafficTarget{
@@ -82,13 +82,13 @@ func TestReconcileIngressUpdate(t *testing.T) {
 			},
 		})
 	})
-	if _, err := reconciler.reconcileIngress(ctx, r, ci2); err != nil {
+	if _, err := reconciler.reconcileIngress(ctx, r, ci2, tc); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
 	updated = getRouteIngressFromClient(ctx, t, r)
 	if diff := cmp.Diff(ci2, updated); diff != "" {
-		t.Error("Unexpected diff (-want +got):", diff)
+		t.Errorf("Unexpected diff (-want +got):\n%s", diff)
 	}
 	if diff := cmp.Diff(ci, updated); diff == "" {
 		t.Error("Expected difference, but found none")
@@ -197,7 +197,7 @@ func getLastPinnedTimestamp(t *testing.T, rev *v1.Revision) (string, error) {
 	return lastPinnedTime, nil
 }
 
-func newTestIngress(t *testing.T, r *v1.Route, trafficOpts ...func(tc *traffic.Config)) *netv1alpha1.Ingress {
+func newTestIngress(t *testing.T, r *v1.Route, trafficOpts ...func(tc *traffic.Config)) (*netv1alpha1.Ingress, *traffic.Config) {
 	tc := &traffic.Config{Targets: map[string]traffic.RevisionTargets{
 		traffic.DefaultTarget: {{
 			TrafficTarget: v1.TrafficTarget{
@@ -219,7 +219,7 @@ func newTestIngress(t *testing.T, r *v1.Route, trafficOpts ...func(tc *traffic.C
 	if err != nil {
 		t.Error("Unexpected error:", err)
 	}
-	return ingress
+	return ingress, tc
 }
 
 func TestReconcileIngressClassAnnotation(t *testing.T) {
@@ -232,19 +232,19 @@ func TestReconcileIngressClassAnnotation(t *testing.T) {
 	const expClass = "foo.ingress.networking.knative.dev"
 
 	r := Route("test-ns", "test-route")
-	ci := newTestIngress(t, r)
-	if _, err := reconciler.reconcileIngress(ctx, r, ci); err != nil {
+	ci, tc := newTestIngress(t, r)
+	if _, err := reconciler.reconcileIngress(ctx, r, ci, tc); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
 	updated := getRouteIngressFromClient(ctx, t, r)
 	fakeciinformer.Get(ctx).Informer().GetIndexer().Add(updated)
 
-	ci2 := newTestIngress(t, r)
+	ci2, tc := newTestIngress(t, r)
 	// Add ingress.class annotation.
 	ci2.Annotations[networking.IngressClassAnnotationKey] = expClass
 
-	if _, err := reconciler.reconcileIngress(ctx, r, ci2); err != nil {
+	if _, err := reconciler.reconcileIngress(ctx, r, ci2, tc); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
diff --git a/pkg/reconciler/route/resources/ingress_test.go b/pkg/reconciler/route/resources/ingress_test.go
index 4c894bce3..2a77cd15d 100644
--- a/pkg/reconciler/route/resources/ingress_test.go
+++ b/pkg/reconciler/route/resources/ingress_test.go
@@ -51,8 +51,6 @@ const (
 	testRouteName       = "test-route"
 	testAnnotationValue = "test-annotation-value"
 	testIngressClass    = "test-ingress"
-
-	emptyRollout = "{}"
 )
 
 func TestMakeIngressCorrectMetadata(t *testing.T) {
@@ -81,7 +79,6 @@ func TestMakeIngressCorrectMetadata(t *testing.T) {
 			// Make sure to get passdownIngressClass instead of ingressClass
 			networking.IngressClassAnnotationKey: passdownIngressClass,
 			"test-annotation":                    "bar",
-			networking.RolloutAnnotationKey:      emptyRollout,
 		},
 		OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)},
 	}
@@ -133,7 +130,6 @@ func TestMakeIngressWithRollout(t *testing.T) {
 			// Make sure to get passdownIngressClass instead of ingressClass
 			networking.IngressClassAnnotationKey: passdownIngressClass,
 			"test-annotation":                    "bar",
-			networking.RolloutAnnotationKey:      serializeRollout(context.Background(), cfg.BuildRollout()),
 		},
 		OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)},
 	}
@@ -922,7 +918,6 @@ func TestMakeIngressWithTLS(t *testing.T) {
 			Namespace: ns,
 			Annotations: map[string]string{
 				networking.IngressClassAnnotationKey: ingressClass,
-				networking.RolloutAnnotationKey:      emptyRollout,
 			},
 			Labels: map[string]string{
 				serving.RouteLabelKey:          "test-route",
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 95cbbe892..054439d1e 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -713,7 +713,11 @@ func TestReconcile(t *testing.T) {
 						}},
 					},
 				},
-			),
+				simpleRollout("config", []traffic.RevisionRollout{{
+					RevisionName: "config-00001", Percent: 99,
+				}, {
+					RevisionName: "config-00002", Percent: 1,
+				}})),
 		}},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Route("default", "new-latest-ready", WithConfigTarget("config"),
@@ -913,7 +917,11 @@ func TestReconcile(t *testing.T) {
 						}},
 					},
 				},
-			),
+				simpleRollout("config", []traffic.RevisionRollout{{
+					RevisionName: "config-00001", Percent: 99,
+				}, {
+					RevisionName: "config-00002", Percent: 1,
+				}})),
 		}},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Route("default", "update-ci-failure", WithConfigTarget("config"),
@@ -2555,6 +2563,12 @@ func ingressWithClass(r *v1.Route, tc *traffic.Config, class string, io ...Ingre
 func baseIngressWithClass(r *v1.Route, tc *traffic.Config, class string, io ...IngressOption) *netv1alpha1.Ingress {
 	ingress, _ := resources.MakeIngress(getContext(), r, tc, nil, class)
 
+	// By default attach current rollout.
+	ro := tc.BuildRollout()
+	ingress.Annotations = kmeta.UnionMaps(ingress.Annotations, map[string]string{
+		networking.RolloutAnnotationKey: serializeRollout(context.Background(), ro),
+	})
+
 	for _, opt := range io {
 		opt(ingress)
 	}
@@ -2569,6 +2583,12 @@ func ingressWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTL
 func baseIngressWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTLS, challenges []netv1alpha1.HTTP01Challenge, io ...IngressOption) *netv1alpha1.Ingress {
 	ingress, _ := resources.MakeIngress(getContext(), r, tc, tls, TestIngressClass, challenges...)
 
+	// By default attach current rollout.
+	ro := tc.BuildRollout()
+	ingress.Annotations = kmeta.UnionMaps(ingress.Annotations, map[string]string{
+		networking.RolloutAnnotationKey: serializeRollout(context.Background(), ro),
+	})
+
 	for _, opt := range io {
 		opt(ingress)
 	}
@@ -2724,3 +2744,17 @@ func setResponsiveGCFeature(ctx context.Context, flag cfgmap.Flag) context.Conte
 	c.Features.ResponsiveRevisionGC = flag
 	return cfgmap.ToContext(ctx, c)
 }
+
+func simpleRollout(cfg string, revs []traffic.RevisionRollout) IngressOption {
+	return func(i *netv1alpha1.Ingress) {
+		r := &traffic.Rollout{
+			Configurations: []traffic.ConfigurationRollout{{
+				ConfigurationName: cfg,
+				Percent:           100,
+				Revisions:         revs,
+			}},
+		}
+		ro := serializeRollout(context.Background(), r)
+		i.Annotations[networking.RolloutAnnotationKey] = ro
+	}
+}
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index 863b2e870..6f610d74c 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -63,6 +63,10 @@ var (
 	unreadyConfig *v1.Configuration
 	unreadyRev    *v1.Revision
 
+	readyUnreadyConfig *v1.Configuration
+	readyRevision      *v1.Revision
+	newUnreadyRev      *v1.Revision
+
 	// failedConfig only has failedRev, and it fails to be ready.
 	failedConfig *v1.Configuration
 	failedRev    *v1.Revision
@@ -94,6 +98,7 @@ func setUp() {
 	emptyConfig = getTestEmptyConfig("empty")
 	revDeletedConfig = testConfigWithDeletedRevision("latest-rev-deleted")
 	unreadyConfig, unreadyRev = getTestUnreadyConfig("unready")
+	readyUnreadyConfig, readyRevision, newUnreadyRev = createReadyUnreadyConfig("ready-unready")
 	failedConfig, failedRev = getTestFailedConfig("failed")
 	inactiveConfig, inactiveRev = getTestInactiveConfig("inactive")
 	goodConfig, goodOldRev, goodNewRev = getTestReadyConfig("good")
@@ -117,6 +122,7 @@ func setUp() {
 		emptyConfig,
 		goodConfig, goodOldRev, goodNewRev,
 		niceConfig, niceOldRev, niceNewRev,
+		readyUnreadyConfig, readyRevision, newUnreadyRev,
 	}
 
 	for _, obj := range objs {
@@ -1136,6 +1142,65 @@ func TestBuildTrafficConfigurationNotRoutableRevision(t *testing.T) {
 	}
 }
 
+func TestBuildTrafficConfigurationReadyNotReadyConfig(t *testing.T) {
+	expected := &Config{
+		Targets: map[string]RevisionTargets{
+			DefaultTarget: {{
+				TrafficTarget: v1.TrafficTarget{
+					RevisionName:      readyRevision.Name,
+					ConfigurationName: readyUnreadyConfig.Name,
+					LatestRevision:    ptr.Bool(true),
+					Percent:           ptr.Int64(100),
+				},
+				Protocol: net.ProtocolHTTP1,
+			}},
+		},
+		revisionTargets: []RevisionTarget{{
+			TrafficTarget: v1.TrafficTarget{
+				RevisionName:      readyRevision.Name,
+				ConfigurationName: readyUnreadyConfig.Name,
+				LatestRevision:    ptr.Bool(true),
+				Percent:           ptr.Int64(100),
+			},
+			Protocol: net.ProtocolHTTP1,
+		}},
+		Revisions: map[string]*v1.Revision{
+			readyRevision.Name: readyRevision,
+		},
+		Configurations: map[string]*v1.Configuration{
+			readyUnreadyConfig.Name: readyUnreadyConfig,
+		},
+	}
+	tc, err := BuildTrafficConfiguration(
+		configLister, revLister,
+		testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
+			ConfigurationName: readyUnreadyConfig.Name,
+			LatestRevision:    ptr.Bool(true),
+			Percent:           ptr.Int64(100),
+		},
+		)))
+	if err != nil {
+		t.Fatal("BuildTrafficConfiguration =", err)
+	}
+	if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
+		t.Errorf("Incorrect trafficm diff:(-want +got):\n%s", cmp.Diff(want, got, cmpOpts...))
+	}
+	gotR := tc.BuildRollout()
+	wantR := &Rollout{
+		Configurations: []ConfigurationRollout{{
+			ConfigurationName: readyUnreadyConfig.Name,
+			Percent:           100,
+			Revisions: []RevisionRollout{{
+				RevisionName: readyRevision.Name,
+				Percent:      100,
+			}},
+		}},
+	}
+	if !cmp.Equal(gotR, wantR) {
+		t.Errorf("Rollout mismatch, diff(-want,+got):\n%s", cmp.Diff(wantR, gotR))
+	}
+}
+
 func TestBuildTrafficConfigurationNotRoutableConfiguration(t *testing.T) {
 	expected := &Config{
 		Targets:        map[string]RevisionTargets{},
@@ -1358,8 +1423,23 @@ func testConfigWithDeletedRevision(name string) *v1.Configuration {
 	return config
 }
 
+func createReadyUnreadyConfig(name string) (*v1.Configuration, *v1.Revision, *v1.Revision) {
+	config := testConfig(name + "-config")
+
+	rev1 := testRevForConfig(config, name+"-revision-1")
+	rev1.Status.MarkResourcesAvailableTrue()
+	rev1.Status.MarkContainerHealthyTrue()
+	rev1.Status.MarkActiveTrue()
+	config.Status.SetLatestReadyRevisionName(rev1.Name)
+
+	rev2 := testRevForConfig(config, name+"-revision")
+	config.Status.SetLatestCreatedRevisionName(rev2.Name)
+	return config, rev1, rev2
+}
+
 func getTestUnreadyConfig(name string) (*v1.Configuration, *v1.Revision) {
 	config := testConfig(name + "-config")
+
 	rev := testRevForConfig(config, name+"-revision")
 	config.Status.SetLatestCreatedRevisionName(rev.Name)
 	return config, rev