Repository: knative/serving
Pull request #10401 (2020-12-17 19:35:18)

Return the actual applied rollout from reconcile resources
This will be used to compute Route status.traffic and Route.status.Ready

/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/10401#" 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":[]} -->
vagababov: For #10402
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10401?src=pr&el=h1) Report
> Merging [#10401](https://codecov.io/gh/knative/serving/pull/10401?src=pr&el=desc) (54e435b) into [master](https://codecov.io/gh/knative/serving/commit/87e635f76208b63b710f341e014a843675d7fd90?el=desc) (87e635f) will **decrease** coverage by `0.03%`.
> The diff coverage is `75.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10401      +/-   ##
==========================================
- Coverage   88.05%   88.02%   -0.04%     
==========================================
  Files         186      186              
  Lines        8756     8757       +1     
==========================================
- Hits         7710     7708       -2     
- Misses        809      811       +2     
- Partials      237      238       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10401?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10401/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `80.83% <72.72%> (+0.16%)` | :arrow_up: |
| [pkg/reconciler/route/route.go](https://codecov.io/gh/knative/serving/pull/10401/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcm91dGUuZ28=) | `76.24% <100.00%> (ø)` | |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/10401/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9mb3J3YXJkZXIuZ28=) | `90.74% <0.00%> (-5.56%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/processor.go](https://codecov.io/gh/knative/serving/pull/10401/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9wcm9jZXNzb3IuZ28=) | `83.33% <0.00%> (-5.56%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/leases.go](https://codecov.io/gh/knative/serving/pull/10401/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9sZWFzZXMuZ28=) | `78.41% <0.00%> (+1.43%)` | :arrow_up: |

------

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

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

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

Patch:
diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index ea0fcec80..e5dd242ed 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -26,6 +26,7 @@ import (
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 	"golang.org/x/sync/errgroup"
+
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
@@ -48,30 +49,31 @@ func (c *Reconciler) reconcileIngress(
 	tls []netv1alpha1.IngressTLS,
 	ingressClass string,
 	acmeChallenges ...netv1alpha1.HTTP01Challenge,
-) (*netv1alpha1.Ingress, error) {
+) (*netv1alpha1.Ingress, *traffic.Rollout, error) {
 	recorder := controller.GetEventRecorder(ctx)
+	var effectiveRO *traffic.Rollout
 
 	ingress, err := c.ingressLister.Ingresses(r.Namespace).Get(names.Ingress(r))
 	if apierrs.IsNotFound(err) {
 		desired, err := resources.MakeIngress(ctx, r, tc, tls, ingressClass, acmeChallenges...)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		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)
-			return nil, fmt.Errorf("failed to create Ingress: %w", err)
+			return nil, nil, fmt.Errorf("failed to create Ingress: %w", err)
 		}
 
 		recorder.Eventf(r, corev1.EventTypeNormal, "Created", "Created Ingress %q", ingress.GetName())
-		return ingress, nil
+		return ingress, tc.BuildRollout(), nil
 	} else if err != nil {
-		return nil, err
+		return nil, nil, err
 	} else {
 		// Ingress exists. We need to compute the rollout spec diff.
 		// Get the current rollout state as described by the traffic.
 		curRO := tc.BuildRollout()
-		effectiveRO := curRO
+		effectiveRO = curRO
 		nextStepTime := int64(0)
 		cfg := config.FromContext(ctx)
 
@@ -113,7 +115,7 @@ func (c *Reconciler) reconcileIngress(
 		desired, err := resources.MakeIngressWithRollout(ctx, r, tc, effectiveRO,
 			tls, ingressClass, acmeChallenges...)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
 		if !equality.Semantic.DeepEqual(ingress.Spec, desired.Spec) ||
@@ -135,13 +137,13 @@ func (c *Reconciler) reconcileIngress(
 			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 nil, nil, fmt.Errorf("failed to update Ingress: %w", err)
 			}
-			return updated, nil
+			return updated, effectiveRO, nil
 		}
 	}
 
-	return ingress, err
+	return ingress, effectiveRO, err
 }
 
 func (c *Reconciler) deleteServices(ctx context.Context, namespace string, serviceNames sets.String) error {
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index def84686c..4b9e67af7 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -147,7 +147,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil
 	}
 
 	// Reconcile ingress and its children resources.
-	ingress, err := c.reconcileIngress(ctx, r, traffic, tls, ingressClassForRoute(ctx, r), acmeChallenges...)
+	ingress, _, err := c.reconcileIngress(ctx, r, traffic, tls, ingressClassForRoute(ctx, r), acmeChallenges...)
 	if err != nil {
 		return err
 	}


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index c7f47bbf6..4c00c6ac8 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -50,9 +50,13 @@ func TestReconcileIngressInsert(t *testing.T) {
 
 	r := Route("test-ns", "test-route")
 	tc, tls := testIngressParams(t, r)
-	if _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
+	_, ro, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress")
+	if err != nil {
 		t.Error("Unexpected error:", err)
 	}
+	if got, want := ro, tc.BuildRollout(); !cmp.Equal(got, want) {
+		t.Errorf("Default rollout mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got))
+	}
 }
 
 func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
@@ -97,17 +101,23 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 		}}
 	})
 	ctx = updateContext(ctx, 120.0)
