From a302d2f025f56440cadb313cee8944ba6e90da7b Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Fri, 4 Oct 2024 17:55:38 +0300 Subject: [PATCH] clean up --- pkg/reconciler/autoscaling/kpa/kpa.go | 2 +- pkg/reconciler/autoscaling/kpa/kpa_test.go | 47 +++++++++---------- pkg/reconciler/autoscaling/kpa/scaler.go | 8 ++-- pkg/reconciler/autoscaling/kpa/scaler_test.go | 4 +- 4 files changed, 29 insertions(+), 32 deletions(-) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 69e40793eb4b..6edb890b8011 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -122,7 +122,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *autoscalingv1alpha1. // Get the appropriate current scale from the metric, and right size // the scaleTargetRef based on it. - want, err := c.scaler.scale(ctx, pa, sks, decider.Status.DesiredScale, c.Client, &podCounter) + want, err := c.scaler.scale(ctx, pa, sks, decider.Status.DesiredScale, &podCounter) if err != nil { return fmt.Errorf("error scaling target: %w", err) } diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 8aafe5edec38..01ecec110ec8 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -40,7 +40,6 @@ import ( fakepainformer "knative.dev/serving/pkg/client/injection/informers/autoscaling/v1alpha1/podautoscaler/fake" fakerevisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision/fake" "knative.dev/serving/pkg/metrics" - pkgtesting "knative.dev/serving/pkg/testing" networkingclient "knative.dev/networking/pkg/client/injection/client" filteredinformerfactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered" @@ -295,7 +294,7 @@ func TestReconcile(t *testing.T) { } activeKPAMinScale := func(g, w int32) *autoscalingv1alpha1.PodAutoscaler { return kpa( - testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(g, w), WithReachabilityReachable, withMinScale(defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), @@ -333,7 +332,7 @@ func TestReconcile(t *testing.T) { Name: "steady state", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, WithScaleTargetNoScaleFailures, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, @@ -359,12 +358,12 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithTraffic, - markScaleTargetInitialized, WithPASKSReady, pkgtesting.WithScaleTargetNoScaleFailures, + markScaleTargetInitialized, WithPASKSReady, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), }, { Object: kpa(testNamespace, testRevision, WithTraffic, - markScaleTargetInitialized, WithPASKSReady, pkgtesting.WithScaleTargetNoScaleFailures, + markScaleTargetInitialized, WithPASKSReady, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), }}, @@ -411,11 +410,11 @@ func TestReconcile(t *testing.T) { Name: "create metric", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(1, defaultScale), WithPAStatusService(testRevision)), defaultSKS, defaultDeployment, defaultReady}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + Object: kpa(testNamespace, testRevision, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(1, defaultScale), WithPASKSReady, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), }}, @@ -426,7 +425,7 @@ func TestReconcile(t *testing.T) { Name: "scale up deployment", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, @@ -496,7 +495,7 @@ func TestReconcile(t *testing.T) { preciseReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, withScales(defaultScale, defaultScale), - WithPASKSNotReady(""), WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + WithPASKSNotReady(""), WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), WithPAStatusService(testRevision), WithObservedGeneration(1)), }}, }, { @@ -556,7 +555,7 @@ func TestReconcile(t *testing.T) { defaultDeployment, defaultReady}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithPASKSReady, - WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, withMinScale(2), WithPAMetricsService(privateSvc), + WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withMinScale(2), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnreachable, WithObservedGeneration(1)), }}, @@ -572,7 +571,7 @@ func TestReconcile(t *testing.T) { }, makeReadyPods(2, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithPASKSReady, - WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, withMinScale(2), WithPAMetricsService(privateSvc), + WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityReachable, WithObservedGeneration(1)), }}, @@ -588,7 +587,7 @@ func TestReconcile(t *testing.T) { }, makeReadyPods(2, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithPASKSReady, - WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, withMinScale(2), WithPAMetricsService(privateSvc), + WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnknown, WithObservedGeneration(1)), }}, @@ -750,7 +749,7 @@ func TestReconcile(t *testing.T) { metric(testNamespace, testRevision), deploy(testNamespace, testRevision), defaultReady}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, withScales(1, 0), + Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(1, 0), WithPASKSReady, WithPAMetricsService(privateSvc), WithNoTraffic(noTrafficReason, "The target is not receiving traffic."), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), @@ -766,7 +765,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */)), Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, - markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, withScales(1, 1), + markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(1, 1), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), @@ -784,7 +783,7 @@ func TestReconcile(t *testing.T) { metric(testNamespace, testRevision), deploy(testNamespace, testRevision), defaultReady}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, WithPASKSReady, WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPASKSReady, WithPAMetricsService(privateSvc), WithNoTraffic("TimedOut", "The target could not be activated."), withScales(1, 0), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), @@ -860,7 +859,7 @@ func TestReconcile(t *testing.T) { minScalePatch, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: activatingKPAMinScale(underscale, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, WithPASKSReady), + Object: activatingKPAMinScale(underscale, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPASKSReady), }}, }, { // Scale to `minScale` and mark PA "active" @@ -955,7 +954,7 @@ func TestReconcile(t *testing.T) { -42 /* ebc */)), Objects: []runtime.Object{ kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, - markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), + markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultProxySKS, metric(testNamespace, testRevision), @@ -967,7 +966,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -18 /* ebc */)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, @@ -985,7 +984,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ 1 /* ebc */)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultProxySKS, @@ -1039,7 +1038,7 @@ func TestReconcile(t *testing.T) { }, makeReadyPods(20, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, - markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, withScales(20, 20), withInitialScale(20), WithReachabilityReachable, + markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(20, 20), withInitialScale(20), WithReachabilityReachable, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), ), }}, @@ -1065,7 +1064,7 @@ func TestReconcile(t *testing.T) { }), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithNoTraffic(noTrafficReason, "The target is not receiving traffic."), withScales(0, -1), WithReachabilityReachable, WithPAMetricsService(privateSvc), WithObservedGeneration(1), @@ -1135,7 +1134,7 @@ func TestReconcile(t *testing.T) { }), }, makeReadyPods(2, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, WithTraffic, WithPASKSReady, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + Object: kpa(testNamespace, testRevision, WithTraffic, WithPASKSReady, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, withScales(2, 2), WithReachabilityReachable, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), ), @@ -1147,7 +1146,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ 1 /* ebc */)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultProxySKS, @@ -1162,7 +1161,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ 1 /* ebc */)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, pkgtesting.WithScaleTargetNoScaleFailures, + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, WithScaleTargetNoScaleFailures, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go index aa687cc56f3b..59f36b48fc17 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -38,7 +38,6 @@ import ( "knative.dev/serving/pkg/activator" autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1" "knative.dev/serving/pkg/autoscaler/config/autoscalerconfig" - clientset "knative.dev/serving/pkg/client/clientset/versioned" "knative.dev/serving/pkg/reconciler/autoscaling/config" kparesources "knative.dev/serving/pkg/reconciler/autoscaling/kpa/resources" aresources "knative.dev/serving/pkg/reconciler/autoscaling/resources" @@ -332,7 +331,7 @@ func (ks *scaler) applyScale(ctx context.Context, pa *autoscalingv1alpha1.PodAut } // scale attempts to scale the given PA's target reference to the desired scale. -func (ks *scaler) scale(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler, sks *netv1alpha1.ServerlessService, desiredScale int32, client clientset.Interface, podCounter *resources.PodAccessor) (int32, error) { +func (ks *scaler) scale(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler, sks *netv1alpha1.ServerlessService, desiredScale int32, podCounter *resources.PodAccessor) (int32, error) { asConfig := config.FromContext(ctx).Autoscaler logger := logging.FromContext(ctx) @@ -410,10 +409,9 @@ func checkForPodErrorsBeforeScalingDown(logger *zap.SugaredLogger, pod *corev1.P pa.Status.MarkWithScaleFailures(v1.ExitCodeReason(t.ExitCode), v1.RevisionContainerExitingMessage("container exited with no error")) break - } else { - pa.Status.MarkWithScaleFailures(v1.ExitCodeReason(t.ExitCode), v1.RevisionContainerExitingMessage(t.Message)) - break } + pa.Status.MarkWithScaleFailures(v1.ExitCodeReason(t.ExitCode), v1.RevisionContainerExitingMessage(t.Message)) + break } else if w := status.State.Waiting; w != nil { logger.Debugf("marking pa as failed: %s: %s", w.Reason, w.Message) pa.Status.MarkWithScaleFailures(w.Reason, w.Message) diff --git a/pkg/reconciler/autoscaling/kpa/scaler_test.go b/pkg/reconciler/autoscaling/kpa/scaler_test.go index d9fd9f76593a..b4956e818d6b 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler_test.go +++ b/pkg/reconciler/autoscaling/kpa/scaler_test.go @@ -556,7 +556,7 @@ func TestScaler(t *testing.T) { test.configMutator(cfg) } ctx = config.ToContext(ctx, cfg) - desiredScale, err := revisionScaler.scale(ctx, pa, sks, test.scaleTo, fakeservingclient.Get(ctx), nil) + desiredScale, err := revisionScaler.scale(ctx, pa, sks, test.scaleTo, nil) if err != nil { t.Error("Scale got an unexpected error:", err) } @@ -647,7 +647,7 @@ func TestDisableScaleToZero(t *testing.T) { conf := defaultConfig() conf.Autoscaler.EnableScaleToZero = false ctx = config.ToContext(ctx, conf) - desiredScale, err := revisionScaler.scale(ctx, pa, nil /*sks doesn't matter in this test*/, test.scaleTo, fakeservingclient.Get(ctx), nil) + desiredScale, err := revisionScaler.scale(ctx, pa, nil /*sks doesn't matter in this test*/, test.scaleTo, nil) if err != nil { t.Error("Scale got an unexpected error:", err)