diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index 03e7c36a2885..48be08e7badc 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -30,7 +30,7 @@ import ( ) var podCondSet = apis.NewLivingConditionSet( - PodAutoscalerConditionActive, + PodAutoscalerConditionBootstrap, ) func (pa *PodAutoscaler) GetGroupVersionKind() schema.GroupVersionKind { @@ -138,6 +138,12 @@ func (pas *PodAutoscalerStatus) IsReady() bool { return podCondSet.Manage(pas.duck()).IsHappy() } +// HasFailed returns true if the Ready condition is false. +func (pas *PodAutoscalerStatus) HasFailed() bool { + cond := pas.GetCondition(PodAutoscalerConditionReady) + return cond != nil && cond.Status == corev1.ConditionFalse +} + // IsActivating returns true if the pod autoscaler is Activating if it is neither // Active nor Inactive func (pas *PodAutoscalerStatus) IsActivating() bool { @@ -151,6 +157,12 @@ func (pas *PodAutoscalerStatus) IsInactive() bool { return cond != nil && cond.Status == corev1.ConditionFalse } +// IsActive returns true if the pod autoscaler is Active. +func (pas *PodAutoscalerStatus) IsActive() bool { + cond := pas.GetCondition(PodAutoscalerConditionActive) + return cond != nil && cond.Status == corev1.ConditionTrue +} + // GetCondition gets the condition `t`. func (pas *PodAutoscalerStatus) GetCondition(t apis.ConditionType) *apis.Condition { return podCondSet.Manage(pas.duck()).GetCondition(t) @@ -179,17 +191,39 @@ func (pas *PodAutoscalerStatus) MarkInactive(reason, message string) { // MarkResourceNotOwned changes the "Active" condition to false to reflect that the // resource of the given kind and name has already been created, and we do not own it. func (pas *PodAutoscalerStatus) MarkResourceNotOwned(kind, name string) { - pas.MarkInactive("NotOwned", + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, "NotOwned", fmt.Sprintf("There is an existing %s %q that we do not own.", kind, name)) } // MarkResourceFailedCreation changes the "Active" condition to false to reflect that a // critical resource of the given kind and name was unable to be created. func (pas *PodAutoscalerStatus) MarkResourceFailedCreation(kind, name string) { - pas.MarkInactive("FailedCreate", + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, "FailedCreate", fmt.Sprintf("Failed to create %s %q.", kind, name)) } +// MarkContainerExiting changes the "Bootstrap" condition to false and records the exit code. +func (pas *PodAutoscalerStatus) MarkContainerExiting(exitCode int32, message string) { + exitCodeString := fmt.Sprintf("ExitCode%d", exitCode) + message = fmt.Sprintf("Container failed with: %s", message) + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, exitCodeString, message) +} + +// MarkImagePullError changes the "Bootstrap" condition to false. +func (pas *PodAutoscalerStatus) MarkImagePullError(reason, message string) { + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, reason, message) +} + +// MarkPodUnscheduled changes the "Bootstrap" condition to false and records reason and message. +func (pas *PodAutoscalerStatus) MarkPodUnscheduled(reason, message string) { + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, reason, message) +} + +// MarkBootstrapSuccessful changes the "Bootstrap" condition to true. +func (pas *PodAutoscalerStatus) MarkBootstrapSuccessful() { + podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionBootstrap) +} + // CanScaleToZero checks whether the pod autoscaler has been in an inactive state // for at least the specified grace period. func (pas *PodAutoscalerStatus) CanScaleToZero(gracePeriod time.Duration) bool { diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 4f590efa84e5..690b6582cd88 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -253,7 +253,7 @@ func TestIsActivating(t *testing.T) { Status: duckv1beta1.Status{ Conditions: duckv1beta1.Conditions{{ Type: PodAutoscalerConditionActive, - Status: corev1.ConditionTrue, + Status: corev1.ConditionFalse, }}, }, }, @@ -267,6 +267,57 @@ func TestIsActivating(t *testing.T) { } } +func TestIsActive(t *testing.T) { + cases := []struct { + name string + status PodAutoscalerStatus + isActive bool + }{{ + name: "empty status", + status: PodAutoscalerStatus{}, + isActive: false, + }, { + name: "active=unknown", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionActive, + Status: corev1.ConditionUnknown, + }}, + }, + }, + isActive: false, + }, { + name: "active=false", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionActive, + Status: corev1.ConditionFalse, + }}, + }, + }, + isActive: false, + }, { + name: "active=true", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionActive, + Status: corev1.ConditionTrue, + }}, + }, + }, + isActive: true, + }} + + for _, tc := range cases { + if e, a := tc.isActive, tc.status.IsActive(); e != a { + t.Errorf("%q expected: %v got: %v", tc.name, e, a) + } + } +} + func TestIsReady(t *testing.T) { cases := []struct { name string @@ -281,7 +332,7 @@ func TestIsReady(t *testing.T) { status: PodAutoscalerStatus{ Status: duckv1beta1.Status{ Conditions: duckv1beta1.Conditions{{ - Type: PodAutoscalerConditionActive, + Type: PodAutoscalerConditionBootstrap, Status: corev1.ConditionTrue, }}, }, @@ -337,6 +388,9 @@ func TestIsReady(t *testing.T) { Conditions: duckv1beta1.Conditions{{ Type: PodAutoscalerConditionActive, Status: corev1.ConditionTrue, + }, { + Type: PodAutoscalerConditionBootstrap, + Status: corev1.ConditionTrue, }, { Type: PodAutoscalerConditionReady, Status: corev1.ConditionTrue, @@ -351,6 +405,9 @@ func TestIsReady(t *testing.T) { Conditions: duckv1beta1.Conditions{{ Type: PodAutoscalerConditionActive, Status: corev1.ConditionTrue, + }, { + Type: PodAutoscalerConditionBootstrap, + Status: corev1.ConditionTrue, }, { Type: PodAutoscalerConditionReady, Status: corev1.ConditionFalse, @@ -367,6 +424,112 @@ func TestIsReady(t *testing.T) { } } +func TestHasFailed(t *testing.T) { + cases := []struct { + name string + status PodAutoscalerStatus + hasFailed bool + }{{ + name: "empty status should not have failed", + status: PodAutoscalerStatus{}, + hasFailed: false, + }, { + name: "Different condition type should not have failed", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionBootstrap, + Status: corev1.ConditionFalse, + }}, + }, + }, + hasFailed: false, + }, { + name: "Unknown condition status should not have failed", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionReady, + Status: corev1.ConditionUnknown, + }}, + }, + }, + hasFailed: false, + }, { + name: "Missing condition status should not be ready", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionReady, + }}, + }, + }, + hasFailed: false, + }, { + name: "True condition status should not have failed", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + hasFailed: false, + }, { + name: "Multiple conditions with ready status should not have failed", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionActive, + Status: corev1.ConditionFalse, + }, { + Type: PodAutoscalerConditionBootstrap, + Status: corev1.ConditionFalse, + }, { + Type: PodAutoscalerConditionReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + hasFailed: false, + }, { + name: "False condition status should have failed", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionReady, + Status: corev1.ConditionFalse, + }}, + }, + }, + hasFailed: true, + }, { + name: "Multiple conditions with ready status false should have failed", + status: PodAutoscalerStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: PodAutoscalerConditionActive, + Status: corev1.ConditionTrue, + }, { + Type: PodAutoscalerConditionBootstrap, + Status: corev1.ConditionTrue, + }, { + Type: PodAutoscalerConditionReady, + Status: corev1.ConditionFalse, + }}, + }, + }, + hasFailed: true, + }} + + for _, tc := range cases { + if e, a := tc.hasFailed, tc.status.HasFailed(); e != a { + t.Errorf("%q expected: %v got: %v", tc.name, e, a) + } + } +} + func TestTargetAnnotation(t *testing.T) { cases := []struct { name string @@ -495,26 +658,82 @@ func TestScaleBounds(t *testing.T) { func TestMarkResourceNotOwned(t *testing.T) { pa := pa(map[string]string{}) pa.Status.MarkResourceNotOwned("doesn't", "matter") - active := pa.Status.GetCondition("Active") - if active.Status != corev1.ConditionFalse { - t.Errorf("TestMarkResourceNotOwned expected active.Status: False got: %v", active.Status) + bootstrap := pa.Status.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionFalse { + t.Errorf("TestMarkResourceNotOwned expected bootstrap.Status: False got: %v", bootstrap.Status) } - if active.Reason != "NotOwned" { - t.Errorf("TestMarkResourceNotOwned expected active.Reason: NotOwned got: %v", active.Reason) + if bootstrap.Reason != "NotOwned" { + t.Errorf("TestMarkResourceNotOwned expected bootstrap.Reason: NotOwned got: %v", bootstrap.Reason) } } func TestMarkResourceFailedCreation(t *testing.T) { pa := &PodAutoscalerStatus{} pa.MarkResourceFailedCreation("doesn't", "matter") - apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionActive, t) + apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionBootstrap, t) + + bootstrap := pa.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionFalse { + t.Errorf("TestMarkResourceFailedCreation expected active.Status: False got: %v", bootstrap.Status) + } + if bootstrap.Reason != "FailedCreate" { + t.Errorf("TestMarkResourceFailedCreation expected active.Reason: FailedCreate got: %v", bootstrap.Reason) + } +} - active := pa.GetCondition("Active") - if active.Status != corev1.ConditionFalse { - t.Errorf("TestMarkResourceFailedCreation expected active.Status: False got: %v", active.Status) +func TestMarkContainerExiting(t *testing.T) { + pa := &PodAutoscalerStatus{} + pa.MarkContainerExiting(5, "I'm failing") + apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionBootstrap, t) + + bootstrap := pa.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionFalse { + t.Errorf("TestMarkContainerExiting expected bootstrap.status: False got: %v", bootstrap.Status) } - if active.Reason != "FailedCreate" { - t.Errorf("TestMarkResourceFailedCreation expected active.Reason: FailedCreate got: %v", active.Reason) + if bootstrap.Reason != "ExitCode5" { + t.Errorf("TestMarkContainerExiting expected bootstrap.Reason: ExitCode5 got: %v", bootstrap.Reason) + } +} + +func TestMarkImagePullError(t *testing.T) { + pa := &PodAutoscalerStatus{} + pa.MarkImagePullError("PullProblem", "Some pull problem occurred") + apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionBootstrap, t) + + bootstrap := pa.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionFalse { + t.Errorf("TestMarkImagePullError expected bootstrap.status: False got: %v", bootstrap.Status) + } + if bootstrap.Reason != "PullProblem" { + t.Errorf("TestMarkImagePullError expected bootstrap.Reason: PullProblem got: %v", bootstrap.Reason) + } +} + +func TestMarkPodUnscheduled(t *testing.T) { + pa := &PodAutoscalerStatus{} + pa.MarkPodUnscheduled("PodUnscheduled", "Pod cannot be scheduled") + apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionBootstrap, t) + + bootstrap := pa.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionFalse { + t.Errorf("TestMarkPodUnscheduled expected bootstrap.status: False got: %v", bootstrap.Status) + } + if bootstrap.Reason != "PodUnscheduled" { + t.Errorf("TestMarkPodUnscheduled expected bootstrap.Reason: PodUnscheduled got: %v", bootstrap.Reason) + } +} + +func TestMarkBootstrapSuccessful(t *testing.T) { + pa := &PodAutoscalerStatus{} + pa.MarkBootstrapSuccessful() + apitest.CheckConditionSucceeded(pa.duck(), PodAutoscalerConditionBootstrap, t) + + bootstrap := pa.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionTrue { + t.Errorf("TestMarkMarkBootstrapSuccessful expected bootstrap.status: True got: %v", bootstrap.Status) + } + if bootstrap.Reason != "" { + t.Errorf("TestMarkBootstrapSuccessful expected bootstrap.Reason: got: %v", bootstrap.Reason) } } @@ -757,36 +976,43 @@ func pa(annotations map[string]string) *PodAutoscaler { func TestTypicalFlow(t *testing.T) { r := &PodAutoscalerStatus{} r.InitializeConditions() + r.MarkActivating("Initializing", "") apitest.CheckConditionOngoing(r.duck(), PodAutoscalerConditionActive, t) + apitest.CheckConditionOngoing(r.duck(), PodAutoscalerConditionBootstrap, t) apitest.CheckConditionOngoing(r.duck(), PodAutoscalerConditionReady, t) - // When we see traffic, mark ourselves active. + // When minPods are up, mark ourselves active and bootstrap successfully. r.MarkActive() + r.MarkBootstrapSuccessful() apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionActive, t) + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionBootstrap, t) apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionReady, t) // Check idempotency. r.MarkActive() + r.MarkBootstrapSuccessful() apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionActive, t) + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionBootstrap, t) apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionReady, t) // When we stop seeing traffic, mark outselves inactive. r.MarkInactive("TheReason", "the message") apitest.CheckConditionFailed(r.duck(), PodAutoscalerConditionActive, t) - if !r.IsInactive() { - t.Errorf("IsInactive was not set.") - } - apitest.CheckConditionFailed(r.duck(), PodAutoscalerConditionReady, t) + // We are inactive but still ready. + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionBootstrap, t) + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionReady, t) // When traffic hits the activator and we scale up the deployment we mark // ourselves as activating. r.MarkActivating("Activating", "Red team, GO!") apitest.CheckConditionOngoing(r.duck(), PodAutoscalerConditionActive, t) - apitest.CheckConditionOngoing(r.duck(), PodAutoscalerConditionReady, t) + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionBootstrap, t) + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionReady, t) // When the activator successfully forwards traffic to the deployment, // we mark ourselves as active once more. r.MarkActive() apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionActive, t) + apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionBootstrap, t) apitest.CheckConditionSucceeded(r.duck(), PodAutoscalerConditionReady, t) } diff --git a/pkg/apis/autoscaling/v1alpha1/pa_types.go b/pkg/apis/autoscaling/v1alpha1/pa_types.go index 18229f71f688..cf28e90d6a3a 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_types.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_types.go @@ -99,8 +99,11 @@ const ( // PodAutoscalerConditionReady is set when the revision is starting to materialize // runtime resources, and becomes true when those resources are ready. PodAutoscalerConditionReady = apis.ConditionReady - // PodAutoscalerConditionActive is set when the PodAutoscaler's ScaleTargetRef is receiving traffic. + // PodAutoscalerConditionActive becomes true when the PodAutoscaler's ScaleTargetRef can receive traffic. PodAutoscalerConditionActive apis.ConditionType = "Active" + // PodAutoscalerConditionBootstrap is false when there is some failure preventing the pods from booting up. + // It becomes true when a pod boots successfully. + PodAutoscalerConditionBootstrap apis.ConditionType = "Bootstrap" ) // PodAutoscalerStatus communicates the observed state of the PodAutoscaler (from the controller). diff --git a/pkg/apis/autoscaling/v1alpha1/podscalable_types.go b/pkg/apis/autoscaling/v1alpha1/podscalable_types.go index 2f06e01078c2..60e936cadeea 100644 --- a/pkg/apis/autoscaling/v1alpha1/podscalable_types.go +++ b/pkg/apis/autoscaling/v1alpha1/podscalable_types.go @@ -50,7 +50,8 @@ type PodScalableSpec struct { // PodScalableStatus is the observed state of a PodScalable (or at // least our shared portion). type PodScalableStatus struct { - Replicas int32 `json:"replicas,omitempty"` + Replicas int32 `json:"replicas,omitempty"` + ReadyReplicas int32 `json:"readyReplicas,omitempty"` } var _ duck.Populatable = (*PodScalable)(nil) @@ -92,7 +93,8 @@ func (t *PodScalable) Populate() { }, } t.Status = PodScalableStatus{ - Replicas: 42, + Replicas: 42, + ReadyReplicas: 41, } } diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle.go b/pkg/apis/serving/v1alpha1/revision_lifecycle.go index 4e8102262510..64460e54a539 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle.go @@ -164,9 +164,8 @@ func (rs *RevisionStatus) MarkContainerHealthy() { revCondSet.Manage(rs).MarkTrue(RevisionConditionContainerHealthy) } -func (rs *RevisionStatus) MarkContainerExiting(exitCode int32, message string) { - exitCodeString := fmt.Sprintf("ExitCode%d", exitCode) - revCondSet.Manage(rs).MarkFalse(RevisionConditionContainerHealthy, exitCodeString, RevisionContainerExitingMessage(message)) +func (rs *RevisionStatus) MarkContainerUnhealthy(reason, message string) { + revCondSet.Manage(rs).MarkFalse(RevisionConditionContainerHealthy, reason, message) } func (rs *RevisionStatus) MarkResourcesAvailable() { @@ -201,17 +200,9 @@ func RevisionContainerMissingMessage(image string, message string) string { return fmt.Sprintf("Unable to fetch image %q: %s", image, message) } -// RevisionContainerExitingMessage constructs the status message if a container -// fails to come up. -func RevisionContainerExitingMessage(message string) string { - return fmt.Sprintf("Container failed with: %s", message) -} - const ( AnnotationParseErrorTypeMissing = "Missing" AnnotationParseErrorTypeInvalid = "Invalid" - LabelParserErrorTypeMissing = "Missing" - LabelParserErrorTypeInvalid = "Invalid" ) // +k8s:deepcopy-gen=false diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go index 46a1fdaaebe1..44da268db173 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go @@ -255,6 +255,22 @@ func TestGetSetCondition(t *testing.T) { } } +func TestRevisionStatus_MarkContainerHealthy(t *testing.T) { + rs := &RevisionStatus{} + rs.MarkContainerHealthy() + if cond := rs.GetCondition(RevisionConditionContainerHealthy); cond.Status != corev1.ConditionTrue { + t.Errorf("TestRevisionStatus_MarkContainerHealthy expected True, got %v", cond.Status) + } +} + +func TestRevisionStatus_MarkContainerUnhealthy(t *testing.T) { + rs := &RevisionStatus{} + rs.MarkContainerUnhealthy("SomethingWrong", "Something went wrong with container") + if cond := rs.GetCondition(RevisionConditionContainerHealthy); cond.Status != corev1.ConditionFalse { + t.Errorf("TestRevisionStatus_MarkContainerUnhealthy expected False, got %v", cond.Status) + } +} + func TestTypicalFlowWithProgressDeadlineExceeded(t *testing.T) { r := &RevisionStatus{} r.InitializeConditions() diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 69d06faf806b..184bf8821499 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -166,7 +166,7 @@ const ( RevisionConditionResourcesAvailable apis.ConditionType = "ResourcesAvailable" // RevisionConditionContainerHealthy is set when the revision readiness check completes. RevisionConditionContainerHealthy apis.ConditionType = "ContainerHealthy" - // RevisionConditionActive is set when the revision is receiving traffic. + // RevisionConditionActive is set when the revision can receive traffic. RevisionConditionActive apis.ConditionType = "Active" ) diff --git a/pkg/reconciler/autoscaling/hpa/hpa.go b/pkg/reconciler/autoscaling/hpa/hpa.go index db3aee58f6bd..03301e2da799 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa.go +++ b/pkg/reconciler/autoscaling/hpa/hpa.go @@ -105,6 +105,9 @@ func (c *Reconciler) reconcile(ctx context.Context, key string, pa *pav1alpha1.P pa.SetDefaults(ctx) pa.Status.InitializeConditions() + if pa.Status.GetCondition(pav1alpha1.PodAutoscalerConditionActive) == nil { + pa.Status.MarkActivating("Initializing", "") + } logger.Debug("PA exists") // HPA-class PAs don't yet support scale-to-zero @@ -158,6 +161,7 @@ func (c *Reconciler) reconcile(ctx context.Context, key string, pa *pav1alpha1.P pa.Status.MarkInactive("ServicesNotReady", "SKS Services are not ready yet") } else { pa.Status.MarkActive() + pa.Status.MarkBootstrapSuccessful() } pa.Status.ObservedGeneration = pa.Generation diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go index 147ffaef1819..df61e1937a18 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa_test.go +++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go @@ -92,6 +92,10 @@ func TestControllerCanReconcile(t *testing.T) { } } +func markActive(pa *asv1a1.PodAutoscaler) { + pa.Status.MarkActive() +} + func TestReconcile(t *testing.T) { const deployName = testRevision + "-deployment" usualSelector := map[string]string{"a": "b"} @@ -99,8 +103,8 @@ func TestReconcile(t *testing.T) { table := TableTest{{ Name: "no op", Objects: []runtime.Object{ - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithPAStatusService(testRevision)), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), + pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }, @@ -108,30 +112,30 @@ func TestReconcile(t *testing.T) { }, { Name: "create hpa & sks", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass), + pa(testRevision, testNamespace, WithHPAClass, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), }, Key: key(testRevision, testNamespace), WantCreates: []runtime.Object{ sks(testNamespace, testRevision, WithDeployRef(deployName)), hpa(testRevision, testNamespace, pa(testRevision, testNamespace, - WithHPAClass, WithMetricAnnotation("cpu"))), + WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testRevision, testNamespace, WithHPAClass, - WithNoTraffic("ServicesNotReady", "SKS Services are not ready yet")), + WithNoTraffic("ServicesNotReady", "SKS Services are not ready yet"), WithSuccessfulBootstrap()), }}, }, { Name: "create hpa, sks and metric service", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation(autoscaling.Concurrency)), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation(autoscaling.Concurrency), WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), }, Key: key(testRevision, testNamespace), WantCreates: []runtime.Object{ sks(testNamespace, testRevision, WithDeployRef(deployName)), hpa(testRevision, testNamespace, pa(testRevision, testNamespace, - WithHPAClass, WithMetricAnnotation(autoscaling.Concurrency))), + WithHPAClass, WithMetricAnnotation(autoscaling.Concurrency), WithSuccessfulBootstrap())), metricsSvc(testNamespace, testRevision, WithSvcSelector(usualSelector), SvcWithAnnotationValue(autoscaling.ClassAnnotationKey, autoscaling.HPA), SvcWithAnnotationValue(autoscaling.MetricAnnotationKey, autoscaling.Concurrency)), @@ -139,14 +143,14 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation(autoscaling.Concurrency), WithNoTraffic("ServicesNotReady", "SKS Services are not ready yet"), - WithMSvcStatus(testRevision+"-00001")), + WithMSvcStatus(testRevision+"-00001"), WithSuccessfulBootstrap()), }}, }, { Name: "reconcile sks is still not ready", Objects: []runtime.Object{ hpa(testRevision, testNamespace, - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testRevision, testNamespace, WithHPAClass), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), + pa(testRevision, testNamespace, WithHPAClass, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithPubService, WithPrivateService(testRevision+"-rand")), @@ -154,35 +158,35 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithNoTraffic("ServicesNotReady", "SKS Services are not ready yet"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, Key: key(testRevision, testNamespace), }, { Name: "reconcile sks becomes ready", Objects: []runtime.Object{ hpa(testRevision, testNamespace, - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testRevision, testNamespace, WithHPAClass, WithPAStatusService("the-wrong-one")), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), + pa(testRevision, testNamespace, WithHPAClass, WithPAStatusService("the-wrong-one"), WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testRevision, testNamespace, WithHPAClass, - WithTraffic, WithPAStatusService(testRevision)), + WithTraffic, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, Key: key(testRevision, testNamespace), }, { Name: "reconcile sks", Objects: []runtime.Object{ hpa(testRevision, testNamespace, - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testRevision, testNamespace, WithHPAClass, WithTraffic), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), + pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef("bar"), WithSKSReady), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithPAStatusService(testRevision)), + Object: pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, Key: key(testRevision, testNamespace), WantUpdates: []ktesting.UpdateActionImpl{{ @@ -192,8 +196,8 @@ func TestReconcile(t *testing.T) { Name: "reconcile unhappy sks", Objects: []runtime.Object{ hpa(testRevision, testNamespace, - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testRevision, testNamespace, WithHPAClass, WithTraffic), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), + pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName+"-hairy"), WithPubService, WithPrivateService(testRevision+"-rand")), @@ -201,7 +205,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testRevision, testNamespace, WithHPAClass, WithNoTraffic("ServicesNotReady", "SKS Services are not ready yet"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, Key: key(testRevision, testNamespace), WantUpdates: []ktesting.UpdateActionImpl{{ @@ -210,10 +214,10 @@ func TestReconcile(t *testing.T) { }, { Name: "reconcile sks - update fails", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass, WithTraffic), + pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef("bar"), WithSKSReady), - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), }, Key: key(testRevision, testNamespace), WithReactors: []ktesting.ReactionFunc{ @@ -229,9 +233,9 @@ func TestReconcile(t *testing.T) { }, { Name: "create sks - create fails", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass, WithTraffic), + pa(testRevision, testNamespace, WithHPAClass, WithTraffic, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), }, Key: key(testRevision, testNamespace), WithReactors: []ktesting.ReactionFunc{ @@ -247,15 +251,15 @@ func TestReconcile(t *testing.T) { }, { Name: "sks is disowned", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass), + pa(testRevision, testNamespace, WithHPAClass, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSOwnersRemoved, WithSKSReady), - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), }, Key: key(testRevision, testNamespace), WantErr: true, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, MarkResourceNotOwnedByPA("ServerlessService", testRevision)), + Object: pa(testRevision, testNamespace, WithHPAClass, markActive, MarkResourceNotOwnedByPA("ServerlessService", testRevision)), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", `error reconciling SKS: PA: test-revision does not own SKS: test-revision`), @@ -263,17 +267,17 @@ func TestReconcile(t *testing.T) { }, { Name: "pa is disowned", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass), + pa(testRevision, testNamespace, WithHPAClass, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName)), hpa(testRevision, testNamespace, - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithPAOwnersRemoved), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithPAOwnersRemoved, WithSuccessfulBootstrap()), withHPAOwnersRemoved), }, Key: key(testRevision, testNamespace), WantErr: true, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, MarkResourceNotOwnedByPA("HorizontalPodAutoscaler", testRevision)), + Object: pa(testRevision, testNamespace, WithHPAClass, markActive, MarkResourceNotOwnedByPA("HorizontalPodAutoscaler", testRevision)), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", @@ -283,7 +287,7 @@ func TestReconcile(t *testing.T) { Name: "nop deletion reconcile", // Test that with a DeletionTimestamp we do nothing. Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass, WithPADeletionTimestamp), + pa(testRevision, testNamespace, WithHPAClass, WithPADeletionTimestamp, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), }, Key: key(testRevision, testNamespace), @@ -291,14 +295,14 @@ func TestReconcile(t *testing.T) { Name: "update pa fails", Objects: []runtime.Object{ hpa(testRevision, testNamespace, - pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testRevision, testNamespace, WithHPAClass, WithPAStatusService("the-wrong-one")), + pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), + pa(testRevision, testNamespace, WithHPAClass, WithPAStatusService("the-wrong-one"), WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testRevision, testNamespace, WithHPAClass, - WithTraffic, WithPAStatusService(testRevision)), + WithTraffic, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, Key: key(testRevision, testNamespace), WantErr: true, @@ -312,14 +316,14 @@ func TestReconcile(t *testing.T) { Name: "update hpa fails", Objects: []runtime.Object{ pa(testRevision, testNamespace, WithHPAClass, WithTraffic, - WithPAStatusService(testRevision), WithTargetAnnotation("1")), - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), + WithPAStatusService(testRevision), WithTargetAnnotation("1"), WithSuccessfulBootstrap()), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), deploy(testNamespace, testRevision), }, Key: key(testRevision, testNamespace), WantUpdates: []ktesting.UpdateActionImpl{{ - Object: hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithTargetAnnotation("1"), WithMetricAnnotation("cpu"))), + Object: hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithTargetAnnotation("1"), WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), }}, WantErr: true, WithReactors: []ktesting.ReactionFunc{ @@ -332,8 +336,8 @@ func TestReconcile(t *testing.T) { Name: "update hpa with target usage", Objects: []runtime.Object{ pa(testRevision, testNamespace, WithHPAClass, WithTraffic, - WithPAStatusService(testRevision), WithTargetAnnotation("1")), - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), + WithPAStatusService(testRevision), WithTargetAnnotation("1"), WithSuccessfulBootstrap()), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }, @@ -350,19 +354,19 @@ func TestReconcile(t *testing.T) { }, { Name: "failure to create hpa", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass), + pa(testRevision, testNamespace, WithHPAClass, markActive, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), }, Key: key(testRevision, testNamespace), WantCreates: []runtime.Object{ - hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"))), + hpa(testRevision, testNamespace, pa(testRevision, testNamespace, WithHPAClass, WithMetricAnnotation("cpu"), WithSuccessfulBootstrap())), }, WithReactors: []ktesting.ReactionFunc{ InduceFailure("create", "horizontalpodautoscalers"), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, WithNoTraffic( - "FailedCreate", "Failed to create HorizontalPodAutoscaler \"test-revision\".")), + Object: pa(testRevision, testNamespace, WithHPAClass, markActive, + WithResourceFailedCreation("HorizontalPodAutoscaler", "test-revision")), }}, WantErr: true, WantEvents: []string{ diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 5534324fd30e..af1d6ae5d7a3 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -38,9 +38,11 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/runtime" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" + ) // Reconciler tracks PAs and right sizes the ScaleTargetRef based on the @@ -118,6 +120,9 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler pa.SetDefaults(ctx) pa.Status.InitializeConditions() + if pa.Status.GetCondition(pav1alpha1.PodAutoscalerConditionActive) == nil { + pa.Status.MarkActivating("Initializing", "") + } logger.Debug("PA exists") metricSvc, err := c.ReconcileMetricsService(ctx, pa) @@ -177,6 +182,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler return perrors.Wrap(err, "error re-reconciling SKS") } } + + err = c.computeBootstrapConditions(ctx, pa) + if err != nil { + return perrors.Wrap(err, "error checking bootstrap conditions") + } + return nil } @@ -204,6 +215,56 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *pav1alpha1.PodAut return decider, nil } +func (c *Reconciler) computeBootstrapConditions(ctx context.Context, pa *pav1alpha1.PodAutoscaler) error { + logger := logging.FromContext(ctx) + + // The PodAutoscaler is active, meaning that it has no bootstrap problem. + if pa.Status.IsActive() { + pa.Status.MarkBootstrapSuccessful() + return nil + } + ps, err := resourceutil.GetScaleResource(pa.Namespace, pa.Spec.ScaleTargetRef, c.PSInformerFactory) + if err != nil { + logger.Errorf("Error getting PodScalable: %v", err) + return err + } + // If the PodScalable does not go up from 0, it may be stuck in a bootstrap terminal problem. + if ps.Spec.Replicas != nil && *ps.Spec.Replicas > 0 && ps.Status.ReadyReplicas == 0 { + pods, err := c.KubeClientSet.CoreV1().Pods(pa.Namespace).List(metav1.ListOptions{LabelSelector: metav1.FormatLabelSelector(ps.Spec.Selector)}) + if err != nil { + logger.Errorf("Error getting pods: %v", err) + return err + } else if len(pods.Items) > 0 { + // Arbitrarily grab the very first pod, as they all should be crashing if one does. + pod := pods.Items[0] + + // If pod cannot be scheduled then we expect the container status to be empty. + for _, cond := range pod.Status.Conditions { + if cond.Type == corev1.PodScheduled && cond.Status == corev1.ConditionFalse { + pa.Status.MarkPodUnscheduled(cond.Reason, cond.Message) + break + } + } + + for _, status := range pod.Status.ContainerStatuses { + // Only check user container. + if len(ps.Spec.Template.Spec.Containers) > 0 && status.Name == ps.Spec.Template.Spec.Containers[0].Name { + if t := status.LastTerminationState.Terminated; t != nil { + logger.Infof("%s marking %s exiting with: %d/%s", pa.Name, status.Name, t.ExitCode, t.Message) + pa.Status.MarkContainerExiting(t.ExitCode, t.Message) + } else if w := status.State.Waiting; w != nil && (w.Reason == "ImagePullBackOff" || w.Reason == "ErrImagePull") { + logger.Infof("%s marking %s is waiting with: %s: %s", pa.Name, w.Reason, w.Message) + pa.Status.MarkImagePullError(w.Reason, w.Message) + } + break + } + } + } + } + + return nil +} + func reportMetrics(pa *pav1alpha1.PodAutoscaler, want int32, got int) error { var serviceLabel string var configLabel string @@ -244,7 +305,10 @@ func computeActiveCondition(pa *pav1alpha1.PodAutoscaler, want int32, got int) ( pa.Status.MarkActive() case want == scaleUnknown: - // We don't know what scale we want, so don't touch PA at all. + // Add condition Active if we don't have it. Otherwise don't touch it because it is unknown. + if pa.Status.GetCondition(pav1alpha1.PodAutoscalerConditionActive) == nil { + pa.Status.MarkActivating("Deploying", "") + } } pa.Status.ObservedGeneration = pa.Generation diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 716f1775ccb3..a74c88acb045 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -193,7 +193,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - markOld, WithPAStatusService(testRevision), + WithSuccessfulBootstrap(), markOld, WithPAStatusService(testRevision), withMSvcStatus("rocking-in-the-free-world")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), @@ -210,7 +210,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - markOld, WithPAStatusService(testRevision), + WithSuccessfulBootstrap(), markOld, WithPAStatusService(testRevision), withMSvcStatus("out-of-the-blue")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), @@ -226,12 +226,143 @@ func TestReconcileAndScaleToZero(t *testing.T) { Name: deployName, Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), }}, + }, { + Name: "scale to zero on container exiting", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, + WithContainerExiting(3, "Ouch!!!"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("out-of-the-blue")), + deploy(testNamespace, testRevision), + // Should be present, but empty. + makeSKSPrivateEndpoints(0, testNamespace, testRevision), + }, + WantPatches: []clientgotesting.PatchActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: testNamespace, + }, + Name: deployName, + Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), + }}, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithContainerExiting(3, "Ouch!!!"), WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithServeMode), + }}, + }, { + Name: "scale to zero on unschedulable pod", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, + WithPodUnscheduled("Foo", "Cannot scheduled pod"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("out-of-the-blue")), + deploy(testNamespace, testRevision), + // Should be present, but empty. + makeSKSPrivateEndpoints(0, testNamespace, testRevision), + }, + WantPatches: []clientgotesting.PatchActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: testNamespace, + }, + Name: deployName, + Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), + }}, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithPodUnscheduled("Foo", "Cannot scheduled pod"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithServeMode), + }}, + }, { + Name: "scale to zero on pull image backoff", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, + WithImagePullError("ImagePullBackOff", "missing image"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("out-of-the-blue")), + deploy(testNamespace, testRevision), + // Should be present, but empty. + makeSKSPrivateEndpoints(0, testNamespace, testRevision), + }, + WantPatches: []clientgotesting.PatchActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: testNamespace, + }, + Name: deployName, + Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), + }}, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithImagePullError("ImagePullBackOff", "missing image"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithServeMode), + }}, + }, { + Name: "scale to zero on pull image backoff", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, + WithImagePullError("ErrImagePull", "something is wrong"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("out-of-the-blue")), + deploy(testNamespace, testRevision), + // Should be present, but empty. + makeSKSPrivateEndpoints(0, testNamespace, testRevision), + }, + WantPatches: []clientgotesting.PatchActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: testNamespace, + }, + Name: deployName, + Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), + }}, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithImagePullError("ErrImagePull", "something is wrong"), + WithPAStatusService(testRevision), + withMSvcStatus("out-of-the-blue")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithServeMode), + }}, }, { Name: "from serving to proxy", Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, markOld, - WithPAStatusService(testRevision), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), @@ -242,7 +373,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, @@ -253,7 +384,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, markOld, - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("you-ask-for-this")), @@ -271,7 +402,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, @@ -282,7 +413,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), withMSvcStatus("but-we-give-you-that")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("but-we-give-you-that")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("but-we-give-you-that")), @@ -358,7 +489,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a330-200"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("a330-200")), @@ -370,7 +501,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a350-900ULR"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("b777-200LR")), @@ -391,7 +522,7 @@ func TestReconcile(t *testing.T) { }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), }}, WantCreates: []runtime.Object{ metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -400,7 +531,7 @@ func TestReconcile(t *testing.T) { Name: "metric-service-exists-not-on-status", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision)), + kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("erj-e190")), @@ -409,14 +540,14 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), withMSvcStatus("erj-e190")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("erj-e190")), }}, }, { Name: "delete redundant metrics svc", Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a380-800"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("a380-800")), @@ -446,14 +577,14 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), }}, WantCreates: []runtime.Object{ metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -467,7 +598,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -484,7 +615,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), deploy(testNamespace, testRevision), @@ -506,7 +637,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), deploy(testNamespace, testRevision), @@ -528,7 +659,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a321neo"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -547,7 +678,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("b767-300er"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -567,7 +698,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), withMSvcStatus("b737max-800")), + WithPAStatusService(testRevision), withMSvcStatus("b737max-800"), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), func(s *corev1.Service) { s.OwnerReferences = nil @@ -589,7 +720,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -604,7 +735,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), // SKS is ready here, since its endpoints are populated with Activator endpoints. sks(testNamespace, testRevision, WithProxyMode, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -617,73 +748,76 @@ func TestReconcile(t *testing.T) { WithDeployRef(deployName)), }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "sks is still not ready", Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithTraffic, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithPubService, WithPrivateService(testRevision+"-rand")), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "sks becomes ready", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision), + kpa(testNamespace, testRevision, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "kpa does not become ready without minScale endpoints", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, withMinScale(2)), + kpa(testNamespace, testRevision, withMinScale(2), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActivating, withMinScale(2), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "kpa becomes ready with minScale endpoints", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAStatusService(testRevision)), + kpa(testNamespace, testRevision, markActivating, withMinScale(2), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, makeSKSPrivateEndpoints(2, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, withMinScale(2), WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActive, withMinScale(2), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "sks does not exist", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ // SKS does not exist, so we're just creating and have no status. - Object: kpa(testNamespace, testRevision, markActivating), + Object: kpa(testNamespace, testRevision, markActivating, WithSuccessfulBootstrap()), }}, WantCreates: []runtime.Object{ sks(testNamespace, testRevision, WithDeployRef(deployName)), @@ -692,7 +826,7 @@ func TestReconcile(t *testing.T) { Name: "sks is out of whack", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef("bar"), WithPubService), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -700,7 +834,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ // SKS just got updated and we don't have up to date status. - Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithPubService, @@ -710,7 +844,7 @@ func TestReconcile(t *testing.T) { Name: "sks cannot be created", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, }, @@ -729,7 +863,7 @@ func TestReconcile(t *testing.T) { Name: "sks cannot be updated", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef("bar")), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -748,7 +882,7 @@ func TestReconcile(t *testing.T) { Name: "sks is disowned", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithSKSOwnersRemoved), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -756,7 +890,7 @@ func TestReconcile(t *testing.T) { }, WantErr: true, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markResourceNotOwned("ServerlessService", testRevision)), + Object: kpa(testNamespace, testRevision, markActive, markResourceNotOwned("ServerlessService", testRevision)), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "error reconciling SKS: PA: test-revision does not own SKS: test-revision"), diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go index 7f1b2ad47e50..c7d0d05bd3c8 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -132,24 +132,28 @@ func applyBounds(min, max, x int32) int32 { } func (ks *scaler) handleScaleToZero(pa *pav1alpha1.PodAutoscaler, desiredScale int32, config *autoscaler.Config) (int32, bool) { + // Check preconditions: + // (1) we want to scale to 0 (desiredScale is 0) + // (2) enable-scale-to-zero from configmap is true + // (3) PA's Ready condition is not false. if desiredScale != 0 { return desiredScale, true } - - // We should only scale to zero when three of the following conditions are true: - // a) enable-scale-to-zero from configmap is true - // b) The PA has been active for at least the stable window, after which it gets marked inactive - // c) The PA has been inactive for at least the grace period - if !config.EnableScaleToZero { return 1, true } + if pa.Status.HasFailed() { + return desiredScale, true + } - if pa.Status.IsActivating() { // Active=Unknown - return scaleUnknown, false - } else if pa.Status.IsReady() { // Active=True - // Don't scale-to-zero if the PA is active + // When the preconditions are met, we scale to 0 based on the Active condition: + // (1) The PA has been active for at least the stable window, after which it gets marked inactive + // (2) The PA has been inactive for at least the grace period + if pa.Status.IsActivating() { + // Don't scale-to-zero during activation + return scaleUnknown, false + } else if pa.Status.IsActive() { // Do not scale to 0, but return desiredScale of 0 to mark PA inactive. sw := aresources.StableWindow(pa, config) if pa.Status.CanMarkInactive(sw) { @@ -223,7 +227,12 @@ func (ks *scaler) Scale(ctx context.Context, pa *pav1alpha1.PodAutoscaler, desir if desiredScale < 0 { logger.Debug("Metrics are not yet being collected.") - return desiredScale, nil + if pa.Status.HasFailed() { + // Want to scale to 0 when the PA has failed. + desiredScale = 0 + } else { + return desiredScale, nil + } } min, max := pa.ScaleBounds() diff --git a/pkg/reconciler/autoscaling/kpa/scaler_test.go b/pkg/reconciler/autoscaling/kpa/scaler_test.go index f65baba14ef8..bfc61a773c48 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler_test.go +++ b/pkg/reconciler/autoscaling/kpa/scaler_test.go @@ -102,6 +102,16 @@ func TestScaler(t *testing.T) { kpaMarkActive(k, time.Now().Add(-paStableWindow).Add(1*time.Second)) }, wantCBCount: 1, + }, { + label: "waits to scale to zero after idle period (custom PA window)", + startReplicas: 1, + scaleTo: 0, + wantReplicas: 0, + wantScaling: false, + kpaMutation: func(k *pav1alpha1.PodAutoscaler) { + WithWindowAnnotation(paStableWindow.String())(k) + kpaMarkActive(k, time.Now().Add(-paStableWindow)) + }, }, { label: "custom PA window, check for standard window, no probe", startReplicas: 1, @@ -132,15 +142,15 @@ func TestScaler(t *testing.T) { kpaMarkActive(k, time.Now().Add(-stableWindow)) }, }, { - label: "waits to scale to zero after idle period (custom PA window)", + label: "waits to scale to zero (just before grace period)", startReplicas: 1, scaleTo: 0, wantReplicas: 0, wantScaling: false, kpaMutation: func(k *pav1alpha1.PodAutoscaler) { - WithWindowAnnotation(paStableWindow.String())(k) - kpaMarkActive(k, time.Now().Add(-paStableWindow)) + kpaMarkInactive(k, time.Now().Add(-gracePeriod).Add(1*time.Second)) }, + wantCBCount: 1, }, { label: "scale to zero after grace period", startReplicas: 1, @@ -150,16 +160,6 @@ func TestScaler(t *testing.T) { kpaMutation: func(k *pav1alpha1.PodAutoscaler) { kpaMarkInactive(k, time.Now().Add(-gracePeriod)) }, - }, { - label: "waits to scale to zero (just before grace period)", - startReplicas: 1, - scaleTo: 0, - wantReplicas: 0, - wantScaling: false, - kpaMutation: func(k *pav1alpha1.PodAutoscaler) { - kpaMarkInactive(k, time.Now().Add(-gracePeriod).Add(1*time.Second)) - }, - wantCBCount: 1, }, { label: "scale to zero after grace period, but fail prober", startReplicas: 1, @@ -184,6 +184,42 @@ func TestScaler(t *testing.T) { }, proberfunc: func(*pav1alpha1.PodAutoscaler, http.RoundTripper) (bool, error) { return false, nil }, wantAsyncProbeCount: 1, + }, { + label: "scale to zero when pod unschedulable", + startReplicas: 1, + scaleTo: -1, + wantReplicas: 0, + wantScaling: true, + kpaMutation: func(k *pav1alpha1.PodAutoscaler) { + k.Status.MarkPodUnscheduled("PodUnschedulable", "") + }, + }, { + label: "scale to zero when container exiting", + startReplicas: 1, + scaleTo: -1, + wantReplicas: 0, + wantScaling: true, + kpaMutation: func(k *pav1alpha1.PodAutoscaler) { + k.Status.MarkContainerExiting(3, "Ouch!") + }, + }, { + label: "scale to zero when ImagePullBackOff", + startReplicas: 1, + scaleTo: -1, + wantReplicas: 0, + wantScaling: true, + kpaMutation: func(k *pav1alpha1.PodAutoscaler) { + k.Status.MarkImagePullError("ImagePullBackOff", "Cannot pull image") + }, + }, { + label: "scale to zero when ErrPullImage", + startReplicas: 1, + scaleTo: -1, + wantReplicas: 0, + wantScaling: true, + kpaMutation: func(k *pav1alpha1.PodAutoscaler) { + k.Status.MarkImagePullError("ErrPullImage", "Cannot pull image") + }, }, { label: "does not scale while activating", startReplicas: 1, diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 88cf96881556..786a881046ae 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -65,39 +65,6 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi } } - // If a container keeps crashing (no active pods in the deployment although we want some) - if *deployment.Spec.Replicas > 0 && deployment.Status.AvailableReplicas == 0 { - pods, err := c.KubeClientSet.CoreV1().Pods(ns).List(metav1.ListOptions{LabelSelector: metav1.FormatLabelSelector(deployment.Spec.Selector)}) - if err != nil { - logger.Errorf("Error getting pods: %v", err) - } else if len(pods.Items) > 0 { - // Arbitrarily grab the very first pod, as they all should be crashing - pod := pods.Items[0] - - // Update the revision status if pod cannot be scheduled(possibly resource constraints) - // If pod cannot be scheduled then we expect the container status to be empty. - for _, cond := range pod.Status.Conditions { - if cond.Type == corev1.PodScheduled && cond.Status == corev1.ConditionFalse { - rev.Status.MarkResourcesUnavailable(cond.Reason, cond.Message) - break - } - } - - for _, status := range pod.Status.ContainerStatuses { - if status.Name == rev.Spec.GetContainer().Name { - if t := status.LastTerminationState.Terminated; t != nil { - logger.Infof("%s marking exiting with: %d/%s", rev.Name, t.ExitCode, t.Message) - rev.Status.MarkContainerExiting(t.ExitCode, t.Message) - } else if w := status.State.Waiting; w != nil && hasDeploymentTimedOut(deployment) { - logger.Infof("%s marking resources unavailable with: %s: %s", rev.Name, w.Reason, w.Message) - rev.Status.MarkResourcesUnavailable(w.Reason, w.Message) - } - break - } - } - } - } - // Now that we have a Deployment, determine whether there is any relevant // status to surface in the Revision. if hasDeploymentTimedOut(deployment) && !rev.Status.IsActivationRequired() { @@ -176,7 +143,7 @@ func (c *Reconciler) reconcileKPA(ctx context.Context, rev *v1alpha1.Revision) e rev.Status.ServiceName = kpa.Status.ServiceName // Reflect the KPA status in our own. - cond := kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionReady) + cond := kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionActive) switch { case cond == nil: rev.Status.MarkActivating("Deploying", "") @@ -187,12 +154,19 @@ func (c *Reconciler) reconcileKPA(ctx context.Context, rev *v1alpha1.Revision) e rev.Status.MarkInactive(cond.Reason, cond.Message) case cond.Status == corev1.ConditionTrue: rev.Status.MarkActive() + } - // Precondition for PA being active is SKS being active and - // that entices that |service.endpoints| > 0. - rev.Status.MarkResourcesAvailable() - rev.Status.MarkContainerHealthy() + cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionReady) + if cond != nil { + switch { + case cond.Status == corev1.ConditionTrue: + rev.Status.MarkContainerHealthy() + rev.Status.MarkResourcesAvailable() + case cond.Status == corev1.ConditionFalse: + rev.Status.MarkContainerUnhealthy(cond.Reason, cond.Message) + } } + return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index fc9c44c705b5..b2bcc076e834 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -103,6 +103,7 @@ func testReadyKPA(rev *v1alpha1.Revision) *av1alpha1.PodAutoscaler { kpa := resources.MakeKPA(rev) kpa.Status.InitializeConditions() kpa.Status.MarkActive() + kpa.Status.MarkBootstrapSuccessful() kpa.Status.ServiceName = serviceName(rev.Name) return kpa } diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index ba64ecd90a4a..b081300d3ce0 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -257,7 +257,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "kpa-ready", withK8sServiceName("old-stuff"), WithLogURL, AllUnknownConditions), - kpa("foo", "kpa-ready", WithTraffic, WithPAStatusService("new-stuff")), + kpa("foo", "kpa-ready", WithTraffic, WithPAStatusService("new-stuff"), WithSuccessfulBootstrap()), deploy("foo", "kpa-ready"), image("foo", "kpa-ready"), }, @@ -345,7 +345,7 @@ func TestReconcile(t *testing.T) { rev("foo", "fix-mutated-kpa", withK8sServiceName("ill-follow-the-sun"), WithLogURL, MarkRevisionReady), kpa("foo", "fix-mutated-kpa", WithProtocolType(networking.ProtocolH2C), - WithTraffic, WithPAStatusService("fix-mutated-kpa")), + WithTraffic, WithPAStatusService("fix-mutated-kpa"), WithSuccessfulBootstrap()), deploy("foo", "fix-mutated-kpa"), image("foo", "fix-mutated-kpa"), }, @@ -358,7 +358,7 @@ func TestReconcile(t *testing.T) { }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa("foo", "fix-mutated-kpa", WithTraffic, - WithPAStatusService("fix-mutated-kpa")), + WithPAStatusService("fix-mutated-kpa"), WithSuccessfulBootstrap()), }}, Key: "foo/fix-mutated-kpa", }, { @@ -416,15 +416,14 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "pull-backoff", withK8sServiceName("the-taxman"), WithLogURL, MarkActivating("Deploying", "")), - kpa("foo", "pull-backoff"), // KPA can't be ready since deployment times out. - pod("foo", "pull-backoff", WithWaitingContainer("user-container", "ImagePullBackoff", "can't pull it")), + kpa("foo", "pull-backoff", WithImagePullError("ImagePullBackoff", "can't pull it")), timeoutDeploy(deploy("foo", "pull-backoff")), image("foo", "pull-backoff"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pull-backoff", WithLogURL, AllUnknownConditions, - MarkResourcesUnavailable("ImagePullBackoff", "can't pull it")), + MarkContainerUnhealthy("ImagePullBackoff", "can't pull it")), }}, Key: "foo/pull-backoff", }, { @@ -436,14 +435,14 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "pod-error", withK8sServiceName("a-pod-error"), WithLogURL, AllUnknownConditions, MarkActive), - kpa("foo", "pod-error"), // PA can't be ready, since no traffic. + kpa("foo", "pod-error", WithContainerExiting(5, "I failed man!")), pod("foo", "pod-error", WithFailingContainer("user-container", 5, "I failed man!")), deploy("foo", "pod-error"), image("foo", "pod-error"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-error", - WithLogURL, AllUnknownConditions, MarkContainerExiting(5, "I failed man!")), + WithLogURL, AllUnknownConditions, MarkContainerUnhealthy("ExitCode5", "Container failed with: I failed man!")), }}, Key: "foo/pod-error", }, { @@ -454,14 +453,13 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "pod-schedule-error", withK8sServiceName("a-pod-schedule-error"), WithLogURL, AllUnknownConditions, MarkActive), - kpa("foo", "pod-schedule-error"), // PA can't be ready, since no traffic. - pod("foo", "pod-schedule-error", WithUnschedulableContainer("Insufficient energy", "Unschedulable")), + kpa("foo", "pod-schedule-error", WithPodUnscheduled("Insufficient energy", "Unschedulable")), deploy("foo", "pod-schedule-error"), image("foo", "pod-schedule-error"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-schedule-error", - WithLogURL, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable")), + WithLogURL, AllUnknownConditions, MarkContainerUnhealthy("Insufficient energy", "Unschedulable")), }}, Key: "foo/pod-schedule-error", }, { @@ -473,7 +471,7 @@ func TestReconcile(t *testing.T) { // This signal should make our Reconcile mark the Revision as Ready. Objects: []runtime.Object{ rev("foo", "steady-ready", withK8sServiceName("very-steady"), WithLogURL), - kpa("foo", "steady-ready", WithTraffic, WithPAStatusService("steadier-even")), + kpa("foo", "steady-ready", WithTraffic, WithPAStatusService("steadier-even"), WithSuccessfulBootstrap()), deploy("foo", "steady-ready"), image("foo", "steady-ready"), }, diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index 2cb78d295812..b2111e7903c1 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -84,6 +84,48 @@ func WithNoTraffic(reason, message string) PodAutoscalerOption { } } +// WithSuccessfulBootstrap updates the PA to have Bootstrap condition true. +func WithSuccessfulBootstrap() PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkBootstrapSuccessful() + } +} + +// WithContainerExiting updates the PA to have Bootstrap condition false with the exit code and message. +func WithContainerExiting(exitCode int32, message string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkContainerExiting(exitCode, message) + } +} + +// WithImagePullError updates the PA to have Bootstrap condition false. +func WithImagePullError(reason, message string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkImagePullError(reason, message) + } +} + +// WithPodUnscheduled updates the PA to have Bootstrap condition false. +func WithPodUnscheduled(reason, message string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkPodUnscheduled(reason, message) + } +} + +// WithResourceNotOwned updates the PA to have Bootstrap condition false. +func WithResourceNotOwned(kind, name string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkResourceNotOwned(kind, name) + } +} + +// WithResourceFailedCreation updates the PA to have Bootstrap condition false. +func WithResourceFailedCreation(kind, name string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkResourceFailedCreation(kind, name) + } +} + // WithPADeletionTimestamp will set the DeletionTimestamp on the PodAutoscaler. func WithPADeletionTimestamp(r *autoscalingv1alpha1.PodAutoscaler) { t := metav1.NewTime(time.Unix(1e9, 0)) @@ -342,6 +384,11 @@ func WithProxyMode(sks *netv1alpha1.ServerlessService) { sks.Spec.Mode = netv1alpha1.SKSOperationModeProxy } +// WithServeMode puts SKS into serve mode. +func WithServeMode(sks *netv1alpha1.ServerlessService) { + sks.Spec.Mode = netv1alpha1.SKSOperationModeServe +} + // SKS creates a generic ServerlessService object. func SKS(ns, name string, so ...SKSOption) *netv1alpha1.ServerlessService { s := &netv1alpha1.ServerlessService{ diff --git a/pkg/testing/v1alpha1/revision.go b/pkg/testing/v1alpha1/revision.go index b0580f221c7a..4a71acb9e51c 100644 --- a/pkg/testing/v1alpha1/revision.go +++ b/pkg/testing/v1alpha1/revision.go @@ -132,10 +132,10 @@ func MarkContainerMissing(rev *v1alpha1.Revision) { rev.Status.MarkContainerMissing("It's the end of the world as we know it") } -// MarkContainerExiting calls .Status.MarkContainerExiting on the Revision. -func MarkContainerExiting(exitCode int32, message string) RevisionOption { +// MarkContainerUnhealthy is an adaptor to call .Status.MarkContainerUnhealthy on the Revision. +func MarkContainerUnhealthy(reason, message string) RevisionOption { return func(r *v1alpha1.Revision) { - r.Status.MarkContainerExiting(exitCode, message) + r.Status.MarkContainerUnhealthy(reason, message) } }