Repository: knative/serving (go)

Issue:
TestGradualRollout sometimes fails because Rollout duration is a few seconds shorter
/area test-and-release

## What version of Knative?

The last error happened when build on aac0df2f3c1f849a810bd0390d04f3b137608e9e

## Expected Behavior

- TestGradualRollout pssed without flake.

## Actual Behavior

- Please refer to the following error.

https://prow.knative.dev/view/gcs/knative-prow/logs/ci-knative-serving-kourier-stable/1354223700995477504
```
    rollout_test.go:130: Actual Rollout duration shorter than requested: 59.422313315s < 1m0s
    rollout_test.go:132: Rollout Duration = 59.422313315s
```

https://prow.knative.dev/view/gcs/knative-prow/logs/ci-knative-serving-kourier-stable/1354178402256424960
```
    rollout_test.go:130: Actual Rollout duration shorter than requested: 57.352517039s < 1m0s
    rollout_test.go:132: Rollout Duration = 57.352517039s
```

Also downstream which uses Kourier hits this issue.

## Steps to Reproduce the Problem

(This happens on CI smetimes.)

## Addtional note

- The issue happened on Kourier only so far. But I guess the issue should not be related to KIngress :thinking: 

Pull request:
Remove the time computation
#10641 reports that in Kourier this check is not conservative enough, since by the time we observe the start time the process might be well underway.
So, remove the check altogether, since it's not very useful anyway

Fixes #10641

/assign @dprotaso @tcnghia 

