Repository: knative/serving
Pull request #10365 (2020-12-14 20:31:21)

Add a guard not to step at all if RolloutDuration = 0
This might start rollout when it is not desired, which would be a bug
But also perform work that is not required at all.

Update and enahnce testing and add negative tests, to ensure no
regression might happen.

part of #9766

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10365#" 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: /assign @ZhiminXiang 
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10365?src=pr&el=h1) Report
> Merging [#10365](https://codecov.io/gh/knative/serving/pull/10365?src=pr&el=desc) (ef1b551) into [master](https://codecov.io/gh/knative/serving/commit/00a08be100977249cee97ccc70490b5b19e0da49?el=desc) (00a08be) will **decrease** coverage by `0.04%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10365      +/-   ##
==========================================
- Coverage   88.02%   87.98%   -0.05%     
==========================================
  Files         186      186              
  Lines        8712     8716       +4     
==========================================
  Hits         7669     7669              
- Misses        806      810       +4     
  Partials      237      237              
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10365?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10365/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `80.67% <100.00%> (-7.16%)` | :arrow_down: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10365/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `86.71% <0.00%> (+2.34%)` | :arrow_up: |
| [pkg/reconciler/revision/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10365/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcmV2aXNpb24vcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `83.13% <0.00%> (+2.40%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10365?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/10365?src=pr&el=footer). Last update [00a08be...7e62da4](https://codecov.io/gh/knative/serving/pull/10365?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-tls | 7e62da41659023b1cad6dfc8ef9072b0a6e7e3f6 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10365/pull-knative-serving-istio-stable-no-mesh-tls/1338582402032734208) | `/test pull-knative-serving-istio-stable-no-mesh-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10365). [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: /test pull-knative-serving-istio-stable-no-mesh-tls
vagababov: ```
2020/12/14 21:44:53 Failed to setup DNS record: timed out waiting for the condition"
```
Doesn't seem to be related.
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-tls | 7e62da41659023b1cad6dfc8ef9072b0a6e7e3f6 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10365/pull-knative-serving-istio-stable-no-mesh-tls/1338593947290701824) | `/test pull-knative-serving-istio-stable-no-mesh-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10365). [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: /test pull-knative-serving-istio-stable-no-mesh-tls
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-tls | 7e62da41659023b1cad6dfc8ef9072b0a6e7e3f6 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10365/pull-knative-serving-istio-stable-no-mesh-tls/1338604779181445120) | `/test pull-knative-serving-istio-stable-no-mesh-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10365). [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/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index e0f52ec43..ea0fcec80 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -71,41 +71,45 @@ func (c *Reconciler) reconcileIngress(
 		// 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
+		nextStepTime := int64(0)
+		cfg := config.FromContext(ctx)
 
-		// Get the previous rollout state from the annotation.
-		// If it's corrupt, inexistent, or otherwise incorrect,
-		// the prevRO will be just nil rollout.
-		prevRO := deserializeRollout(ctx,
-			ingress.Annotations[networking.RolloutAnnotationKey])
-
-		// And recompute the rollout state.
-		now := c.clock.Now().UnixNano()
-
-		// Now check if the ingress status changed from not ready to ready.
-		rtView := r.Status.GetCondition(v1.RouteConditionIngressReady)
-		logger := logging.FromContext(ctx).Desugar()
-		if prevRO != nil && ingress.IsReady() && !rtView.IsTrue() {
-			cfg := config.FromContext(ctx)
-			logger.Debug("Observing Ingress not-ready to ready switch condition for rollout",
+		if cfg.Network.RolloutDurationSecs > 0 {
+			logger := logging.FromContext(ctx).Desugar().With(
 				zap.Int("durationSecs", cfg.Network.RolloutDurationSecs))
-			prevRO.ObserveReady(now, float64(cfg.Network.RolloutDurationSecs))
-		}
+			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,
+			// the prevRO will be just nil rollout.
+			prevRO := deserializeRollout(ctx,
+				ingress.Annotations[networking.RolloutAnnotationKey])
+
+			// And recompute the rollout state.
+			now := c.clock.Now().UnixNano()
+
+			// Now check if the ingress status changed from not ready to ready.
+			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(now, float64(cfg.Network.RolloutDurationSecs))
+			}
 
-		effectiveRO, nextStepTime := curRO.Step(prevRO, now)
-		if nextStepTime > 0 {
-			nextStepTime -= now
-			c.enqueueAfter(r, time.Duration(nextStepTime))
-			logger.Debug("Re-enqueuing after", zap.Duration("duration", time.Duration(nextStepTime)))
-		}
+			effectiveRO, nextStepTime = curRO.Step(prevRO, now)
+			if nextStepTime > 0 {
+				nextStepTime -= now
+				c.enqueueAfter(r, time.Duration(nextStepTime))
+				logger.Debug("Re-enqueuing after", zap.Duration("duration", time.Duration(nextStepTime)))
+			}
 
-		// Comparing and diffing isn't cheap so do it only if we're going
-		// to actually log the message.
-		// Those are well known types, cmp won't panic.
-		if logger.Core().Enabled(zapcore.DebugLevel) && !cmp.Equal(prevRO, effectiveRO) {
-			logger.Debug("Rollout diff:(-was,+now)",
-				zap.String("diff", cmp.Diff(prevRO, effectiveRO)))
+			// Comparing and diffing isn't cheap so do it only if we're going
+			// to actually log the message.
+			// Those are well known types, cmp won't panic.
+			if logger.Core().Enabled(zapcore.DebugLevel) && !cmp.Equal(prevRO, effectiveRO) {
+				logger.Debug("Rollout diff:(-was,+now)",
+					zap.String("diff", cmp.Diff(prevRO, effectiveRO)))
+			}
 		}
-
 		desired, err := resources.MakeIngressWithRollout(ctx, r, tc, effectiveRO,
 			tls, ingressClass, acmeChallenges...)
 		if err != nil {


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index aff96a5ac..c7f47bbf6 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -175,6 +175,54 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) {
 	}
 }
 
+func TestReconcileIngressUpdateNoRollout(t *testing.T) {
+	var reconciler *Reconciler
+	ctx, _, _, _, cancel := newTestSetup(t, func(r *Reconciler) {
+		reconciler = r
+	})
+	defer cancel()
+	ctx = updateContext(ctx, 0)
+
+	r := Route("test-ns", "test-route")
+
+	tc, tls := testIngressParams(t, r)
+	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
+		t.Error("Unexpected error:", err)
+	}
+
+	initial := getRouteIngressFromClient(ctx, t, r)
+	fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(initial)
+
+	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 {
+		t.Error("Unexpected error:", err)
+	}
+
+	updated := getRouteIngressFromClient(ctx, t, r)
+	if diff := cmp.Diff(initial, updated); diff == "" {
+		t.Error("Expected difference, but found none")
+	}
+	initialdRO := deserializeRollout(ctx, initial.Annotations[networking.RolloutAnnotationKey])
+	if initialdRO == nil {
+		t.Fatal("Rollout was bogus and impossible to deserialize")
+	}
+	updatedRO := deserializeRollout(ctx, updated.Annotations[networking.RolloutAnnotationKey])
+	if updatedRO == nil {
+		t.Fatal("Rollout was bogus and impossible to deserialize")
+	}
+	// This verifies no rollout was started.
+	if got := len(updatedRO.Configurations[0].Revisions); got != 1 {
+		t.Errorf("|revisions in rollout| = %d, want: 1", got)
+	}
+	// This verifies that the new *noop* rollout was installed.
+	if was, become := initialdRO.Configurations[0].Revisions[0].RevisionName,
+		updatedRO.Configurations[0].Revisions[0].RevisionName; was == become {
+		t.Errorf("Revision for default rollout = %s was unchanged", was)
+	}
+}
+
 func TestReconcileIngressUpdate(t *testing.T) {
 	var reconciler *Reconciler
 	ctx, _, _, _, cancel := newTestSetup(t, func(r *Reconciler) {
@@ -360,7 +408,7 @@ func TestReconcileIngressClassAnnotation(t *testing.T) {
 
 func updateContext(ctx context.Context, rolloutDurationSecs int) context.Context {
 	cfg := reconcilerTestConfig(false)
-	cfg.Network.RolloutDurationSecs = 120
+	cfg.Network.RolloutDurationSecs = rolloutDurationSecs
 	c := config.ToContext(ctx, cfg)
 	return c
 }
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 69c3d78a9..307f03a48 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -746,7 +746,8 @@ func TestReconcile(t *testing.T) {
 		// A new LatestCreatedRevisionName on the Configuration alone should result in no changes to the Route.
 		Key: "default/new-latest-created",
 	}, {
-		Name: "new latest ready revision",
+		Name: "new latest ready revision. rollout enabled",
+		Ctx:  context.WithValue(context.Background(), rolloutDurationKey, 120),
 		Objects: []runtime.Object{
 			Route("default", "new-latest-ready", WithConfigTarget("config"),
 				WithURL, WithAddress, WithRouteConditionsAutoTLSDisabled, WithRouteGeneration(1),
@@ -816,6 +817,72 @@ func TestReconcile(t *testing.T) {
 					})),
 		}},
 		Key: "default/new-latest-ready",
+	}, {
+		Name: "new latest ready revision, rollout disabled",
+		Objects: []runtime.Object{
+			Route("default", "new-latest-ready", WithConfigTarget("config"),
+				WithURL, WithAddress, WithRouteConditionsAutoTLSDisabled, WithRouteGeneration(1),
+				MarkTrafficAssigned, MarkIngressReady, WithRouteObservedGeneration, WithRouteFinalizer, WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName: "config-00001",
+						Percent:      ptr.Int64(100),
+					})),
+			cfg("default", "config",
+				WithConfigGeneration(2), WithLatestCreated("config-00002"), WithLatestReady("config-00002"),
+				// The Route controller attaches our label to this Configuration.
+				WithConfigLabel("serving.knative.dev/route", "new-latest-ready"),
+			),
+			rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001"), WithServiceName("magnolia")),
+			// This is the name of the new revision we're referencing above.
+			rev("default", "config", 2, MarkRevisionReady, WithRevName("config-00002"), WithServiceName("belltown")),
+			simpleReadyIngress(
+				Route("default", "new-latest-ready", WithConfigTarget("config"), WithURL),
+				&traffic.Config{
+					Targets: map[string]traffic.RevisionTargets{
+						traffic.DefaultTarget: {{
+							TrafficTarget: v1.TrafficTarget{
+								ConfigurationName: "config",
+								LatestRevision:    ptr.Bool(true),
+								RevisionName:      "config-00001",
+								Percent:           ptr.Int64(100),
+							},
+							ServiceName: "magnolia",
+						}},
+					},
+				},
+			),
+			simpleK8sService(Route("default", "new-latest-ready", WithConfigTarget("config"))),
+		},
+		// A new LatestReadyRevisionName on the Configuration should result in the new Revision being rolled out.
+		WantUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: simpleReadyIngress(
+				Route("default", "new-latest-ready", WithConfigTarget("config"), WithURL, WithRouteGeneration(1)),
+				&traffic.Config{
+					Targets: map[string]traffic.RevisionTargets{
+						traffic.DefaultTarget: {{
+							TrafficTarget: v1.TrafficTarget{
+								ConfigurationName: "config",
+								LatestRevision:    ptr.Bool(true),
+								// This is the new config we're making become ready.
+								RevisionName: "config-00002",
+								Percent:      ptr.Int64(100),
+							},
+							ServiceName: "belltown",
+						}},
+					},
+				}),
+		}},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: Route("default", "new-latest-ready", WithConfigTarget("config"),
+				WithURL, WithAddress, WithRouteConditionsAutoTLSDisabled, WithRouteGeneration(1),
+				MarkTrafficAssigned, MarkIngressReady, WithRouteObservedGeneration, WithRouteFinalizer, WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00002",
+						Percent:        ptr.Int64(100),
+						LatestRevision: ptr.Bool(true),
+					})),
+		}},
+		Key: "default/new-latest-ready",
 	}, {
 		Name: "public becomes cluster local",
 		Objects: []runtime.Object{
@@ -1002,12 +1069,7 @@ func TestReconcile(t *testing.T) {
 							ServiceName: "wallingford",
 						}},
 					},
-				},
-				simpleRollout("config", []traffic.RevisionRollout{{
-					RevisionName: "config-00001", Percent: 99,
-				}, {
-					RevisionName: "config-00002", Percent: 1,
-				}}, fakeCurTime)),
+				}),
 		}},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Route("default", "update-ci-failure", WithConfigTarget("config"),
diff --git a/test/e2e-auto-tls-tests.sh b/test/e2e-auto-tls-tests.sh
index 1997deb73..6b142d06d 100755
--- a/test/e2e-auto-tls-tests.sh
+++ b/test/e2e-auto-tls-tests.sh
@@ -151,10 +151,22 @@ function cleanup_per_selfsigned_namespace_auto_tls() {
 
 function setup_dns_record() {
   go run ./test/e2e/autotls/config/dnssetup/
+  if [ $? -eq 0 ]; then
+    echo "Successfully set up DNS record"
+  else
+    echo "Error setting up DNS record"
+    exit 1
+  fi
 }
 
 function delete_dns_record() {
   go run ./test/e2e/autotls/config/dnscleanup/
+  if [ $? -eq 0 ]; then
+    echo "Successfully tore down DNS record"
+  else
+    echo "Error deleting up DNS record"
+    exit 1
+  fi
 }
 
 # Script entry point.