Repository: knative/serving
Pull request #10602 (2021-01-21 21:03:46)

Add the code to use the annotation value, if provided, for rollout duration
Add the test to cover that case.

/assign @tcnghia mattmoor @dprotaso 

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10602#" 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:

- ~~[OWNERS](https://github.com/knative/serving/blob/master/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/10602?src=pr&el=h1) Report
> Merging [#10602](https://codecov.io/gh/knative/serving/pull/10602?src=pr&el=desc) (a833efe) into [master](https://codecov.io/gh/knative/serving/commit/d6c9426c0875f1024261759708d161789016c6ae?el=desc) (d6c9426) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10602      +/-   ##
==========================================
+ Coverage   88.19%   88.24%   +0.04%     
==========================================
  Files         186      186              
  Lines        8894     8897       +3     
==========================================
+ Hits         7844     7851       +7     
+ Misses        808      806       -2     
+ Partials      242      240       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10602?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10602/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `81.30% <100.00%> (+0.46%)` | :arrow_up: |
| [pkg/autoscaler/statforwarder/leases.go](https://codecov.io/gh/knative/serving/pull/10602/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9sZWFzZXMuZ28=) | `78.41% <0.00%> (+1.43%)` | :arrow_up: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10602/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/10602?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/10602?src=pr&el=footer). Last update [d6c9426...a673196](https://codecov.io/gh/knative/serving/pull/10602?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-build-tests | a673196b0343054c56a0958a9a1bffe92684a1c6 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10602/pull-knative-serving-build-tests/1352361291695525888) | `/test pull-knative-serving-build-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10602). [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 -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10602#" 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":[]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10602#pullrequestreview-573745774" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10602#" 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 1c5e253ee..189ba1ab8 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -77,9 +77,15 @@ func (c *Reconciler) reconcileIngress(
 		nextStepTime := int64(0)
 		cfg := config.FromContext(ctx)
 
-		if cfg.Network.RolloutDurationSecs > 0 {
+		// Is there rollout duration specified?
+		rd := int(r.RolloutDuration().Seconds())
+		if rd == 0 {
+			// If not, check if there's a cluster-wide default.
+			rd = cfg.Network.RolloutDurationSecs
+		}
+		if rd > 0 {
 			logger := logging.FromContext(ctx).Desugar().With(
-				zap.Int("durationSecs", cfg.Network.RolloutDurationSecs))
+				zap.Int("durationSecs", rd))
 			logger.Debug("Rollout is enabled. Stepping from previous state.")
 			// Get the previous rollout state from the annotation.
 			// If it's corrupt, inexistent, or otherwise incorrect,
@@ -94,7 +100,7 @@ func (c *Reconciler) reconcileIngress(
 			rtView := r.Status.GetCondition(v1.RouteConditionIngressReady)
 			if prevRO != nil && ingress.IsReady() && !rtView.IsTrue() {
 				logger.Debug("Observing Ingress not-ready to ready switch condition for rollout")
-				prevRO.ObserveReady(ctx, now, float64(cfg.Network.RolloutDurationSecs))
+				prevRO.ObserveReady(ctx, now, float64(rd))
 			}
 
 			effectiveRO, nextStepTime = curRO.Step(ctx, prevRO, now)


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index 48025e261..1078964df 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -31,6 +31,7 @@ import (
 	fakeingressinformer "knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/ingress/fake"
 	"knative.dev/pkg/ptr"
 	rtesting "knative.dev/pkg/reconciler/testing"
+	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	fakeservingclient "knative.dev/serving/pkg/client/injection/client/fake"
 	fakerevisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision/fake"
@@ -199,6 +200,149 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 	}
 }
 
+func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) {
+	var reconciler *Reconciler
+	const (
+		fakeCurSecs = 19551982
+		fakeCurNsec = 19841988
+		fakeCurTime = fakeCurSecs*1_000_000_000 + fakeCurNsec // *1e9
+	)
+	fakeClock := &rtesting.FakeClock{Time: time.Unix(fakeCurSecs, fakeCurNsec)}
+	ctx, _, _, _, cancel := newTestSetup(t, func(r *Reconciler) {
+		r.clock = fakeClock
+		reconciler = r
+	})
+	defer cancel()
+	wasReenqueued := false
+	duration := 0 * time.Second
+	reconciler.enqueueAfter = func(_ interface{}, d time.Duration) {
+		wasReenqueued = true
+		duration = d
+	}
+
+	r := Route("test-ns", "rollout-route")
+	r.Annotations = map[string]string{
+		serving.RolloutDurationKey: "120s",
+	}
+
+	tc, tls := testIngressParams(t, r, func(tc *traffic.Config) {
+		tc.Targets[traffic.DefaultTarget] = traffic.RevisionTargets{{
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: "thor",
+				RevisionName:      "thursday",
+				Percent:           ptr.Int64(52),
+				LatestRevision:    ptr.Bool(true),
+			},
+			Protocol: networking.ProtocolHTTP1,
+		}, {
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: "odin",
+				RevisionName:      "wednesday",
+				Percent:           ptr.Int64(48),
+				LatestRevision:    ptr.Bool(true),
+			},
+			Protocol: networking.ProtocolHTTP1,
+		}}
+	})
+	ctx = updateContext(ctx, 180.0) // This value should be ignored.
+	_, 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"
+
+	_, updRO, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class")
+	if err != nil {
+		t.Error("Unexpected error:", err)
+	}
+
+	// This should update the rollout with the new annotation.
+	updated := getRouteIngressFromClient(ctx, t, r)
+
+	if got, want := updated.Annotations[networking.RolloutAnnotationKey],
+		ing.Annotations[networking.RolloutAnnotationKey]; got == want {
+		t.Fatal("Expected difference in the rollout annotation, but found none")
+	}
+	if wasReenqueued {
+		t.Fatal("Unexpected re-enqueuing happened")
+	}
+
+	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)
+	}
+
+	// OK. So now let's observe ready to start the rollout. For that we need to make ingress ready.
+	cp := updated.DeepCopy()
+	cp.Status.MarkLoadBalancerReady(nil, nil)
+	cp.Status.MarkNetworkConfigured()
+	fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(cp)
+	// For comparisons.
+	ing = updated
+
+	// Advance the time. This 5s will become step time as well.
+	fakeClock.Time = fakeClock.Time.Add(5 * time.Second)
+
+	_, updRO, err = reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress")
+	if err != nil {
+		t.Error("Unexpected error:", err)
+	}
+
+	// This should update the rollout with the new annotation with steps and stuff.
+	updated = getRouteIngressFromClient(ctx, t, r)
+
+	if got, want := updated.Annotations[networking.RolloutAnnotationKey],
+		ing.Annotations[networking.RolloutAnnotationKey]; got == want {
+		t.Fatal("Expected difference in the rollout annotation, but found none")
+	}
+	ro = deserializeRollout(ctx, updated.Annotations[networking.RolloutAnnotationKey])
+	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)
+	}
+	if !wasReenqueued {
+		t.Fatal("Re-enqueuing didn't happen")
+	}
+	if duration != 5*time.Second {
+		t.Errorf("Re-enqueue duration = %v, want: 5s", duration)
+	}
+}
+
 func TestReconcileIngressUpdateNoRollout(t *testing.T) {
 	var reconciler *Reconciler
 	ctx, _, _, _, cancel := newTestSetup(t, func(r *Reconciler) {