Patch:
diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go
index d3d5ab5b4..32ac6180a 100644
--- a/pkg/apis/serving/v1/revision_types.go
+++ b/pkg/apis/serving/v1/revision_types.go
@@ -122,14 +122,14 @@ func IsRevisionCondition(t apis.ConditionType) bool {
 type RevisionStatus struct {
 	duckv1.Status `json:",inline"`
 
-	// DeprecatedServiceName holds the name of a core Kubernetes Service resource that
+	// ServiceName holds the name of a core Kubernetes Service resource that
 	// load balances over the pods backing this Revision.
-	// DEPRECATED: revision service name is effectively equal to the revision name,
+	// Deprecated: revision service name is effectively equal to the revision name,
 	// as per #10540.
 	// 0.23 — stop populating
 	// 0.25 — remove.
 	// +optional
-	DeprecatedServiceName string `json:"serviceName,omitempty"`
+	ServiceName string `json:"serviceName,omitempty"`
 
 	// LogURL specifies the generated logging url for this particular revision
 	// based on the revision url template specified in the controller's config.
diff --git a/pkg/autoscaler/scaling/autoscaler.go b/pkg/autoscaler/scaling/autoscaler.go
index 93cc94a48..332da5d4f 100644
--- a/pkg/autoscaler/scaling/autoscaler.go
+++ b/pkg/autoscaler/scaling/autoscaler.go
@@ -27,7 +27,6 @@ import (
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 
-	"knative.dev/pkg/logging"
 	pkgmetrics "knative.dev/pkg/metrics"
 	"knative.dev/serving/pkg/apis/autoscaling"
 	"knative.dev/serving/pkg/autoscaler/aggregation/max"
@@ -141,8 +140,7 @@ func (a *autoscaler) Update(deciderSpec *DeciderSpec) {
 // validScale signifies whether the desiredPodCount should be applied or not.
 // Scale is not thread safe in regards to panic state, but it's thread safe in
 // regards to acquiring the decider spec.
-func (a *autoscaler) Scale(ctx context.Context, now time.Time) ScaleResult {
-	logger := logging.FromContext(ctx)
+func (a *autoscaler) Scale(logger *zap.SugaredLogger, now time.Time) ScaleResult {
 	desugared := logger.Desugar()
 	debugEnabled := desugared.Core().Enabled(zapcore.DebugLevel)
 
diff --git a/pkg/autoscaler/scaling/multiscaler.go b/pkg/autoscaler/scaling/multiscaler.go
index 2a40c6572..3cea9f190 100644
--- a/pkg/autoscaler/scaling/multiscaler.go
+++ b/pkg/autoscaler/scaling/multiscaler.go
@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"knative.dev/pkg/logging"
 	av1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
 	"knative.dev/serving/pkg/autoscaler/metrics"
 )
@@ -117,7 +118,7 @@ var invalidSR = ScaleResult{
 // UniScaler records statistics for a particular Decider and proposes the scale for the Decider's target based on those statistics.
 type UniScaler interface {
 	// Scale computes a scaling suggestion for a revision.
-	Scale(context.Context, time.Time) ScaleResult
+	Scale(*zap.SugaredLogger, time.Time) ScaleResult
 
 	// Update reconfigures the UniScaler according to the DeciderSpec.
 	Update(*DeciderSpec)
@@ -131,6 +132,7 @@ type scalerRunner struct {
 	scaler UniScaler
 	stopCh chan struct{}
 	pokeCh chan struct{}
+	logger *zap.SugaredLogger
 
 	// mux guards access to decider.
 	mux     sync.RWMutex
@@ -285,7 +287,7 @@ func (m *MultiScaler) Inform(event types.NamespacedName) bool {
 	return false
 }
 
-func (m *MultiScaler) runScalerTicker(ctx context.Context, runner *scalerRunner) {
+func (m *MultiScaler) runScalerTicker(runner *scalerRunner) {
 	metricKey := types.NamespacedName{Namespace: runner.decider.Namespace, Name: runner.decider.Name}
 	ticker := m.tickProvider(tickInterval)
 	go func() {
@@ -297,9 +299,9 @@ func (m *MultiScaler) runScalerTicker(ctx context.Context, runner *scalerRunner)
 			case <-runner.stopCh:
 				return
 			case <-ticker.C:
-				m.tickScaler(ctx, runner.scaler, runner, metricKey)
+				m.tickScaler(runner.scaler, runner, metricKey)
 			case <-runner.pokeCh:
-				m.tickScaler(ctx, runner.scaler, runner, metricKey)
+				m.tickScaler(runner.scaler, runner, metricKey)
 			}
 		}
 	}()
@@ -317,6 +319,7 @@ func (m *MultiScaler) createScaler(ctx context.Context, decider *Decider) (*scal
 		stopCh:  make(chan struct{}),
 		decider: d,
 		pokeCh:  make(chan struct{}),
+		logger:  logging.FromContext(ctx),
 	}
 	d.Status.DesiredScale = -1
 	switch tbc := d.Spec.TargetBurstCapacity; tbc {
@@ -328,12 +331,12 @@ func (m *MultiScaler) createScaler(ctx context.Context, decider *Decider) (*scal
 		d.Status.ExcessBurstCapacity = int32(float64(d.Spec.InitialScale)*d.Spec.TotalValue - tbc)
 	}
 
-	m.runScalerTicker(ctx, runner)
+	m.runScalerTicker(runner)
 	return runner, nil
 }
 
-func (m *MultiScaler) tickScaler(ctx context.Context, scaler UniScaler, runner *scalerRunner, metricKey types.NamespacedName) {
-	sr := scaler.Scale(ctx, time.Now())
+func (m *MultiScaler) tickScaler(scaler UniScaler, runner *scalerRunner, metricKey types.NamespacedName) {
+	sr := scaler.Scale(runner.logger, time.Now())
 
 	if !sr.ScaleValid {
 		return
diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go
index 233d8b75e..8f7e1ed2e 100644
--- a/pkg/reconciler/revision/reconcile_resources.go
+++ b/pkg/reconciler/revision/reconcile_resources.go
@@ -174,7 +174,7 @@ func (c *Reconciler) reconcilePA(ctx context.Context, rev *v1.Revision) error {
 
 	// The public service name is always equal to the revision name itself.
 	// Historically it's been acquired from the PA object, so the assignment is here.
-	rev.Status.DeprecatedServiceName = rev.Name
+	rev.Status.ServiceName = rev.Name
 
 	logger.Debugf("Observed PA Status=%#v", pa.Status)
 	rev.Status.PropagateAutoscalerStatus(&pa.Status)


Test patch:
diff --git a/pkg/autoscaler/scaling/autoscaler_test.go b/pkg/autoscaler/scaling/autoscaler_test.go
index 474221151..abda2dc34 100644
--- a/pkg/autoscaler/scaling/autoscaler_test.go
+++ b/pkg/autoscaler/scaling/autoscaler_test.go
@@ -31,11 +31,11 @@ import (
 	"knative.dev/pkg/metrics/metricskey"
 	"knative.dev/pkg/metrics/metricstest"
 
+	logtesting "knative.dev/pkg/logging/testing"
 	"knative.dev/serving/pkg/autoscaler/metrics"
 	smetrics "knative.dev/serving/pkg/metrics"
 	"knative.dev/serving/pkg/resources"
 
-	. "knative.dev/pkg/logging/testing"
 	_ "knative.dev/pkg/metrics/testing"
 )
 
@@ -78,7 +78,7 @@ func TestAutoscalerScaleDownDelay(t *testing.T) {
 		PanicThreshold:   100,
 		ScaleDownDelay:   5 * time.Minute,
 	}
-	as := New(TestContextWithLogger(t), testNamespace, testRevision, metrics, pc, spec)
+	as := New(context.Background(), testNamespace, testRevision, metrics, pc, spec)
 
 	now := time.Time{}
 
@@ -147,7 +147,7 @@ func TestAutoscalerScaleDownDelayZero(t *testing.T) {
 		PanicThreshold:   100,
 		ScaleDownDelay:   0,
 	}
-	as := New(TestContextWithLogger(t), testNamespace, testRevision, metrics, pc, spec)
+	as := New(context.Background(), testNamespace, testRevision, metrics, pc, spec)
 
 	now := time.Time{}
 
@@ -560,7 +560,7 @@ func TestCantCountPods(t *testing.T) {
 	metrics := &metricClient{StableConcurrency: 1000, PanicConcurrency: 888}
 	a, pc := newTestAutoscaler(10, 81, metrics)
 	pc.err = errors.New("peaches-in-regalia")
-	if got, want := a.Scale(context.Background(), time.Now()), invalidSR; !cmp.Equal(got, want) {
+	if got, want := a.Scale(logtesting.TestLogger(t), time.Now()), invalidSR; !cmp.Equal(got, want) {
 		t.Errorf("Scale = %v, want: %v", got, want)
 	}
 }
@@ -646,7 +646,7 @@ func approxEquateInt32(field string) cmp.Option {
 
 func expectScale(t *testing.T, a UniScaler, now time.Time, want ScaleResult) {
 	t.Helper()
-	got := a.Scale(TestContextWithLogger(t), now)
+	got := a.Scale(logtesting.TestLogger(t), now)
 	if !cmp.Equal(got, want, approxEquateInt32("ExcessBurstCapacity")) {
 		t.Error("ScaleResult mismatch(-want,+got):\n", cmp.Diff(want, got))
 	}
@@ -764,6 +764,6 @@ func BenchmarkAutoscaler(b *testing.B) {
 	now := time.Now()
 
 	for i := 0; i < b.N; i++ {
-		a.Scale(context.Background(), now)
+		a.Scale(logtesting.TestLogger(b), now)
 	}
 }
diff --git a/pkg/autoscaler/scaling/multiscaler_test.go b/pkg/autoscaler/scaling/multiscaler_test.go
index 03e01a65d..8a193583b 100644
--- a/pkg/autoscaler/scaling/multiscaler_test.go
+++ b/pkg/autoscaler/scaling/multiscaler_test.go
@@ -413,7 +413,7 @@ func (u *fakeUniScaler) fakeUniScalerFactory(*Decider) (UniScaler, error) {
 	return u, nil
 }
 
-func (u *fakeUniScaler) Scale(context.Context, time.Time) ScaleResult {
+func (u *fakeUniScaler) Scale(*zap.SugaredLogger, time.Time) ScaleResult {
 	u.mutex.Lock()
 	defer u.mutex.Unlock()
 	u.scaleCount++
diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go
index 56af1c695..5db2740c1 100644
--- a/pkg/testing/v1/revision.go
+++ b/pkg/testing/v1/revision.go
@@ -52,7 +52,7 @@ func WithRevName(name string) RevisionOption {
 // WithServiceName propagates the given service name to the revision status.
 func WithServiceName(sn string) RevisionOption {
 	return func(rev *v1.Revision) {
-		rev.Status.DeprecatedServiceName = sn
+		rev.Status.ServiceName = sn
 	}
 }
 
@@ -139,7 +139,7 @@ func MarkActive(r *v1.Revision) {
 
 // WithK8sServiceName applies sn to the revision status field.
 func WithK8sServiceName(r *v1.Revision) {
-	r.Status.DeprecatedServiceName = r.Name
+	r.Status.ServiceName = r.Name
 }
 
 // MarkInactive calls .Status.MarkInactive on the Revision.
diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go
index 6450e1165..cfbb9a22a 100644
--- a/test/e2e/e2e.go
+++ b/test/e2e/e2e.go
@@ -131,7 +131,7 @@ func waitForActivatorEndpoints(ctx *TestContext) error {
 		}
 
 		svcEps, err := ctx.clients.KubeClient.CoreV1().Endpoints(test.ServingNamespace).Get(
-			context.Background(), ctx.resources.Revision.Status.DeprecatedServiceName, metav1.GetOptions{})
+			context.Background(), ctx.resources.Revision.Name, metav1.GetOptions{})
 		if err != nil {
 			return false, err
 		}
diff --git a/test/e2e/rollout_test.go b/test/e2e/rollout_test.go
index 532312150..f34267aa7 100644
--- a/test/e2e/rollout_test.go
+++ b/test/e2e/rollout_test.go
@@ -57,7 +57,6 @@ func TestGradualRollout(t *testing.T) {
 	if _, err := v1test.PatchService(t, clients, robjs.Service, testingv1.WithServiceImage(pkgtest.ImagePath(test.Autoscale))); err != nil {
 		t.Fatalf("Patch update for Service %s with image %s failed: %v", names.Service, test.Autoscale, err)
 	}
-	start := time.Now()
 
 	// This will cover all the status checks:
 	// - Status is in Rollout
@@ -124,10 +123,4 @@ func TestGradualRollout(t *testing.T) {
 		}, "RolloutFinished"); err != nil {
 		t.Fatalf("Failed waiting for Rollout %q to complete: %+v", names.Service, err)
 	}
-
-	dur := time.Since(start)
-	if dur < time.Minute {
-		t.Errorf("Actual Rollout duration shorter than requested: %v < %v", dur, rolloutDuration)
-	}
-	t.Log("Rollout Duration =", dur)
 }