-	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class"); err != nil {
+	_, ro, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class")
+	if err != nil {
 		t.Fatal("Unexpected error:", err)
 	}
 
+	if got, want := ro, tc.BuildRollout(); !cmp.Equal(got, want) {
+		t.Errorf("Complex initial rollout mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got))
+	}
+
 	ing := getRouteIngressFromClient(ctx, t, r)
 	fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing)
 
 	// Now we have initial version. Let's make a rollout.
 	tc.Targets[traffic.DefaultTarget][1].RevisionName = "miercoles"
 
-	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class"); err != nil {
+	_, updRO, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class")
+	if err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
@@ -121,11 +131,17 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 	if wasReenqueued {
 		t.Fatal("Unexpected re-enqueuing happened")
 	}
-	ro := deserializeRollout(ctx, updated.Annotations[networking.RolloutAnnotationKey])
+
+	ro = deserializeRollout(ctx, updated.Annotations[networking.RolloutAnnotationKey])
 	if ro == nil {
 		t.Fatal("Rollout was bogus and impossible to deserialize")
 	}
 
+	// Verify the same rollout was returned by the ReconcileResources.
+	if !cmp.Equal(ro, updRO) {
+		t.Errorf("Returned and Annotation Rollouts differ: diff(-ret,+ann):\n%s", cmp.Diff(updRO, ro))
+	}
+
 	// After sorting `odin` will come first.
 	if got, want := ro.Configurations[0].StepParams.StartTime, int64(fakeCurTime); got != want {
 		t.Errorf("Rollout StartTime = %d, want: %d", got, want)
@@ -142,7 +158,8 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 	// Advance the time. This 5s will become step time as well.
 	fakeClock.Time = fakeClock.Time.Add(5 * time.Second)
 
-	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
+	_, updRO, err = reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress")
+	if err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
@@ -157,12 +174,19 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 	if ro == nil {
 		t.Fatal("Rollout was bogus and impossible to deserialize")
 	}
+
 	// Verify NextStepTime and StepSize are set as expected
 	if got, want := ro.Configurations[0].StepParams.NextStepTime,
 		fakeClock.Time.Add(5*time.Second).UnixNano(); got != want {
 		t.Errorf("NextStepTime = %d, want: %d", got, want)
 	}
 
+	// Verify the same rollout was returned by the ReconcileResources.
+	if !cmp.Equal(ro, updRO) {
+		t.Errorf("Returned and Annotation Rollouts after re-enqueue differ: diff(-ret,+ann):\n%s",
+			cmp.Diff(updRO, ro))
+	}
+
 	// Rounding up, so step size should be 2.
 	if got, want := ro.Configurations[0].StepParams.StepSize, 47/24+1; got != want {
 		t.Errorf("StepSize = %d, want: %d", got, want)
@@ -186,7 +210,7 @@ func TestReconcileIngressUpdateNoRollout(t *testing.T) {
 	r := Route("test-ns", "test-route")
 
 	tc, tls := testIngressParams(t, r)
-	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
+	if _, _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
@@ -196,7 +220,7 @@ func TestReconcileIngressUpdateNoRollout(t *testing.T) {
 	tc, tls = testIngressParams(t, r, func(tc *traffic.Config) {
 		tc.Targets[traffic.DefaultTarget][0].RevisionName = "revision2"
 	})
-	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
+	if _, _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
@@ -233,7 +257,7 @@ func TestReconcileIngressUpdate(t *testing.T) {
 	r := Route("test-ns", "test-route")
 
 	tc, tls := testIngressParams(t, r)
-	if _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
+	if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
@@ -249,7 +273,7 @@ func TestReconcileIngressUpdate(t *testing.T) {
 			},
 		})
 	})
-	if _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
+	if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
@@ -303,7 +327,7 @@ func TestReconcileIngressRolloutDeserializeFail(t *testing.T) {
 			ingClient := fakenetworkingclient.Get(ctx).NetworkingV1alpha1().Ingresses(ing.Namespace)
 			ingClient.Create(ctx, ing, metav1.CreateOptions{})
 			fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing)
-			if _, err := reconciler.reconcileIngress(ctx, r, traffic, tls, "foo-ingress"); err != nil {
+			if _, _, err := reconciler.reconcileIngress(ctx, r, traffic, tls, "foo-ingress"); err != nil {
 				t.Error("Unexpected error:", err)
 			}
 			ing, err = ingClient.Get(ctx, ing.Name, metav1.GetOptions{})
@@ -388,14 +412,14 @@ func TestReconcileIngressClassAnnotation(t *testing.T) {
 
 	r := Route("test-ns", "test-route")
 	tc, tls := testIngressParams(t, r)
-	if _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
+	if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil {
 		t.Error("Unexpected error:", err)
 	}
 
 	updated := getRouteIngressFromClient(ctx, t, r)
 	fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(updated)
 
-	if _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, expClass); err != nil {
+	if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, expClass); err != nil {
 		t.Error("Unexpected error:", err)
 	}