Repository: knative/serving
Pull request #10839 (2021-02-23 21:17:22)

More deep cleaning of the Route reconciler it needed for some time.
- remove some double O(N) operations, where one was enough
- reflow rollout code so that all `rolloutInProgress` pieces are next to
  each other. I don't recall why I didn't do this in the first place.
- this led to removing some unneeded public functions, and instead we
  have nice inline code now. Test coverage is basically same :D
- remove debug printfs
- improve somec constants

/assign @tcnghia @ZhiminXiang 

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **vagababov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @vagababov` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

<details open>
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)**

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"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10839?src=pr&el=h1) Report
> Merging [#10839](https://codecov.io/gh/knative/serving/pull/10839?src=pr&el=desc) (389e593) into [master](https://codecov.io/gh/knative/serving/commit/a675c759ac7ab42b187194d33277769d1408a640?el=desc) (a675c75) will **decrease** coverage by `0.02%`.
> The diff coverage is `92.30%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10839      +/-   ##
==========================================
- Coverage   88.05%   88.03%   -0.03%     
==========================================
  Files         188      188              
  Lines        9044     9036       -8     
==========================================
- Hits         7964     7955       -9     
  Misses        829      829              
- Partials      251      252       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10839?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cmd/activator/main.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-Y21kL2FjdGl2YXRvci9tYWluLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/activator/handler/context.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9oYW5kbGVyL2NvbnRleHQuZ28=) | `100.00% <ø> (ø)` | |
| [pkg/reconciler/route/resources/service.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVzb3VyY2VzL3NlcnZpY2UuZ28=) | `91.25% <ø> (-0.42%)` | :arrow_down: |
| [pkg/reconciler/route/route.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcm91dGUuZ28=) | `76.43% <88.88%> (+0.12%)` | :arrow_up: |
| [cmd/activator/request\_log.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-Y21kL2FjdGl2YXRvci9yZXF1ZXN0X2xvZy5nbw==) | `86.95% <100.00%> (-1.94%)` | :arrow_down: |
| [pkg/activator/handler/context\_handler.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9oYW5kbGVyL2NvbnRleHRfaGFuZGxlci5nbw==) | `96.29% <100.00%> (ø)` | |
| [pkg/activator/handler/metric\_handler.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9oYW5kbGVyL21ldHJpY19oYW5kbGVyLmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/route/controller.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvY29udHJvbGxlci5nbw==) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `81.60% <100.00%> (-0.15%)` | :arrow_down: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10839/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `85.93% <0.00%> (-0.79%)` | :arrow_down: |

