From 078dc21165cfaf504747e42d81f0f5af8d8e26ca Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Mon, 17 Jun 2019 18:27:32 -0700 Subject: [PATCH 01/14] Revise PodAutoscaler's life cycle Currently PodAutoscaler's Ready condition is the same as Active condition. It is not only mismatched with Revision's Ready condition, where the Active condition is only an informative condition that does not define the Ready condition, but also not capturing the PodAutoscaler's status very well. For example, when everything is good but there's no traffic, the PodAutoscaler should be Ready, but Inactive. Changes are: 1. Make Active an informational condition for PodAutoscaler. 2. Define ContainersHealthy and PodsHealthy as living conditions for PodAutoscaler's Ready. 3. PodAutoscaler checks pods and determines PodsHealthy and ContainersHealthy conditions. 4. Revision reads PodAutoscaler's PodsHealthy and ContainersHealthy conditions and updates its ContainerHealthy and ResourceAvailable conditions. Note that Revision can have extra checks for those conditions. 5. PodAutoscaler scales down (to 0 if possible) when it fails to become Ready. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 42 +++- pkg/apis/autoscaling/v1alpha1/pa_types.go | 6 +- .../serving/v1alpha1/revision_lifecycle.go | 11 +- pkg/reconciler/autoscaling/kpa/kpa.go | 66 +++++- pkg/reconciler/autoscaling/kpa/kpa_test.go | 211 ++++++++++-------- pkg/reconciler/autoscaling/kpa/scaler.go | 31 ++- .../autoscaling/resources/names/names.go | 5 + .../revision/reconcile_resources.go | 54 ++--- pkg/testing/functional.go | 14 ++ pkg/testing/v1alpha1/revision.go | 5 - 10 files changed, 285 insertions(+), 160 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index 6606ac54058d..2454ae7be81e 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -29,7 +29,8 @@ import ( ) var podCondSet = apis.NewLivingConditionSet( - PodAutoscalerConditionActive, + PodAutoscalerConditionContainersHealthy, + PodAutoscalerConditionPodsHealthy, ) func (pa *PodAutoscaler) GetGroupVersionKind() schema.GroupVersionKind { @@ -127,6 +128,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 { @@ -140,6 +147,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,6 +192,33 @@ func (pas *PodAutoscalerStatus) MarkResourceFailedCreation(kind, name string) { fmt.Sprintf("Failed to create %s %q.", kind, name)) } +// MarkContainerExiting changes the "ContainersHealthy" 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(PodAutoscalerConditionContainersHealthy, exitCodeString, message) +} + +// MarkContainerWaiting changes the "ContainersHealthy" condition to false and records the exit code. +func (pas *PodAutoscalerStatus) MarkContainerWaiting(reason, message string) { + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionContainersHealthy, reason, message) +} + +// MarkContainersHealthy changes the "ContainersHealthy" condition to true. +func (pas *PodAutoscalerStatus) MarkContainersHealthy() { + podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionContainersHealthy) +} + +// MarkPodUnscheduled changes the "PodsHealthy" condition to false and records reason and message. +func (pas *PodAutoscalerStatus) MarkPodUnscheduled(reason, message string) { + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionPodsHealthy, reason, message) +} + +// MarkPodsHealthy changes the "PodsHealthy" condition to true. +func (pas *PodAutoscalerStatus) MarkPodsHealthy() { + podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionPodsHealthy) +} + // 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_types.go b/pkg/apis/autoscaling/v1alpha1/pa_types.go index 5d50ffa04dc8..b9d8adbf609f 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_types.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_types.go @@ -99,8 +99,12 @@ 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" + // PodAutoscalerConditionContainersHealthy becomes true when the containers are up and running. + PodAutoscalerConditionContainersHealthy apis.ConditionType = "ContainersHealthy" + // PodAutoscalerConditionPodsHealthy becomes true when the pods are up and running. + PodAutoscalerConditionPodsHealthy apis.ConditionType = "PodsHealthy" ) // PodAutoscalerStatus communicates the observed state of the PodAutoscaler (from the controller). diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle.go b/pkg/apis/serving/v1alpha1/revision_lifecycle.go index b54212249b50..e559573ae396 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle.go @@ -169,9 +169,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() { @@ -206,12 +205,6 @@ 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" diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 40c1a1a9b154..6525cf207cf0 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 @@ -123,6 +125,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) @@ -182,6 +187,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler return perrors.Wrap(err, "error re-reconciling SKS") } } + + err = c.updateContainerHealth(ctx, pa) + if err != nil { + return perrors.Wrap(err, "error checking container health") + } + return nil } @@ -209,6 +220,56 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *pav1alpha1.PodAut return decider, nil } +func (c *Reconciler) updateContainerHealth(ctx context.Context, pa *pav1alpha1.PodAutoscaler) error { + logger := logging.FromContext(ctx) + + if pa.Status.IsActive() { + pa.Status.MarkContainersHealthy() + pa.Status.MarkPodsHealthy() + return nil + } + ps, err := resourceutil.GetScaleResource(pa.Namespace, pa.Spec.ScaleTargetRef, c.PSInformerFactory) + if err != nil { + logger.Errorf("Error getting deployment: %v", err) + return err + } + // If a container keeps crashing (no active pods in the deployment although we want some) + if ps.Spec.Replicas != nil && *ps.Spec.Replicas > 0 && ps.Status.Replicas == 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 { + logger.Infof("%s marking %s is waiting with: %s: %s", pa.Name, w.Reason, w.Message) + pa.Status.MarkContainerWaiting(w.Reason, w.Message) + } + break + } + } + } + } + + return nil +} + func reportMetrics(pa *pav1alpha1.PodAutoscaler, want int32, got int) error { var serviceLabel string var configLabel string @@ -249,7 +310,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 41d74b8d969c..f48618774aba 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -19,6 +19,7 @@ package kpa import ( "context" "fmt" + "github.com/knative/pkg/ptr" "strconv" "sync" "testing" @@ -45,7 +46,6 @@ import ( "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" logtesting "github.com/knative/pkg/logging/testing" - "github.com/knative/pkg/ptr" "github.com/knative/pkg/system" _ "github.com/knative/pkg/system/testing" "github.com/knative/serving/pkg/apis/autoscaling" @@ -191,6 +191,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithHealthyContainers(), WithHealthyPods(), markOld, WithPAStatusService(testRevision), withMSvcStatus("rocking-in-the-free-world")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), @@ -202,12 +203,13 @@ func TestReconcileAndScaleToZero(t *testing.T) { // Should be present, but empty. makeSKSPrivateEndpoints(0, testNamespace, testRevision), }, - }, { + }, { Name: "steady not serving (scale to zero)", Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithHealthyContainers(), WithHealthyPods(), markOld, WithPAStatusService(testRevision), withMSvcStatus("out-of-the-blue")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), @@ -224,69 +226,74 @@ func TestReconcileAndScaleToZero(t *testing.T) { Name: deployName, Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), }}, - }, { - Name: "from serving to proxy", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markOld, - WithPAStatusService(testRevision), - withMSvcStatus("and-into-the-black")), - sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), - metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), - withMSvcName("and-into-the-black")), - deploy(testNamespace, testRevision), - makeSKSPrivateEndpoints(1, testNamespace, testRevision), - }, - WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, - WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), - }}, - WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: sks(testNamespace, testRevision, WithSKSReady, - WithDeployRef(deployName), WithProxyMode), - }}, - }, { - Name: "from serving to proxy, sks update fail :-(", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markOld, - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), - sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), - metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), - withMSvcName("you-ask-for-this")), - deploy(testNamespace, testRevision), - makeSKSPrivateEndpoints(1, testNamespace, testRevision), - }, - WantErr: true, - WithReactors: []clientgotesting.ReactionFunc{ - InduceFailure("update", "serverlessservices"), - }, - WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", - "error re-reconciling SKS: error updating SKS test-revision: inducing failure for update serverlessservices"), - }, - WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, - WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), - }}, - WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: sks(testNamespace, testRevision, WithSKSReady, - WithDeployRef(deployName), WithProxyMode), - }}, - }, { - Name: "scaling to 0, but not stable for long enough, so no-op", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, - 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")), - deploy(testNamespace, testRevision), - makeSKSPrivateEndpoints(1, testNamespace, testRevision), - }, + }, { + Name: "from serving to proxy", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, markActive, markOld, + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), + withMSvcStatus("and-into-the-black")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("and-into-the-black")), + deploy(testNamespace, testRevision), + makeSKSPrivateEndpoints(1, testNamespace, testRevision), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithProxyMode), + }}, + }, { + Name: "from serving to proxy, sks update fail :-(", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, markActive, markOld, + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("you-ask-for-this")), + deploy(testNamespace, testRevision), + makeSKSPrivateEndpoints(1, testNamespace, testRevision), + }, + WantErr: true, + WithReactors: []clientgotesting.ReactionFunc{ + InduceFailure("update", "serverlessservices"), + }, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", + "error re-reconciling SKS: error updating SKS test-revision: inducing failure for update serverlessservices"), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithProxyMode), + }}, + }, { + Name: "scaling to 0, but not stable for long enough, so no-op", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, markActive, + WithHealthyContainers(), WithHealthyPods(), + 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")), + deploy(testNamespace, testRevision), + makeSKSPrivateEndpoints(1, testNamespace, testRevision), + }, }} defer logtesting.ClearAll() @@ -356,7 +363,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a330-200"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("a330-200")), @@ -368,7 +375,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a350-900ULR"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("b777-200LR")), @@ -389,6 +396,7 @@ func TestReconcile(t *testing.T) { }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, + WithHealthyContainers(), WithHealthyPods(), WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), }}, WantCreates: []runtime.Object{ @@ -398,7 +406,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), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("erj-e190")), @@ -407,6 +415,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, + WithHealthyContainers(), WithHealthyPods(), WithPAStatusService(testRevision), withMSvcStatus("erj-e190")), }}, }, { @@ -414,7 +423,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a380-800"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("a380-800")), @@ -444,13 +453,14 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, + WithHealthyContainers(), WithHealthyPods(), WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), }}, WantCreates: []runtime.Object{ @@ -465,7 +475,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -482,7 +492,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), deploy(testNamespace, testRevision), @@ -504,7 +514,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), deploy(testNamespace, testRevision), @@ -526,7 +536,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a321neo"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -545,7 +555,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("b767-300er"), - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -565,7 +575,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"), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), func(s *corev1.Service) { s.OwnerReferences = nil @@ -577,7 +587,8 @@ func TestReconcile(t *testing.T) { Object: kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), withMSvcStatus("b737max-800"), // We expect this change in status: - markResourceNotOwned("Service", "b737max-800")), + markResourceNotOwned("Service", "b737max-800"), + WithHealthyContainers(), WithHealthyPods()), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "error reconciling metrics service: PA: test-revision does not own Service: b737max-800"), @@ -587,7 +598,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -602,7 +613,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), WithHealthyContainers(), WithHealthyPods(),), // SKS is ready here, since its endpoints are populated with Activator endpoints. sks(testNamespace, testRevision, WithProxyMode, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -615,73 +626,79 @@ func TestReconcile(t *testing.T) { WithDeployRef(deployName)), }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision)), + Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), + WithHealthyContainers(), WithHealthyPods()), }}, }, { Name: "sks is still not ready", Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithTraffic, - WithPAStatusService(testRevision)), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), 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), + WithHealthyContainers(), WithHealthyPods()), }}, }, { Name: "sks becomes ready", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision), + kpa(testNamespace, testRevision, WithHealthyContainers(), WithHealthyPods()), 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), + WithHealthyContainers(), WithHealthyPods()), }}, }, { Name: "kpa does not become ready without minScale endpoints", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, withMinScale(2)), + kpa(testNamespace, testRevision, withMinScale(2), WithHealthyContainers(), WithHealthyPods()), 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), WithHealthyContainers(), WithHealthyPods()), }}, }, { 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), + WithHealthyContainers(), WithHealthyPods()), 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), WithHealthyContainers(), WithHealthyPods()), }}, }, { Name: "sks does not exist", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), 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, WithHealthyContainers(), WithHealthyPods()), }}, WantCreates: []runtime.Object{ sks(testNamespace, testRevision, WithDeployRef(deployName)), @@ -690,7 +707,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, WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef("bar"), WithPubService), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -698,7 +715,8 @@ 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), + WithHealthyContainers(), WithHealthyPods()), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithPubService, @@ -708,7 +726,7 @@ func TestReconcile(t *testing.T) { Name: "sks cannot be created", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, }, @@ -727,7 +745,7 @@ func TestReconcile(t *testing.T) { Name: "sks cannot be updated", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef("bar")), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -746,7 +764,7 @@ func TestReconcile(t *testing.T) { Name: "sks is disowned", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive), + kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithSKSOwnersRemoved), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -754,7 +772,8 @@ func TestReconcile(t *testing.T) { }, WantErr: true, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markResourceNotOwned("ServerlessService", testRevision)), + Object: kpa(testNamespace, testRevision, markResourceNotOwned("ServerlessService", testRevision), + WithHealthyContainers(), WithHealthyPods()), }}, 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 81404c6f2fa7..013ce0b6fbca 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -118,24 +118,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. if pa.Status.CanMarkInactive(config.StableWindow) { // We do not need to enqueue PA here, since this will @@ -208,7 +212,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/resources/names/names.go b/pkg/reconciler/autoscaling/resources/names/names.go index 3ad5b4b88eed..ca1ea9846483 100644 --- a/pkg/reconciler/autoscaling/resources/names/names.go +++ b/pkg/reconciler/autoscaling/resources/names/names.go @@ -21,3 +21,8 @@ package names func SKS(paName string) string { return paName } + +// Deployment returns the name of the Deployment resource scaled by this PA. +func Deployment(paName string) string { + return paName + "-deployment" +} diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 2abece393358..c028ab7fb020 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -64,39 +64,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() { @@ -175,7 +142,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", "") @@ -186,11 +153,26 @@ 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 + cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionContainersHealthy) + switch { + case cond.Status == corev1.ConditionTrue: + rev.Status.MarkContainerHealthy() + case cond.Status == corev1.ConditionFalse: + rev.Status.MarkContainerUnhealthy(cond.Reason, cond.Message) + } + + cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionPodsHealthy) + if cond.Status == corev1.ConditionFalse { + rev.Status.MarkResourcesUnavailable(cond.Reason, cond.Message) + } + + if kpa.Status.IsReady() { + // Precondition for PA being ready is SKS being active and // that entices that |service.endpoints| > 0. rev.Status.MarkResourcesAvailable() - rev.Status.MarkContainerHealthy() } + return nil } diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index b725a596bbd7..6cb7e7c3978b 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -83,6 +83,20 @@ func WithNoTraffic(reason, message string) PodAutoscalerOption { } } +// WithHealthyContainers updates the PA to have ContainersHealthy condition true. +func WithHealthyContainers() PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkContainersHealthy() + } +} + +// WithHealthyPods updates the PA to have PodsHealthy condition true. +func WithHealthyPods() PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkPodsHealthy() + } +} + // WithPADeletionTimestamp will set the DeletionTimestamp on the PodAutoscaler. func WithPADeletionTimestamp(r *autoscalingv1alpha1.PodAutoscaler) { t := metav1.NewTime(time.Unix(1e9, 0)) diff --git a/pkg/testing/v1alpha1/revision.go b/pkg/testing/v1alpha1/revision.go index 33fef02d6bed..b0580f221c7a 100644 --- a/pkg/testing/v1alpha1/revision.go +++ b/pkg/testing/v1alpha1/revision.go @@ -127,11 +127,6 @@ func MarkProgressDeadlineExceeded(r *v1alpha1.Revision) { r.Status.MarkProgressDeadlineExceeded("Unable to create pods for more than 120 seconds.") } -// MarkServiceTimeout calls .Status.MarkServiceTimeout on the Revision. -func MarkServiceTimeout(r *v1alpha1.Revision) { - r.Status.MarkServiceTimeout() -} - // MarkContainerMissing calls .Status.MarkContainerMissing on the Revision. func MarkContainerMissing(rev *v1alpha1.Revision) { rev.Status.MarkContainerMissing("It's the end of the world as we know it") From e73d2a57e61c0c754ed2d310008adebb8a7ac216 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Wed, 19 Jun 2019 11:44:18 -0700 Subject: [PATCH 02/14] Fixed revision tests --- .../serving/v1alpha1/revision_lifecycle.go | 2 - pkg/reconciler/autoscaling/kpa/kpa.go | 4 +- pkg/reconciler/autoscaling/kpa/kpa_test.go | 140 +++++++++--------- .../revision/reconcile_resources.go | 14 +- pkg/reconciler/revision/revision_test.go | 2 + pkg/reconciler/revision/table_test.go | 22 +-- pkg/testing/functional.go | 21 +++ pkg/testing/v1alpha1/revision.go | 6 +- 8 files changed, 117 insertions(+), 94 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle.go b/pkg/apis/serving/v1alpha1/revision_lifecycle.go index e559573ae396..066f21176949 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle.go @@ -208,8 +208,6 @@ func RevisionContainerMissingMessage(image string, message string) string { const ( AnnotationParseErrorTypeMissing = "Missing" AnnotationParseErrorTypeInvalid = "Invalid" - LabelParserErrorTypeMissing = "Missing" - LabelParserErrorTypeInvalid = "Invalid" ) // +k8s:deepcopy-gen=false diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 6525cf207cf0..1593443e5c13 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -188,7 +188,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler } } - err = c.updateContainerHealth(ctx, pa) + err = c.updateHealthConditions(ctx, pa) if err != nil { return perrors.Wrap(err, "error checking container health") } @@ -220,7 +220,7 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *pav1alpha1.PodAut return decider, nil } -func (c *Reconciler) updateContainerHealth(ctx context.Context, pa *pav1alpha1.PodAutoscaler) error { +func (c *Reconciler) updateHealthConditions(ctx context.Context, pa *pav1alpha1.PodAutoscaler) error { logger := logging.FromContext(ctx) if pa.Status.IsActive() { diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index f48618774aba..8e80d6598037 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -203,7 +203,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { // Should be present, but empty. makeSKSPrivateEndpoints(0, testNamespace, testRevision), }, - }, { + }, { Name: "steady not serving (scale to zero)", Key: key, Objects: []runtime.Object{ @@ -226,74 +226,74 @@ func TestReconcileAndScaleToZero(t *testing.T) { Name: deployName, Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`), }}, - }, { - Name: "from serving to proxy", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markOld, - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), - withMSvcStatus("and-into-the-black")), - sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), - metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), - withMSvcName("and-into-the-black")), - deploy(testNamespace, testRevision), - makeSKSPrivateEndpoints(1, testNamespace, testRevision), - }, - WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, - WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), - }}, - WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: sks(testNamespace, testRevision, WithSKSReady, - WithDeployRef(deployName), WithProxyMode), - }}, - }, { - Name: "from serving to proxy, sks update fail :-(", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markOld, - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), - sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), - metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), - withMSvcName("you-ask-for-this")), - deploy(testNamespace, testRevision), - makeSKSPrivateEndpoints(1, testNamespace, testRevision), - }, - WantErr: true, - WithReactors: []clientgotesting.ReactionFunc{ - InduceFailure("update", "serverlessservices"), - }, - WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", - "error re-reconciling SKS: error updating SKS test-revision: inducing failure for update serverlessservices"), - }, - WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, - WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), - }}, - WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: sks(testNamespace, testRevision, WithSKSReady, - WithDeployRef(deployName), WithProxyMode), - }}, - }, { - Name: "scaling to 0, but not stable for long enough, so no-op", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, - WithHealthyContainers(), WithHealthyPods(), - 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")), - deploy(testNamespace, testRevision), - makeSKSPrivateEndpoints(1, testNamespace, testRevision), - }, + }, { + Name: "from serving to proxy", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, markActive, markOld, + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), + withMSvcStatus("and-into-the-black")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("and-into-the-black")), + deploy(testNamespace, testRevision), + makeSKSPrivateEndpoints(1, testNamespace, testRevision), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithProxyMode), + }}, + }, { + Name: "from serving to proxy, sks update fail :-(", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, markActive, markOld, + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), + metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), + withMSvcName("you-ask-for-this")), + deploy(testNamespace, testRevision), + makeSKSPrivateEndpoints(1, testNamespace, testRevision), + }, + WantErr: true, + WithReactors: []clientgotesting.ReactionFunc{ + InduceFailure("update", "serverlessservices"), + }, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", + "error re-reconciling SKS: error updating SKS test-revision: inducing failure for update serverlessservices"), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: kpa(testNamespace, testRevision, + WithNoTraffic("NoTraffic", "The target is not receiving traffic."), + WithHealthyContainers(), WithHealthyPods(), + WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: sks(testNamespace, testRevision, WithSKSReady, + WithDeployRef(deployName), WithProxyMode), + }}, + }, { + Name: "scaling to 0, but not stable for long enough, so no-op", + Key: key, + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, markActive, + WithHealthyContainers(), WithHealthyPods(), + 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")), + deploy(testNamespace, testRevision), + makeSKSPrivateEndpoints(1, testNamespace, testRevision), + }, }} defer logtesting.ClearAll() @@ -613,7 +613,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods(),), + WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods(), ), // SKS is ready here, since its endpoints are populated with Activator endpoints. sks(testNamespace, testRevision, WithProxyMode, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index c028ab7fb020..814c115f4adf 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -156,15 +156,17 @@ func (c *Reconciler) reconcileKPA(ctx context.Context, rev *v1alpha1.Revision) e } cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionContainersHealthy) - switch { - case cond.Status == corev1.ConditionTrue: - rev.Status.MarkContainerHealthy() - case cond.Status == corev1.ConditionFalse: - rev.Status.MarkContainerUnhealthy(cond.Reason, cond.Message) + if cond != nil { + switch { + case cond.Status == corev1.ConditionTrue: + rev.Status.MarkContainerHealthy() + case cond.Status == corev1.ConditionFalse: + rev.Status.MarkContainerUnhealthy(cond.Reason, cond.Message) + } } cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionPodsHealthy) - if cond.Status == corev1.ConditionFalse { + if cond != nil && cond.Status == corev1.ConditionFalse { rev.Status.MarkResourcesUnavailable(cond.Reason, cond.Message) } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 1bca4825312e..19c2540369ee 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -103,6 +103,8 @@ func testReadyKPA(rev *v1alpha1.Revision) *av1alpha1.PodAutoscaler { kpa := resources.MakeKPA(rev) kpa.Status.InitializeConditions() kpa.Status.MarkActive() + kpa.Status.MarkPodsHealthy() + kpa.Status.MarkContainersHealthy() 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 4f3ba8b573aa..d720fa52d206 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -281,7 +281,8 @@ 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"), + WithHealthyPods(), WithHealthyContainers()), deploy("foo", "kpa-ready"), image("foo", "kpa-ready"), }, @@ -369,7 +370,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"), WithHealthyPods(), WithHealthyContainers()), deploy("foo", "fix-mutated-kpa"), image("foo", "fix-mutated-kpa"), }, @@ -382,7 +383,7 @@ func TestReconcile(t *testing.T) { }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa("foo", "fix-mutated-kpa", WithTraffic, - WithPAStatusService("fix-mutated-kpa")), + WithPAStatusService("fix-mutated-kpa"), WithHealthyPods(), WithHealthyContainers()), }}, Key: "foo/fix-mutated-kpa", }, { @@ -440,15 +441,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", WithImagePullBackoff("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", }, { @@ -460,14 +460,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", }, { @@ -478,8 +478,7 @@ 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"), }, @@ -497,7 +496,8 @@ 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"), + WithHealthyPods(), WithHealthyContainers()), deploy("foo", "steady-ready"), image("foo", "steady-ready"), }, diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index 6cb7e7c3978b..5df4ff125472 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -90,6 +90,20 @@ func WithHealthyContainers() PodAutoscalerOption { } } +// WithContainerExiting updates the PA to have ContainersHealthy 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) + } +} + +// WithImagePullBackoff updates the PA to have ContainersHealthy condition false. +func WithImagePullBackoff(reason, message string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkContainerWaiting(reason, message) + } +} + // WithHealthyPods updates the PA to have PodsHealthy condition true. func WithHealthyPods() PodAutoscalerOption { return func(pa *autoscalingv1alpha1.PodAutoscaler) { @@ -97,6 +111,13 @@ func WithHealthyPods() PodAutoscalerOption { } } +// WithPodUnscheduled updates the PA to have PodsHealthy condition false. +func WithPodUnscheduled(reason, message string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkPodUnscheduled(reason, message) + } +} + // WithPADeletionTimestamp will set the DeletionTimestamp on the PodAutoscaler. func WithPADeletionTimestamp(r *autoscalingv1alpha1.PodAutoscaler) { t := metav1.NewTime(time.Unix(1e9, 0)) diff --git a/pkg/testing/v1alpha1/revision.go b/pkg/testing/v1alpha1/revision.go index b0580f221c7a..fb1fa334c8e4 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 calls .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) } } From 8b760543579c766b5260f38fc41f770e1a39229a Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Wed, 19 Jun 2019 11:54:29 -0700 Subject: [PATCH 03/14] Reorder import --- pkg/reconciler/autoscaling/kpa/kpa_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index e986af304ea2..2c874909667d 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -30,12 +30,12 @@ import ( fakekubeclient "github.com/knative/pkg/injection/clients/kubeclient/fake" fakeendpointsinformer "github.com/knative/pkg/injection/informers/kubeinformers/corev1/endpoints/fake" fakeserviceinformer "github.com/knative/pkg/injection/informers/kubeinformers/corev1/service/fake" + "github.com/knative/pkg/ptr" fakeservingclient "github.com/knative/serving/pkg/client/injection/client/fake" fakekpainformer "github.com/knative/serving/pkg/client/injection/informers/autoscaling/v1alpha1/podautoscaler/fake" fakesksinformer "github.com/knative/serving/pkg/client/injection/informers/networking/v1alpha1/serverlessservice/fake" fakerevisioninformer "github.com/knative/serving/pkg/client/injection/informers/serving/v1alpha1/revision/fake" "github.com/knative/serving/pkg/reconciler" - "github.com/knative/pkg/ptr" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/wait" From d1db88af32a6bd658b2ad2c3257eaac49daeb725 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Wed, 19 Jun 2019 23:35:49 -0700 Subject: [PATCH 04/14] Change PA's ContainersHealthy and PodsHealthy to Bootstrap. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 28 ++--- pkg/apis/autoscaling/v1alpha1/pa_types.go | 7 +- pkg/apis/serving/v1alpha1/revision_types.go | 2 +- pkg/reconciler/autoscaling/kpa/kpa.go | 14 +-- pkg/reconciler/autoscaling/kpa/kpa_test.go | 100 ++++++++---------- .../revision/reconcile_resources.go | 14 +-- pkg/reconciler/revision/revision_test.go | 3 +- pkg/reconciler/revision/table_test.go | 12 +-- pkg/testing/functional.go | 15 +-- 9 files changed, 76 insertions(+), 119 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index f8912968dcb9..d8552aee2509 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -29,8 +29,7 @@ import ( ) var podCondSet = apis.NewLivingConditionSet( - PodAutoscalerConditionContainersHealthy, - PodAutoscalerConditionPodsHealthy, + PodAutoscalerConditionBootstrap, ) func (pa *PodAutoscaler) GetGroupVersionKind() schema.GroupVersionKind { @@ -201,31 +200,26 @@ func (pas *PodAutoscalerStatus) MarkResourceFailedCreation(kind, name string) { fmt.Sprintf("Failed to create %s %q.", kind, name)) } -// MarkContainerExiting changes the "ContainersHealthy" condition to false and records the exit code. +// 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(PodAutoscalerConditionContainersHealthy, exitCodeString, message) + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, exitCodeString, message) } -// MarkContainerWaiting changes the "ContainersHealthy" condition to false and records the exit code. -func (pas *PodAutoscalerStatus) MarkContainerWaiting(reason, message string) { - podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionContainersHealthy, reason, message) +// MarkImagePullBackoff changes the "Bootstrap" condition to false. +func (pas *PodAutoscalerStatus) MarkImagePullBackoff(reason, message string) { + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, reason, message) } -// MarkContainersHealthy changes the "ContainersHealthy" condition to true. -func (pas *PodAutoscalerStatus) MarkContainersHealthy() { - podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionContainersHealthy) -} - -// MarkPodUnscheduled changes the "PodsHealthy" condition to false and records reason and message. +// MarkPodUnscheduled changes the "Bootstrap" condition to false and records reason and message. func (pas *PodAutoscalerStatus) MarkPodUnscheduled(reason, message string) { - podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionPodsHealthy, reason, message) + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, reason, message) } -// MarkPodsHealthy changes the "PodsHealthy" condition to true. -func (pas *PodAutoscalerStatus) MarkPodsHealthy() { - podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionPodsHealthy) +// 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 diff --git a/pkg/apis/autoscaling/v1alpha1/pa_types.go b/pkg/apis/autoscaling/v1alpha1/pa_types.go index b9d8adbf609f..ae8fd054104c 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_types.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_types.go @@ -101,10 +101,9 @@ const ( PodAutoscalerConditionReady = apis.ConditionReady // PodAutoscalerConditionActive becomes true when the PodAutoscaler's ScaleTargetRef can receive traffic. PodAutoscalerConditionActive apis.ConditionType = "Active" - // PodAutoscalerConditionContainersHealthy becomes true when the containers are up and running. - PodAutoscalerConditionContainersHealthy apis.ConditionType = "ContainersHealthy" - // PodAutoscalerConditionPodsHealthy becomes true when the pods are up and running. - PodAutoscalerConditionPodsHealthy apis.ConditionType = "PodsHealthy" + // 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/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 3e58602dc6c2..f6ab0b3e9dc4 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/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 1593443e5c13..3f3edcf21abd 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -188,7 +188,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler } } - err = c.updateHealthConditions(ctx, pa) + err = c.computeBootstrapConditions(ctx, pa) if err != nil { return perrors.Wrap(err, "error checking container health") } @@ -220,12 +220,12 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *pav1alpha1.PodAut return decider, nil } -func (c *Reconciler) updateHealthConditions(ctx context.Context, pa *pav1alpha1.PodAutoscaler) error { +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.MarkContainersHealthy() - pa.Status.MarkPodsHealthy() + pa.Status.MarkBootstrapSuccessful() return nil } ps, err := resourceutil.GetScaleResource(pa.Namespace, pa.Spec.ScaleTargetRef, c.PSInformerFactory) @@ -233,7 +233,7 @@ func (c *Reconciler) updateHealthConditions(ctx context.Context, pa *pav1alpha1. logger.Errorf("Error getting deployment: %v", err) return err } - // If a container keeps crashing (no active pods in the deployment although we want some) + // 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.Replicas == 0 { pods, err := c.KubeClientSet.CoreV1().Pods(pa.Namespace).List(metav1.ListOptions{LabelSelector: metav1.FormatLabelSelector(ps.Spec.Selector)}) if err != nil { @@ -257,9 +257,9 @@ func (c *Reconciler) updateHealthConditions(ctx context.Context, pa *pav1alpha1. 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 { + } else if w := status.State.Waiting; w != nil && w.Reason == "ImagePullBackoff" { logger.Infof("%s marking %s is waiting with: %s: %s", pa.Name, w.Reason, w.Message) - pa.Status.MarkContainerWaiting(w.Reason, w.Message) + pa.Status.MarkImagePullBackoff(w.Reason, w.Message) } break } diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 2c874909667d..e413a114445f 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -192,8 +192,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithHealthyContainers(), WithHealthyPods(), - 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,8 +209,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithHealthyContainers(), WithHealthyPods(), - markOld, WithPAStatusService(testRevision), + WithSuccessfulBootstrap(), markOld, WithPAStatusService(testRevision), withMSvcStatus("out-of-the-blue")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), @@ -232,8 +230,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, markOld, - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), @@ -244,8 +241,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("and-into-the-black")), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, @@ -256,8 +252,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, markOld, - WithHealthyContainers(), WithHealthyPods(), - 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")), @@ -275,8 +270,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus("you-ask-for-this")), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, @@ -287,8 +281,7 @@ func TestReconcileAndScaleToZero(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithHealthyContainers(), WithHealthyPods(), - 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")), @@ -364,7 +357,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a330-200"), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("a330-200")), @@ -376,7 +369,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a350-900ULR"), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("b777-200LR")), @@ -397,8 +390,7 @@ func TestReconcile(t *testing.T) { }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), }}, WantCreates: []runtime.Object{ metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -407,7 +399,7 @@ func TestReconcile(t *testing.T) { Name: "metric-service-exists-not-on-status", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("erj-e190")), @@ -416,15 +408,14 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, - WithHealthyContainers(), WithHealthyPods(), - 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), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), withMSvcName("a380-800")), @@ -454,15 +445,14 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, - WithHealthyContainers(), WithHealthyPods(), - WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), + WithSuccessfulBootstrap(), WithPAStatusService(testRevision), withMSvcStatus(testRevision+"-00001")), }}, WantCreates: []runtime.Object{ metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -476,7 +466,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -493,7 +483,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), deploy(testNamespace, testRevision), @@ -515,7 +505,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), deploy(testNamespace, testRevision), @@ -537,7 +527,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("a321neo"), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -556,7 +546,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, withMSvcStatus("b767-300er"), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), expectedDeploy, makeSKSPrivateEndpoints(1, testNamespace, testRevision), @@ -576,7 +566,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), withMSvcStatus("b737max-800"), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), withMSvcStatus("b737max-800"), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector), func(s *corev1.Service) { s.OwnerReferences = nil @@ -588,8 +578,7 @@ func TestReconcile(t *testing.T) { Object: kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), withMSvcStatus("b737max-800"), // We expect this change in status: - markResourceNotOwned("Service", "b737max-800"), - WithHealthyContainers(), WithHealthyPods()), + markResourceNotOwned("Service", "b737max-800"), WithSuccessfulBootstrap()), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "error reconciling metrics service: PA: test-revision does not own Service: b737max-800"), @@ -599,7 +588,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, markActive, - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -614,7 +603,7 @@ func TestReconcile(t *testing.T) { Key: key, Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods(), ), + 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)), @@ -627,43 +616,40 @@ func TestReconcile(t *testing.T) { WithDeployRef(deployName)), }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), - WithHealthyContainers(), WithHealthyPods()), + 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), WithHealthyContainers(), WithHealthyPods()), + 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), - WithHealthyContainers(), WithHealthyPods()), + Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "sks becomes ready", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithHealthyContainers(), WithHealthyPods()), + 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), - WithHealthyContainers(), WithHealthyPods()), + 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), WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, withMinScale(2), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -671,14 +657,14 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActivating, withMinScale(2), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "kpa becomes ready with minScale endpoints", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAStatusService(testRevision), - WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, markActivating, withMinScale(2), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -686,20 +672,20 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, withMinScale(2), - WithPAStatusService(testRevision), WithHealthyContainers(), WithHealthyPods()), + WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, }, { Name: "sks does not exist", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), + 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, WithHealthyContainers(), WithHealthyPods()), + Object: kpa(testNamespace, testRevision, markActivating, WithSuccessfulBootstrap()), }}, WantCreates: []runtime.Object{ sks(testNamespace, testRevision, WithDeployRef(deployName)), @@ -708,7 +694,7 @@ func TestReconcile(t *testing.T) { Name: "sks is out of whack", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef("bar"), WithPubService), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -716,8 +702,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), - WithHealthyContainers(), WithHealthyPods()), + Object: kpa(testNamespace, testRevision, markActivating, WithPAStatusService(testRevision), WithSuccessfulBootstrap()), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithPubService, @@ -727,7 +712,7 @@ func TestReconcile(t *testing.T) { Name: "sks cannot be created", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, }, @@ -746,7 +731,7 @@ func TestReconcile(t *testing.T) { Name: "sks cannot be updated", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef("bar")), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), expectedDeploy, @@ -765,7 +750,7 @@ func TestReconcile(t *testing.T) { Name: "sks is disowned", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithHealthyContainers(), WithHealthyPods()), + kpa(testNamespace, testRevision, markActive, WithSuccessfulBootstrap()), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithSKSOwnersRemoved), metricsSvc(testNamespace, testRevision, withSvcSelector(usualSelector)), @@ -773,8 +758,7 @@ func TestReconcile(t *testing.T) { }, WantErr: true, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markResourceNotOwned("ServerlessService", testRevision), - WithHealthyContainers(), WithHealthyPods()), + Object: kpa(testNamespace, testRevision, markResourceNotOwned("ServerlessService", testRevision), WithSuccessfulBootstrap()), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "error reconciling SKS: PA: test-revision does not own SKS: test-revision"), diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 814c115f4adf..205fc068a7ad 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -155,26 +155,16 @@ func (c *Reconciler) reconcileKPA(ctx context.Context, rev *v1alpha1.Revision) e rev.Status.MarkActive() } - cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionContainersHealthy) + 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) } } - cond = kpa.Status.GetCondition(kpav1alpha1.PodAutoscalerConditionPodsHealthy) - if cond != nil && cond.Status == corev1.ConditionFalse { - rev.Status.MarkResourcesUnavailable(cond.Reason, cond.Message) - } - - if kpa.Status.IsReady() { - // Precondition for PA being ready is SKS being active and - // that entices that |service.endpoints| > 0. - rev.Status.MarkResourcesAvailable() - } - return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 19c2540369ee..8601fc671475 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -103,8 +103,7 @@ func testReadyKPA(rev *v1alpha1.Revision) *av1alpha1.PodAutoscaler { kpa := resources.MakeKPA(rev) kpa.Status.InitializeConditions() kpa.Status.MarkActive() - kpa.Status.MarkPodsHealthy() - kpa.Status.MarkContainersHealthy() + 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 d720fa52d206..83c2bdc3eb93 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -281,8 +281,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"), - WithHealthyPods(), WithHealthyContainers()), + kpa("foo", "kpa-ready", WithTraffic, WithPAStatusService("new-stuff"), WithSuccessfulBootstrap()), deploy("foo", "kpa-ready"), image("foo", "kpa-ready"), }, @@ -370,7 +369,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"), WithHealthyPods(), WithHealthyContainers()), + WithTraffic, WithPAStatusService("fix-mutated-kpa"), WithSuccessfulBootstrap()), deploy("foo", "fix-mutated-kpa"), image("foo", "fix-mutated-kpa"), }, @@ -383,7 +382,7 @@ func TestReconcile(t *testing.T) { }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa("foo", "fix-mutated-kpa", WithTraffic, - WithPAStatusService("fix-mutated-kpa"), WithHealthyPods(), WithHealthyContainers()), + WithPAStatusService("fix-mutated-kpa"), WithSuccessfulBootstrap()), }}, Key: "foo/fix-mutated-kpa", }, { @@ -484,7 +483,7 @@ func TestReconcile(t *testing.T) { }, 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", }, { @@ -496,8 +495,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"), - WithHealthyPods(), WithHealthyContainers()), + 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 3b954efbcc61..b24a6689b752 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -84,10 +84,10 @@ func WithNoTraffic(reason, message string) PodAutoscalerOption { } } -// WithHealthyContainers updates the PA to have ContainersHealthy condition true. -func WithHealthyContainers() PodAutoscalerOption { +// WithSuccessfulBootstrap updates the PA to have ContainersHealthy condition true. +func WithSuccessfulBootstrap() PodAutoscalerOption { return func(pa *autoscalingv1alpha1.PodAutoscaler) { - pa.Status.MarkContainersHealthy() + pa.Status.MarkBootstrapSuccessful() } } @@ -101,14 +101,7 @@ func WithContainerExiting(exitCode int32, message string) PodAutoscalerOption { // WithImagePullBackoff updates the PA to have ContainersHealthy condition false. func WithImagePullBackoff(reason, message string) PodAutoscalerOption { return func(pa *autoscalingv1alpha1.PodAutoscaler) { - pa.Status.MarkContainerWaiting(reason, message) - } -} - -// WithHealthyPods updates the PA to have PodsHealthy condition true. -func WithHealthyPods() PodAutoscalerOption { - return func(pa *autoscalingv1alpha1.PodAutoscaler) { - pa.Status.MarkPodsHealthy() + pa.Status.MarkImagePullBackoff(reason, message) } } From 9a9affbd95a79e76e054e94d20247c6dbd240c70 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Thu, 20 Jun 2019 13:25:59 -0700 Subject: [PATCH 05/14] Fixed lifecycle and hpa unit tests --- .../autoscaling/v1alpha1/pa_lifecycle_test.go | 19 ++-- pkg/reconciler/autoscaling/hpa/hpa.go | 4 + pkg/reconciler/autoscaling/hpa/hpa_test.go | 86 +++++++++---------- 3 files changed, 60 insertions(+), 49 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 0833802424a4..285cc3c0aedf 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -757,36 +757,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/reconciler/autoscaling/hpa/hpa.go b/pkg/reconciler/autoscaling/hpa/hpa.go index 2f39db7850b4..efadab822cce 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa.go +++ b/pkg/reconciler/autoscaling/hpa/hpa.go @@ -110,6 +110,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 @@ -163,6 +166,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 414dea909f19..a53ba14216c1 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa_test.go +++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go @@ -99,8 +99,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 +108,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 +139,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 +154,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 +192,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 +201,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 +210,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 +229,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 +247,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, MarkResourceNotOwnedByPA("ServerlessService", testRevision), WithSuccessfulBootstrap()), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", `error reconciling SKS: PA: test-revision does not own SKS: test-revision`), @@ -263,17 +263,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, MarkResourceNotOwnedByPA("HorizontalPodAutoscaler", testRevision), WithSuccessfulBootstrap()), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", @@ -282,14 +282,14 @@ func TestReconcile(t *testing.T) { }, { Name: "do not create hpa when non-hpa-class pod autoscaler", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithKPAClass), + pa(testRevision, testNamespace, WithKPAClass, WithSuccessfulBootstrap()), }, Key: key(testRevision, testNamespace), }, { 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), @@ -297,14 +297,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, @@ -318,14 +318,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{ @@ -338,8 +338,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), }, @@ -356,19 +356,19 @@ func TestReconcile(t *testing.T) { }, { Name: "failure to create hpa", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass), + pa(testRevision, testNamespace, WithHPAClass, 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\".")), + "FailedCreate", "Failed to create HorizontalPodAutoscaler \"test-revision\"."), WithSuccessfulBootstrap()), }}, WantErr: true, WantEvents: []string{ From de7b6e0daf7e7bc5e14c423245cca824ec8fe005 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Fri, 21 Jun 2019 12:41:21 -0700 Subject: [PATCH 06/14] Add ReadyReplicas to PodScalable for checking container exiting. --- pkg/apis/autoscaling/v1alpha1/podscalable_types.go | 6 ++++-- pkg/reconciler/autoscaling/kpa/kpa.go | 4 ++-- pkg/reconciler/autoscaling/resources/names/names.go | 5 ----- pkg/resources/scale.go | 1 - 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/podscalable_types.go b/pkg/apis/autoscaling/v1alpha1/podscalable_types.go index 4455986adad3..201dd66ee9b5 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/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 3f3edcf21abd..df0dd407da33 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -190,7 +190,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler err = c.computeBootstrapConditions(ctx, pa) if err != nil { - return perrors.Wrap(err, "error checking container health") + return perrors.Wrap(err, "error checking bootstrap conditions") } return nil @@ -234,7 +234,7 @@ func (c *Reconciler) computeBootstrapConditions(ctx context.Context, pa *pav1alp 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.Replicas == 0 { + 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) diff --git a/pkg/reconciler/autoscaling/resources/names/names.go b/pkg/reconciler/autoscaling/resources/names/names.go index ca1ea9846483..3ad5b4b88eed 100644 --- a/pkg/reconciler/autoscaling/resources/names/names.go +++ b/pkg/reconciler/autoscaling/resources/names/names.go @@ -21,8 +21,3 @@ package names func SKS(paName string) string { return paName } - -// Deployment returns the name of the Deployment resource scaled by this PA. -func Deployment(paName string) string { - return paName + "-deployment" -} diff --git a/pkg/resources/scale.go b/pkg/resources/scale.go index 355931d09f7e..8cba15e0c7ba 100644 --- a/pkg/resources/scale.go +++ b/pkg/resources/scale.go @@ -18,7 +18,6 @@ package resources import ( "context" - "github.com/knative/pkg/apis" "github.com/knative/pkg/apis/duck" "github.com/knative/pkg/controller" From 1cf845bf412c0f66f8bc515b6368dd3f15ce6af6 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Fri, 21 Jun 2019 14:15:10 -0700 Subject: [PATCH 07/14] Correct image-pull error reasons. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 4 ++-- pkg/reconciler/autoscaling/kpa/kpa.go | 4 ++-- pkg/reconciler/revision/table_test.go | 2 +- pkg/testing/functional.go | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index d8552aee2509..5db8b01ee46d 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -207,8 +207,8 @@ func (pas *PodAutoscalerStatus) MarkContainerExiting(exitCode int32, message str podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, exitCodeString, message) } -// MarkImagePullBackoff changes the "Bootstrap" condition to false. -func (pas *PodAutoscalerStatus) MarkImagePullBackoff(reason, message string) { +// MarkImagePullError changes the "Bootstrap" condition to false. +func (pas *PodAutoscalerStatus) MarkImagePullError(reason, message string) { podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, reason, message) } diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index df0dd407da33..518d864a2e3e 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -257,9 +257,9 @@ func (c *Reconciler) computeBootstrapConditions(ctx context.Context, pa *pav1alp 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" { + } 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.MarkImagePullBackoff(w.Reason, w.Message) + pa.Status.MarkImagePullError(w.Reason, w.Message) } break } diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 83c2bdc3eb93..f4ec16c17e3b 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -440,7 +440,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "pull-backoff", withK8sServiceName("the-taxman"), WithLogURL, MarkActivating("Deploying", "")), - kpa("foo", "pull-backoff", WithImagePullBackoff("ImagePullBackoff", "can't pull it")), + kpa("foo", "pull-backoff", WithImagePullError("ImagePullBackoff", "can't pull it")), timeoutDeploy(deploy("foo", "pull-backoff")), image("foo", "pull-backoff"), }, diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index b24a6689b752..6bfa316ed888 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -98,10 +98,10 @@ func WithContainerExiting(exitCode int32, message string) PodAutoscalerOption { } } -// WithImagePullBackoff updates the PA to have ContainersHealthy condition false. -func WithImagePullBackoff(reason, message string) PodAutoscalerOption { +// WithImagePullError updates the PA to have ContainersHealthy condition false. +func WithImagePullError(reason, message string) PodAutoscalerOption { return func(pa *autoscalingv1alpha1.PodAutoscaler) { - pa.Status.MarkImagePullBackoff(reason, message) + pa.Status.MarkImagePullError(reason, message) } } From 2b18a346e70698609a98249a57324ffbaa69dc53 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Mon, 24 Jun 2019 03:01:08 -0700 Subject: [PATCH 08/14] Add PA unit tests. --- .../autoscaling/v1alpha1/pa_lifecycle_test.go | 6 + pkg/reconciler/autoscaling/kpa/kpa.go | 3 + pkg/reconciler/autoscaling/kpa/kpa_test.go | 133 +++++++++++++++++- pkg/reconciler/autoscaling/kpa/scaler_test.go | 50 ++++++- pkg/testing/functional.go | 5 + 5 files changed, 189 insertions(+), 8 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 285cc3c0aedf..9a3a098a85cf 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -337,6 +337,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 +354,9 @@ func TestIsReady(t *testing.T) { Conditions: duckv1beta1.Conditions{{ Type: PodAutoscalerConditionActive, Status: corev1.ConditionTrue, + }, { + Type: PodAutoscalerConditionBootstrap, + Status: corev1.ConditionTrue, }, { Type: PodAutoscalerConditionReady, Status: corev1.ConditionFalse, diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 518d864a2e3e..ec219e7beab8 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -135,6 +135,9 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler return perrors.Wrap(err, "error reconciling metrics service") } + // TODO: This looks like a bug reconciling SKS before computing Active condition while + // SKS mode is based on Active condition? PA can be Activating here, then go to Inactive below, + // and SKS has an update from Proxy to Serve here, whereas the mode should be Proxy for Inactive. sks, err := c.ReconcileSKS(ctx, pa) if err != nil { return perrors.Wrap(err, "error reconciling SKS") diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index e413a114445f..60272c68cf2f 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -19,6 +19,7 @@ package kpa import ( "context" "fmt" + "github.com/knative/pkg/ptr" "net/http" "strconv" "sync" @@ -30,7 +31,6 @@ import ( fakekubeclient "github.com/knative/pkg/injection/clients/kubeclient/fake" fakeendpointsinformer "github.com/knative/pkg/injection/informers/kubeinformers/corev1/endpoints/fake" fakeserviceinformer "github.com/knative/pkg/injection/informers/kubeinformers/corev1/service/fake" - "github.com/knative/pkg/ptr" fakeservingclient "github.com/knative/serving/pkg/client/injection/client/fake" fakekpainformer "github.com/knative/serving/pkg/client/injection/informers/autoscaling/v1alpha1/podautoscaler/fake" fakesksinformer "github.com/knative/serving/pkg/client/injection/informers/networking/v1alpha1/serverlessservice/fake" @@ -225,6 +225,137 @@ 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, diff --git a/pkg/reconciler/autoscaling/kpa/scaler_test.go b/pkg/reconciler/autoscaling/kpa/scaler_test.go index 884e7244d49e..5880a8c41b86 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler_test.go +++ b/pkg/reconciler/autoscaling/kpa/scaler_test.go @@ -108,24 +108,24 @@ func TestScaler(t *testing.T) { kpaMarkActive(k, time.Now().Add(-stableWindow)) }, }, { - label: "scale to zero after grace period", + label: "waits to scale to zero (just before grace period)", startReplicas: 1, scaleTo: 0, wantReplicas: 0, - wantScaling: true, + wantScaling: false, kpaMutation: func(k *pav1alpha1.PodAutoscaler) { - kpaMarkInactive(k, time.Now().Add(-gracePeriod)) + kpaMarkInactive(k, time.Now().Add(-gracePeriod).Add(1*time.Second)) }, + wantCBCount: 1, }, { - label: "waits to scale to zero (just before grace period)", + label: "scale to zero after grace period", startReplicas: 1, scaleTo: 0, wantReplicas: 0, - wantScaling: false, + wantScaling: true, kpaMutation: func(k *pav1alpha1.PodAutoscaler) { - kpaMarkInactive(k, time.Now().Add(-gracePeriod).Add(1*time.Second)) + kpaMarkInactive(k, time.Now().Add(-gracePeriod)) }, - wantCBCount: 1, }, { label: "scale to zero after grace period, but fail prober", startReplicas: 1, @@ -150,6 +150,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/testing/functional.go b/pkg/testing/functional.go index 6bfa316ed888..9aed3689dd96 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -370,6 +370,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{ From 9521ec33fceb091886c2affab0e31ce34aeb18e9 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Mon, 24 Jun 2019 12:23:24 -0700 Subject: [PATCH 09/14] Add more pa_lifecycle tests. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 2 + .../autoscaling/v1alpha1/pa_lifecycle_test.go | 217 +++++++++++++++++- 2 files changed, 217 insertions(+), 2 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index b7af5f37f80d..27b1a80459cd 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -191,6 +191,7 @@ 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) { + // TODO: This looks more like a bootstrap condition? pas.MarkInactive("NotOwned", fmt.Sprintf("There is an existing %s %q that we do not own.", kind, name)) } @@ -198,6 +199,7 @@ func (pas *PodAutoscalerStatus) MarkResourceNotOwned(kind, name string) { // 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) { + // TODO: This looks more like a bootstrap condition? pas.MarkInactive("FailedCreate", fmt.Sprintf("Failed to create %s %q.", kind, name)) } diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 09df4a13e5ea..c3cd669c5f45 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, }}, }, @@ -373,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 @@ -524,6 +681,62 @@ func TestMarkResourceFailedCreation(t *testing.T) { } } +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 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) + } +} + func TestClass(t *testing.T) { cases := []struct { name string From bd8f1ff9308a9e88c431e0c643ba883b91fa054c Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Mon, 24 Jun 2019 13:00:15 -0700 Subject: [PATCH 10/14] Add revision_lifecycle_tests --- .../serving/v1alpha1/revision_lifecycle_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go index 3288ba73a252..bcca466e8dbe 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 TestTypicalFlowWithServiceTimeout(t *testing.T) { r := &RevisionStatus{} r.InitializeConditions() From 294462034e954bc07873402a528ef8ed83651861 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Mon, 24 Jun 2019 13:33:55 -0700 Subject: [PATCH 11/14] Minor no-op edits --- pkg/resources/scale.go | 1 + pkg/testing/v1alpha1/revision.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/resources/scale.go b/pkg/resources/scale.go index 8cba15e0c7ba..355931d09f7e 100644 --- a/pkg/resources/scale.go +++ b/pkg/resources/scale.go @@ -18,6 +18,7 @@ package resources import ( "context" + "github.com/knative/pkg/apis" "github.com/knative/pkg/apis/duck" "github.com/knative/pkg/controller" diff --git a/pkg/testing/v1alpha1/revision.go b/pkg/testing/v1alpha1/revision.go index fb1fa334c8e4..4a71acb9e51c 100644 --- a/pkg/testing/v1alpha1/revision.go +++ b/pkg/testing/v1alpha1/revision.go @@ -132,7 +132,7 @@ func MarkContainerMissing(rev *v1alpha1.Revision) { rev.Status.MarkContainerMissing("It's the end of the world as we know it") } -// MarkContainerUnhealthy calls .Status.MarkContainerUnhealthy on the Revision. +// MarkContainerUnhealthy is an adaptor to call .Status.MarkContainerUnhealthy on the Revision. func MarkContainerUnhealthy(reason, message string) RevisionOption { return func(r *v1alpha1.Revision) { r.Status.MarkContainerUnhealthy(reason, message) From 52c35a215cfe79c5f3074a86b66a89b6a58091f3 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Thu, 27 Jun 2019 12:11:25 -0700 Subject: [PATCH 12/14] Make resource-not-owned and resource-failed-creation readiness conditions rather active conditions. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 6 ++--- .../autoscaling/v1alpha1/pa_lifecycle_test.go | 12 +++++----- pkg/reconciler/autoscaling/hpa/hpa_test.go | 14 +++++++----- pkg/reconciler/autoscaling/kpa/kpa.go | 3 --- pkg/reconciler/autoscaling/kpa/kpa_test.go | 4 ++-- pkg/testing/functional.go | 22 +++++++++++++++---- 6 files changed, 37 insertions(+), 24 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index 0577c0969531..48be08e7badc 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -191,16 +191,14 @@ 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) { - // TODO: This looks more like a bootstrap condition? - 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) { - // TODO: This looks more like a bootstrap condition? - pas.MarkInactive("FailedCreate", + podCondSet.Manage(pas).MarkFalse(PodAutoscalerConditionBootstrap, "FailedCreate", fmt.Sprintf("Failed to create %s %q.", kind, name)) } diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index eef03b247c40..6ec3509f8cf6 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -670,14 +670,14 @@ func TestMarkResourceNotOwned(t *testing.T) { func TestMarkResourceFailedCreation(t *testing.T) { pa := &PodAutoscalerStatus{} pa.MarkResourceFailedCreation("doesn't", "matter") - apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionActive, t) + apitest.CheckConditionFailed(pa.duck(), PodAutoscalerConditionBootstrap, t) - active := pa.GetCondition("Active") - if active.Status != corev1.ConditionFalse { - t.Errorf("TestMarkResourceFailedCreation expected active.Status: False got: %v", active.Status) + bootstrap := pa.GetCondition("Bootstrap") + if bootstrap.Status != corev1.ConditionFalse { + t.Errorf("TestMarkResourceFailedCreation expected active.Status: False got: %v", bootstrap.Status) } - if active.Reason != "FailedCreate" { - t.Errorf("TestMarkResourceFailedCreation expected active.Reason: FailedCreate got: %v", active.Reason) + if bootstrap.Reason != "FailedCreate" { + t.Errorf("TestMarkResourceFailedCreation expected active.Reason: FailedCreate got: %v", bootstrap.Reason) } } diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go index 905702854768..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"} @@ -255,7 +259,7 @@ func TestReconcile(t *testing.T) { Key: key(testRevision, testNamespace), WantErr: true, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, MarkResourceNotOwnedByPA("ServerlessService", testRevision), WithSuccessfulBootstrap()), + 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`), @@ -273,7 +277,7 @@ func TestReconcile(t *testing.T) { Key: key(testRevision, testNamespace), WantErr: true, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, MarkResourceNotOwnedByPA("HorizontalPodAutoscaler", testRevision), WithSuccessfulBootstrap()), + Object: pa(testRevision, testNamespace, WithHPAClass, markActive, MarkResourceNotOwnedByPA("HorizontalPodAutoscaler", testRevision)), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", @@ -350,7 +354,7 @@ func TestReconcile(t *testing.T) { }, { Name: "failure to create hpa", Objects: []runtime.Object{ - pa(testRevision, testNamespace, WithHPAClass, WithSuccessfulBootstrap()), + pa(testRevision, testNamespace, WithHPAClass, markActive, WithSuccessfulBootstrap()), deploy(testNamespace, testRevision), }, Key: key(testRevision, testNamespace), @@ -361,8 +365,8 @@ func TestReconcile(t *testing.T) { InduceFailure("create", "horizontalpodautoscalers"), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testRevision, testNamespace, WithHPAClass, WithNoTraffic( - "FailedCreate", "Failed to create HorizontalPodAutoscaler \"test-revision\"."), WithSuccessfulBootstrap()), + 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 ab9944dc749d..c00cf9cb6f73 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -130,9 +130,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pa *pav1alpha1.PodAutoscaler return perrors.Wrap(err, "error reconciling metrics service") } - // TODO: This looks like a bug reconciling SKS before computing Active condition while - // SKS mode is based on Active condition? PA can be Activating here, then go to Inactive below, - // and SKS has an update from Proxy to Serve here, whereas the mode should be Proxy for Inactive. sks, err := c.ReconcileSKS(ctx, pa) if err != nil { return perrors.Wrap(err, "error reconciling SKS") diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 87436443d782..a74c88acb045 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -710,7 +710,7 @@ func TestReconcile(t *testing.T) { Object: kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), withMSvcStatus("b737max-800"), // We expect this change in status: - markResourceNotOwned("Service", "b737max-800"), WithSuccessfulBootstrap()), + markResourceNotOwned("Service", "b737max-800")), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "error reconciling metrics service: PA: test-revision does not own Service: b737max-800"), @@ -890,7 +890,7 @@ func TestReconcile(t *testing.T) { }, WantErr: true, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markResourceNotOwned("ServerlessService", testRevision), WithSuccessfulBootstrap()), + 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/testing/functional.go b/pkg/testing/functional.go index 9aed3689dd96..b2111e7903c1 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -84,34 +84,48 @@ func WithNoTraffic(reason, message string) PodAutoscalerOption { } } -// WithSuccessfulBootstrap updates the PA to have ContainersHealthy condition true. +// 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 ContainersHealthy condition false with the exit code and message. +// 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 ContainersHealthy condition false. +// 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 PodsHealthy condition false. +// 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)) From 05844eb0caaa09a25445f109bba8ce623c16eda6 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Thu, 27 Jun 2019 13:31:31 -0700 Subject: [PATCH 13/14] Fix a test. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 6ec3509f8cf6..690b6582cd88 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -658,12 +658,12 @@ 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) } } From f3ac59e25e131d9fff55fb4672c05297301e5cf7 Mon Sep 17 00:00:00 2001 From: Chi Ho Date: Fri, 28 Jun 2019 09:59:28 -0700 Subject: [PATCH 14/14] Correct a typo in an error message. --- pkg/reconciler/autoscaling/kpa/kpa.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index c00cf9cb6f73..af1d6ae5d7a3 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -225,7 +225,7 @@ func (c *Reconciler) computeBootstrapConditions(ctx context.Context, pa *pav1alp } ps, err := resourceutil.GetScaleResource(pa.Namespace, pa.Spec.ScaleTargetRef, c.PSInformerFactory) if err != nil { - logger.Errorf("Error getting deployment: %v", err) + 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.