Repository: knative/serving
Pull request #8847 (2020-07-29 20:23:21)

Simplify the API of the PA lifecycle functions.
- CanScaleToZero is used once only and is not very useful in itself, since it hides pretty simple idea
- inStatusFor is simplified not to take third parameter. It's easier to check that at the call site (1 place)
- Update comments

/lint
/assign @yanweiguo @markusthoemmes 

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/8847#" 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/apis/autoscaling/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/autoscaling/OWNERS)~~ [vagababov]
- ~~[pkg/reconciler/autoscaling/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/autoscaling/OWNERS)~~ [vagababov]
- ~~[pkg/webhook/OWNERS](https://github.com/knative/serving/blob/master/pkg/webhook/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/8847#" 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/apis/autoscaling/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/autoscaling/OWNERS)~~ [vagababov]
- ~~[pkg/reconciler/autoscaling/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/autoscaling/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/8847#" title="Author self-approved">vagababov</a>*, *<a href="https://github.com/knative/serving/pull/8847#pullrequestreview-457886576" title="LGTM">yanweiguo</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/apis/autoscaling/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/autoscaling/OWNERS)~~ [vagababov]
- ~~[pkg/reconciler/autoscaling/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/autoscaling/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: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-integration-tests | 056ef6c11d33bd2612f6c0ccdb6b4a499d931451 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/8847/pull-knative-serving-integration-tests/1288571306735833089) | `/test pull-knative-serving-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/8847). [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-test-reporter-robot: <!--[AUTOMATED-FLAKY-RETRYER]056ef6c11d33bd2612f6c0ccdb6b4a499d931451[AUTOMATED-FLAKY-RETRYER]-->
The following jobs failed:

Test name | Triggers | Retries
--- | --- | ---
pull-knative-serving-integration-tests |  | 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

```
test/e2e.TestRouteVisibilityChanges
```
yanweiguo: /retest

Patch:
diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml
index 4d483143f..3fd5c2cd5 100644
--- a/config/core/configmaps/features.yaml
+++ b/config/core/configmaps/features.yaml
@@ -20,7 +20,7 @@ metadata:
   labels:
     serving.knative.dev/release: devel
   annotations:
-    knative.dev/example-checksum: "ba0dd46d"
+    knative.dev/example-checksum: "ab930d35"
 data:
   _example: |
     ################################
@@ -60,7 +60,7 @@ data:
     # When "allowed", the server will not run the dry-run validation by default.
     #   However, clients may enable the behavior on an individual Service by
     #   attaching the following metadata annotation: "features.knative.dev/podspec-dryrun":"enabled".
-    kubernetes.podspec-dryrun: "enabled"
+    kubernetes.podspec-dryrun: "allowed"
 
     # Indicates whether new responsive garbage collection is enabled. This
     # feature labels revisions in real-time as they become referenced and
diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
index cc63731e8..0f8393f8c 100644
--- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
+++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
@@ -235,37 +235,32 @@ func (pas *PodAutoscalerStatus) MarkResourceFailedCreation(kind, name string) {
 		fmt.Sprintf("Failed to create %s %q.", kind, name))
 }
 
-// CanScaleToZero checks whether the pod autoscaler has been in an inactive state
-// for at least the specified grace period.
-func (pas *PodAutoscalerStatus) CanScaleToZero(now time.Time, gracePeriod time.Duration) bool {
-	return pas.InactiveFor(now) >= gracePeriod
-}
-
 // InactiveFor returns the time PA spent being inactive.
 func (pas *PodAutoscalerStatus) InactiveFor(now time.Time) time.Duration {
-	return pas.inStatusFor(corev1.ConditionFalse, now, 0)
+	return pas.inStatusFor(corev1.ConditionFalse, now)
 }
 
 // ActiveFor returns the time PA spent being active.
 func (pas *PodAutoscalerStatus) ActiveFor(now time.Time) time.Duration {
-	return pas.inStatusFor(corev1.ConditionTrue, now, 0)
+	return pas.inStatusFor(corev1.ConditionTrue, now)
 }
 
 // CanFailActivation checks whether the pod autoscaler has been activating
 // for at least the specified idle period.
 func (pas *PodAutoscalerStatus) CanFailActivation(now time.Time, idlePeriod time.Duration) bool {
-	return pas.inStatusFor(corev1.ConditionUnknown, now, idlePeriod) > 0
+	return pas.inStatusFor(corev1.ConditionUnknown, now) > idlePeriod
 }
 
-// inStatusFor returns positive duration if the PodAutoscalerStatus's Active condition has stayed in
-// the specified status for at least the specified duration. Otherwise it returns negative duration,
-// including when the status is undetermined.
-func (pas *PodAutoscalerStatus) inStatusFor(status corev1.ConditionStatus, now time.Time, dur time.Duration) time.Duration {
+// inStatusFor returns the duration that the PodAutoscalerStatus's Active
+// condition has stayed in the specified status.
+// inStatusFor will return -1 if condition is not initialized or current
+// status is different.
+func (pas *PodAutoscalerStatus) inStatusFor(status corev1.ConditionStatus, now time.Time) time.Duration {
 	cond := pas.GetCondition(PodAutoscalerConditionActive)
 	if cond == nil || cond.Status != status {
 		return -1
 	}
-	return now.Sub(cond.LastTransitionTime.Inner.Add(dur))
+	return now.Sub(cond.LastTransitionTime.Inner.Time)
 }
 
 // GetDesiredScale returns the desired scale if ever set, or -1.
diff --git a/pkg/apis/config/features.go b/pkg/apis/config/features.go
index c2f471272..bd33649d2 100644
--- a/pkg/apis/config/features.go
+++ b/pkg/apis/config/features.go
@@ -43,7 +43,7 @@ func defaultFeaturesConfig() *Features {
 		MultiContainer:       Disabled,
 		PodSpecAffinity:      Disabled,
 		PodSpecFieldRef:      Disabled,
-		PodSpecDryRun:        Enabled,
+		PodSpecDryRun:        Allowed,
 		PodSpecNodeSelector:  Disabled,
 		PodSpecTolerations:   Disabled,
 		ResponsiveRevisionGC: Disabled,
diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go
index 4b75a7dac..057939af6 100644
--- a/pkg/reconciler/autoscaling/kpa/scaler.go
+++ b/pkg/reconciler/autoscaling/kpa/scaler.go
@@ -234,7 +234,8 @@ func (ks *scaler) handleScaleToZero(ctx context.Context, pa *pav1alpha1.PodAutos
 			// Most conservative check, if it passes we're good.
 			lastPodTimeout := lastPodRetention(pa, cfgAS)
 			lastPodMaxTimeout := durationMax(cfgAS.ScaleToZeroGracePeriod, lastPodTimeout)
-			if pa.Status.CanScaleToZero(now, lastPodMaxTimeout) {
+			// If we have been inactive for this long, we can scale to 0!
+			if pa.Status.InactiveFor(now) >= lastPodMaxTimeout {
 				return desiredScale, true
 			}
 


Test patch:
diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go
index 1fcca085b..feae53b72 100644
--- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go
+++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go
@@ -71,102 +71,6 @@ func TestGeneration(t *testing.T) {
 
 }
 
-func TestCanScaleToZero(t *testing.T) {
-	now := time.Now()
-	cases := []struct {
-		name   string
-		status PodAutoscalerStatus
-		result bool
-		grace  time.Duration
-	}{{
-		name:   "empty status",
-		status: PodAutoscalerStatus{},
-		result: false,
-		grace:  10 * time.Second,
-	}, {
-		name: "active condition",
-		status: PodAutoscalerStatus{
-			Status: duckv1.Status{
-				Conditions: duckv1.Conditions{{
-					Type:   PodAutoscalerConditionActive,
-					Status: corev1.ConditionTrue,
-				}},
-			},
-		},
-		result: false,
-		grace:  10 * time.Second,
-	}, {
-		name: "inactive condition (no LTT)",
-		status: PodAutoscalerStatus{
-			Status: duckv1.Status{
-				Conditions: duckv1.Conditions{{
-					Type:   PodAutoscalerConditionActive,
-					Status: corev1.ConditionFalse,
-					// No LTT = beginning of time, so for sure we can.
-				}},
-			},
-		},
-		result: true,
-		grace:  10 * time.Second,
-	}, {
-		name: "inactive condition (LTT longer than grace period ago)",
-		status: PodAutoscalerStatus{
-			Status: duckv1.Status{
-				Conditions: duckv1.Conditions{{
-					Type:   PodAutoscalerConditionActive,
-					Status: corev1.ConditionFalse,
-					LastTransitionTime: apis.VolatileTime{
-						Inner: metav1.NewTime(now.Add(-30 * time.Second)),
-					},
-					// LTT = 30 seconds ago.
-				}},
-			},
-		},
-		result: true,
-		grace:  10 * time.Second,
-	}, {
-		name: "inactive condition (LTT exact grace period ago)",
-		status: PodAutoscalerStatus{
-			Status: duckv1.Status{
-				Conditions: duckv1.Conditions{{
-					Type:   PodAutoscalerConditionActive,
-					Status: corev1.ConditionFalse,
-					LastTransitionTime: apis.VolatileTime{
-						Inner: metav1.NewTime(now.Add(-10 * time.Second)),
-					},
-					// LTT = 10 seconds ago.
-				}},
-			},
-		},
-		result: true,
-		grace:  10 * time.Second,
-	}, {
-		name: "inactive condition (LTT less than grace period ago)",
-		status: PodAutoscalerStatus{
-			Status: duckv1.Status{
-				Conditions: duckv1.Conditions{{
-					Type:   PodAutoscalerConditionActive,
-					Status: corev1.ConditionFalse,
-					LastTransitionTime: apis.VolatileTime{
-						Inner: metav1.NewTime(now.Add(-10 * time.Second)),
-					},
-					// LTT = 10 seconds ago.
-				}},
-			},
-		},
-		result: false,
-		grace:  30 * time.Second,
-	}}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			if e, a := tc.result, tc.status.CanScaleToZero(now, tc.grace); e != a {
-				t.Errorf("%q expected: %v got: %v", tc.name, e, a)
-			}
-		})
-	}
-}
-
 func TestInactiveFor(t *testing.T) {
 	now := time.Now()
 	cases := []struct {