------

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

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **tcnghia** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @tcnghia` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

<details open>
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)**

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":["tcnghia"]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10839#pullrequestreview-596824906" title="LGTM">tcnghia</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)~~ [tcnghia]

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":[]} -->

Patch:
diff --git a/pkg/reconciler/route/controller.go b/pkg/reconciler/route/controller.go
index 721015cc7..fdcf50202 100644
--- a/pkg/reconciler/route/controller.go
+++ b/pkg/reconciler/route/controller.go
@@ -47,12 +47,12 @@ func NewController(
 	ctx context.Context,
 	cmw configmap.Watcher,
 ) *controller.Impl {
-	return newControllerWithClock(ctx, cmw, clock.RealClock{})
+	return newController(ctx, cmw, clock.RealClock{})
 }
 
 type reconcilerOption func(*Reconciler)
 
-func newControllerWithClock(
+func newController(
 	ctx context.Context,
 	cmw configmap.Watcher,
 	clock clock.Clock,
diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index 552511bd5..cbd182132 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -120,11 +120,10 @@ func (c *Reconciler) reconcilePlaceholderServices(ctx context.Context, route *v1
 	logger := logging.FromContext(ctx)
 	recorder := controller.GetEventRecorder(ctx)
 
-	existingServices, err := c.getServices(route)
+	existingServiceNames, err := c.getPlaceholderServiceNames(route)
 	if err != nil {
 		return nil, fmt.Errorf("failed to fetch existing services: %w", err)
 	}
-	existingServiceNames := resources.GetNames(existingServices)
 
 	ns := route.Namespace
 	names := make(sets.String, len(targets))
@@ -134,6 +133,7 @@ func (c *Reconciler) reconcilePlaceholderServices(ctx context.Context, route *v1
 
 	services := make([]*corev1.Service, 0, names.Len())
 	createdServiceNames := make(sets.String, names.Len())
+	// NB: we don't need to sort here, but it makes table tests work...
 	for _, name := range names.List() {
 		desiredService, err := resources.MakeK8sPlaceholderService(ctx, route, name)
 		if err != nil {
@@ -267,7 +267,7 @@ func (c *Reconciler) reconcileRollout(
 	if nextStepTime > 0 {
 		nextStepTime -= now
 		c.enqueueAfter(r, time.Duration(nextStepTime))
-		logger.Debug("Re-enqueuing after", zap.Duration("duration", time.Duration(nextStepTime)))
+		logger.Debug("Re-enqueuing after", zap.Duration("nextStepTime", time.Duration(nextStepTime)))
 	}
 
 	// Comparing and diffing isn't cheap so do it only if we're going
diff --git a/pkg/reconciler/route/resources/service.go b/pkg/reconciler/route/resources/service.go
index 5d956639c..8d53a8d18 100644
--- a/pkg/reconciler/route/resources/service.go
+++ b/pkg/reconciler/route/resources/service.go
@@ -22,9 +22,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/apimachinery/pkg/util/sets"
 
 	network "knative.dev/networking/pkg"
 	"knative.dev/networking/pkg/apis/networking"
@@ -37,22 +35,6 @@ import (
 
 var errLoadBalancerNotFound = errors.New("failed to fetch loadbalancer domain/IP from ingress status")
 
-// GetNames returns a set of service names.
-func GetNames(services []*corev1.Service) sets.String {
-	names := make(sets.String, len(services))
-
-	for i := range services {
-		names.Insert(services[i].Name)
-	}
-
-	return names
-}
-
-// SelectorFromRoute creates a label selector given a specific route.
-func SelectorFromRoute(route *v1.Route) labels.Selector {
-	return labels.SelectorFromSet(labels.Set{serving.RouteLabelKey: route.Name})
-}
-
 // MakeK8sPlaceholderService creates a placeholder Service to prevent naming collisions. It's owned by the
 // provided v1.Route.
 func MakeK8sPlaceholderService(ctx context.Context, route *v1.Route, targetName string) (*corev1.Service, error) {
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index 02279d6ed..b0aefd33e 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -24,7 +24,6 @@ import (
 	"strings"
 	"time"
 
-	corev1 "k8s.io/api/core/v1"
 	kubelabels "k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/clock"
@@ -42,6 +41,7 @@ import (
 	"knative.dev/pkg/logging"
 	pkgreconciler "knative.dev/pkg/reconciler"
 	"knative.dev/pkg/tracker"
+	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	clientset "knative.dev/serving/pkg/client/clientset/versioned"
 	routereconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/route"
@@ -92,18 +92,21 @@ func certClass(ctx context.Context, r *v1.Route) string {
 	return config.FromContext(ctx).Network.DefaultCertificateClass
 }
 
-func (c *Reconciler) getServices(route *v1.Route) ([]*corev1.Service, error) {
-	currentServices, err := c.serviceLister.Services(route.Namespace).List(resources.SelectorFromRoute(route))
+// getPlaceholderServiceNames returns the placeholder services names, or an error.
+func (c *Reconciler) getPlaceholderServiceNames(route *v1.Route) (sets.String, error) {
+	currentServices, err := c.serviceLister.Services(route.Namespace).List(
+		kubelabels.SelectorFromSet(kubelabels.Set{serving.RouteLabelKey: route.Name}),
+	)
 	if err != nil {
 		return nil, err
 	}
 
-	serviceCopy := make([]*corev1.Service, len(currentServices))
-	for i, svc := range currentServices {
-		serviceCopy[i] = svc.DeepCopy()
+	names := make(sets.String, len(currentServices))
+	for _, svc := range currentServices {
+		names.Insert(svc.Name)
 	}
 
-	return serviceCopy, nil
+	return names, nil
 }
 
 // ReconcileKind implements Interface.ReconcileKind.
@@ -155,11 +158,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil
 	roInProgress := !effectiveRO.Done()
 	if ingress.GetObjectMeta().GetGeneration() != ingress.Status.ObservedGeneration {
 		r.Status.MarkIngressNotConfigured()
-	} else if roInProgress {
-		logger.Info("Rollout is in progress")
-		// Rollout in progress, so mark the status as such.
-		r.Status.MarkIngressRolloutInProgress()
-	} else {
+	} else if !roInProgress {
 		r.Status.PropagateIngressStatus(ingress.Status)
 	}
 
@@ -171,8 +170,11 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil
 	// 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.
+	// will not be triggered.
 	if roInProgress {
+		logger.Info("Rollout is in progress")
+		// Rollout in progress, so mark the status as such.
+		r.Status.MarkIngressRolloutInProgress()
 		// Update the route.Status.Traffic to contain correct traffic
 		// distribution based on rollout status.
 		r.Status.Traffic, err = traffic.GetRevisionTrafficTargets(ctx, r, effectiveRO)


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index 767b3a903..51d35b2af 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -165,12 +165,13 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 			ing = updated
 
 			// Advance the time by 5s.
-			fakeClock.SetTime(fakeClock.Now().Add(5 * time.Second))
+			const drift = 5
+			fakeClock.SetTime(fakeClock.Now().Add(drift * time.Second))
 
-			// 5s of the duration already spent.
-			totalDuration := float64(rd - 5)
+			// Drift seconds of the duration already spent.
+			totalDuration := float64(rd - drift)
 			// Step count is minimum of % points of traffic allocated to the config (1% already shifted).
-			steps := math.Min(totalDuration/5, allocatedTraffic-1)
+			steps := math.Min(totalDuration/drift, allocatedTraffic-1)
 			stepSize := math.Max(1, math.Round((allocatedTraffic-1)/steps)) // we round the step size.
 			stepDuration := time.Duration(int(totalDuration * float64(time.Second) / steps))
 
@@ -270,7 +271,7 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) {
 				wasReenqueued = true
 				duration = d
 			}
-			ctx := updateContext(baseCtx, 311) // This should be ignored, since we set annotation.
+			ctx := updateContext(baseCtx, 311 /*rolloutDuration*/) // This should be ignored, since we set annotation.
 			_, ro, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class")
 			if err != nil {
 				t.Fatal("Unexpected error:", err)
@@ -326,12 +327,13 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) {
 			ing = updated
 
 			// Advance the time by 5s.
-			fakeClock.SetTime(fakeClock.Now().Add(5 * time.Second))
+			const drift = 5
+			fakeClock.SetTime(fakeClock.Now().Add(drift * time.Second))
 
-			// 5s of the duration already spent.
-			totalDuration := float64(rd - 5)
+			// Drift seconds of the duration already spent.
+			totalDuration := float64(rd - drift)
 			// Step count is minimum of % points of traffic allocated to the config (1% already shifted).
-			steps := math.Min(totalDuration/5, allocatedTraffic-1)
+			steps := math.Min(totalDuration/drift, allocatedTraffic-1)
 			stepSize := math.Max(1, math.Round((allocatedTraffic-1)/steps)) // we round the step size.
 			stepDuration := time.Duration(int(totalDuration * float64(time.Second) / steps))
 
diff --git a/pkg/reconciler/route/resources/service_test.go b/pkg/reconciler/route/resources/service_test.go
index 7a61b73aa..df4a5699e 100644
--- a/pkg/reconciler/route/resources/service_test.go
+++ b/pkg/reconciler/route/resources/service_test.go
@@ -24,9 +24,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/apimachinery/pkg/util/sets"
 
 	network "knative.dev/networking/pkg"
 	"knative.dev/networking/pkg/apis/networking"
@@ -356,13 +354,6 @@ func TestMakeK8sPlaceholderService(t *testing.T) {
 	}
 }
 
-func TestSelectorFromRoute(t *testing.T) {
-	selector := SelectorFromRoute(r)
-	if !selector.Matches(labels.Set{serving.RouteLabelKey: r.Name}) {
-		t.Errorf("Unexpected labels in selector")
-	}
-}
-
 func testConfig() *config.Config {
 	return &config.Config{
 		Domain: &config.Domain{
@@ -390,41 +381,3 @@ func testConfig() *config.Config {
 		},
 	}
 }
-
-func TestGetNames(t *testing.T) {
-	tests := []struct {
-		name     string
-		services []*corev1.Service
-		want     sets.String
-	}{
-		{
-			name: "nil services",
-			want: sets.String{},
-		},
-		{
-			name: "multiple services",
-			services: []*corev1.Service{
-				{ObjectMeta: metav1.ObjectMeta{Name: "svc1"}},
-				{ObjectMeta: metav1.ObjectMeta{Name: "svc2"}},
-				{ObjectMeta: metav1.ObjectMeta{Name: "svc3"}},
-			},
-			want: sets.NewString("svc1", "svc2", "svc3"),
-		},
-		{
-			name: "duplicate services",
-			services: []*corev1.Service{
-				{ObjectMeta: metav1.ObjectMeta{Name: "svc1"}},
-				{ObjectMeta: metav1.ObjectMeta{Name: "svc1"}},
-				{ObjectMeta: metav1.ObjectMeta{Name: "svc1"}},
-			},
-			want: sets.NewString("svc1"),
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := GetNames(tt.services); !cmp.Equal(got, tt.want) {
-				t.Error("GetNames() (-want, +got) =", cmp.Diff(tt.want, got))
-			}
-		})
-	}
-}
diff --git a/pkg/reconciler/route/route_test.go b/pkg/reconciler/route/route_test.go
index b8b451c01..efe05a5d9 100644
--- a/pkg/reconciler/route/route_test.go
+++ b/pkg/reconciler/route/route_test.go
@@ -82,9 +82,9 @@ func testConfiguration() *v1.Configuration {
 	}
 }
 
-func revisionForConfig(config *v1.Configuration) *v1.Revision {
+func testRevision() *v1.Revision {
 	return Revision(testNamespace, "p-deadbeef", func(r *v1.Revision) {
-		r.Spec = *config.Spec.GetTemplate().Spec.DeepCopy()
+		r.Spec = *(&v1.ConfigurationSpec{}).GetTemplate().Spec.DeepCopy()
 	}, MarkRevisionReady, WithK8sServiceName)
 }
 
@@ -97,7 +97,7 @@ func newTestSetup(t *testing.T, opts ...reconcilerOption) (
 
 	ctx, cf, informers = SetupFakeContextWithCancel(t)
 	configMapWatcher = &configmap.ManualWatcher{Namespace: system.Namespace()}
-	ctrl = newControllerWithClock(ctx, configMapWatcher, &clock.RealClock{}, opts...)
+	ctrl = newController(ctx, configMapWatcher, &clock.RealClock{}, opts...)
 
 	for _, cfg := range []*corev1.ConfigMap{{
 		ObjectMeta: metav1.ObjectMeta{
@@ -284,7 +284,7 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) {
 			t.Errorf("<-Events = %s, wanted %s", got, want)
 		}
 	case <-time.After(3 * time.Second):
-		t.Error("timed out waiting for expected events.")
+		t.Error("Timed out waiting for expected events.")
 	}
 	select {
 	case got := <-fakeRecorder.Events:
@@ -293,7 +293,7 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) {
 			t.Errorf("<-Events = %s, wanted prefix %s", got, wantPrefix)
 		}
 	case <-time.After(3 * time.Second):
-		t.Error("timed out waiting for expected events.")
+		t.Error("Timed out waiting for expected events.")
 	}
 }
 
@@ -315,7 +315,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) {
 	// A configuration and associated revision. Normally the revision would be
 	// created by the configuration reconciler.
 	config := testConfiguration()
-	cfgrev := revisionForConfig(config)
+	cfgrev := testRevision()
 	config.Status.SetLatestCreatedRevisionName(cfgrev.Name)
 	config.Status.SetLatestReadyRevisionName(cfgrev.Name)
 	fakeservingclient.Get(ctx).ServingV1().Configurations(testNamespace).Create(ctx, config, metav1.CreateOptions{})
@@ -431,7 +431,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) {
 	// A configuration and associated revision. Normally the revision would be
 	// created by the configuration reconciler.
 	config := testConfiguration()
-	cfgrev := revisionForConfig(config)
+	cfgrev := testRevision()
 	config.Status.SetLatestCreatedRevisionName(cfgrev.Name)
 	config.Status.SetLatestReadyRevisionName(cfgrev.Name)
 	fakeservingclient.Get(ctx).ServingV1().Configurations(testNamespace).Create(ctx, config, metav1.CreateOptions{})
@@ -545,7 +545,7 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) {
 	// A configuration and associated revision. Normally the revision would be
 	// created by the configuration reconciler.
 	config := testConfiguration()
-	cfgrev := revisionForConfig(config)
+	cfgrev := testRevision()
 	config.Status.SetLatestCreatedRevisionName(cfgrev.Name)
 	config.Status.SetLatestReadyRevisionName(cfgrev.Name)
 	fakeservingclient.Get(ctx).ServingV1().Configurations(testNamespace).Create(ctx, config, metav1.CreateOptions{})
@@ -749,7 +749,6 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) {
 	}
 
 	if diff := cmp.Diff(expectedSpec, ci.Spec); diff != "" {
-		fmt.Printf("%+v\n", ci.Spec)
 		t.Error("Unexpected rule spec diff (-want +got):", diff)
 	}
 }
@@ -765,7 +764,7 @@ func TestCreateRouteWithNamedTargets(t *testing.T) {
 	// A configuration and associated revision. Normally the revision would be
 	// created by the configuration reconciler.
 	config := testConfiguration()
-	cfgrev := revisionForConfig(config)
+	cfgrev := testRevision()
 	config.Status.SetLatestCreatedRevisionName(cfgrev.Name)
 	config.Status.SetLatestReadyRevisionName(cfgrev.Name)
 	fakeservingclient.Get(ctx).ServingV1().Configurations(testNamespace).Create(ctx, config, metav1.CreateOptions{})
@@ -832,9 +831,7 @@ func TestCreateRouteWithNamedTargets(t *testing.T) {
 			},
 			Visibility: v1alpha1.IngressVisibilityClusterLocal,
 		}, {
-			Hosts: []string{
-				domain,
-			},
+			Hosts: []string{domain},
 			HTTP: &v1alpha1.HTTPIngressRuleValue{
 				Paths: []v1alpha1.HTTPIngressPath{{
 					Splits: []v1alpha1.IngressBackendSplit{{
@@ -887,9 +884,7 @@ func TestCreateRouteWithNamedTargets(t *testing.T) {
 			},
 			Visibility: v1alpha1.IngressVisibilityClusterLocal,
 		}, {
-			Hosts: []string{
-				"bar-test-route.test.test-domain.dev",
-			},
+			Hosts: []string{"bar-test-route.test.test-domain.dev"},
 			HTTP: &v1alpha1.HTTPIngressRuleValue{
 				Paths: []v1alpha1.HTTPIngressPath{{
 					Splits: []v1alpha1.IngressBackendSplit{{
@@ -931,9 +926,7 @@ func TestCreateRouteWithNamedTargets(t *testing.T) {
 			},
 			Visibility: v1alpha1.IngressVisibilityClusterLocal,
 		}, {
-			Hosts: []string{
-				"foo-test-route.test.test-domain.dev",
-			},
+			Hosts: []string{"foo-test-route.test.test-domain.dev"},
 			HTTP: &v1alpha1.HTTPIngressRuleValue{
 				Paths: []v1alpha1.HTTPIngressPath{{
 					Splits: []v1alpha1.IngressBackendSplit{{
@@ -980,7 +973,7 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) {
 	// A configuration and associated revision. Normally the revision would be
 	// created by the configuration reconciler.
 	config := testConfiguration()
-	cfgrev := revisionForConfig(config)
+	cfgrev := testRevision()
 	config.Status.SetLatestCreatedRevisionName(cfgrev.Name)
 	config.Status.SetLatestReadyRevisionName(cfgrev.Name)
 	fakeservingclient.Get(ctx).ServingV1().Configurations(testNamespace).Create(ctx, config, metav1.CreateOptions{})
@@ -1089,9 +1082,7 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) {
 			},
 			Visibility: v1alpha1.IngressVisibilityClusterLocal,
 		}, {
-			Hosts: []string{
-				domain,
-			},
+			Hosts: []string{domain},
 			HTTP: &v1alpha1.HTTPIngressRuleValue{
 				Paths: []v1alpha1.HTTPIngressPath{{
 					Headers: map[string]v1alpha1.HeaderMatch{
@@ -1240,9 +1231,7 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) {
 			},
 			Visibility: v1alpha1.IngressVisibilityClusterLocal,
 		}, {
-			Hosts: []string{
-				"foo-test-route.test.test-domain.dev",
-			},
+			Hosts: []string{"foo-test-route.test.test-domain.dev"},
 			HTTP: &v1alpha1.HTTPIngressRuleValue{
 				Paths: []v1alpha1.HTTPIngressPath{{
 					Splits: []v1alpha1.IngressBackendSplit{{
@@ -1267,7 +1256,6 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) {
 	}
 
 	if diff := cmp.Diff(expectedSpec, ci.Spec); diff != "" {
-		fmt.Printf("%+v\n", ci.Spec)
 		t.Error("Unexpected rule spec diff (-want +got):", diff)
 	}
 }