Skip to content

Commit fe41713

Browse files
zroubalikzroubalik
authored
chore: get the correct HTTPScaledObject reference in the scaler (#1296)
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
1 parent 17b2af0 commit fe41713

File tree

2 files changed

+83
-61
lines changed

2 files changed

+83
-61
lines changed

‎scaler/handlers.go‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,15 +122,22 @@ func (e *impl) GetMetricSpec(_ context.Context, sor *externalscaler.ScaledObject
122122
namespacedName := k8s.NamespacedNameFromScaledObjectRef(sor)
123123
metricName := MetricName(namespacedName)
124124

125-
httpso, err := e.httpsoInformer.Lister().HTTPScaledObjects(sor.Namespace).Get(sor.Name)
126-
if err != nil {
127-
if scalerMetadata := sor.GetScalerMetadata(); scalerMetadata != nil {
125+
scalerMetadata := sor.GetScalerMetadata()
126+
httpScaledObjectName, ok := scalerMetadata[k8s.HTTPScaledObjectKey]
127+
if !ok {
128+
if scalerMetadata != nil {
128129
if interceptorTargetPendingRequests, ok := scalerMetadata[keyInterceptorTargetPendingRequests]; ok {
129130
return e.interceptorMetricSpec(metricName, interceptorTargetPendingRequests)
130131
}
131132
}
133+
err := fmt.Errorf("unable to get HTTPScaledObject reference")
134+
lggr.Error(err, "unable to get the linked HTTPScaledObject for ScaledObject", "name", sor.Name, "namespace", sor.Namespace, "httpScaledObjectName", httpScaledObjectName)
135+
return nil, err
136+
}
132137

133-
lggr.Error(err, "unable to get HTTPScaledObject", "name", sor.Name, "namespace", sor.Namespace)
138+
httpso, err := e.httpsoInformer.Lister().HTTPScaledObjects(sor.Namespace).Get(httpScaledObjectName)
139+
if err != nil {
140+
lggr.Error(err, "unable to get HTTPScaledObject", "name", sor.Name, "namespace", sor.Namespace, "httpScaledObjectName", httpScaledObjectName)
134141
return nil, err
135142
}
136143
targetValue := int64(ptr.Deref(httpso.Spec.TargetPendingRequests, 100))
@@ -185,19 +192,19 @@ func (e *impl) GetMetrics(_ context.Context, metricRequest *externalscaler.GetMe
185192
scalerMetadata := sor.GetScalerMetadata()
186193
httpScaledObjectName, ok := scalerMetadata[k8s.HTTPScaledObjectKey]
187194
if !ok {
188-
if scalerMetadata := sor.GetScalerMetadata(); scalerMetadata != nil {
195+
if scalerMetadata != nil {
189196
if _, ok := scalerMetadata[keyInterceptorTargetPendingRequests]; ok {
190197
return e.interceptorMetrics(metricName)
191198
}
192199
}
193200
err := fmt.Errorf("unable to get HTTPScaledObject reference")
194-
lggr.Error(err, "unable to get the linked HTTPScaledObject for ScaledObject", "name", sor.Name, "namespace", sor.Namespace)
201+
lggr.Error(err, "unable to get the linked HTTPScaledObject for ScaledObject", "name", sor.Name, "namespace", sor.Namespace, "httpScaledObjectName", httpScaledObjectName)
195202
return nil, err
196203
}
197204

198205
httpso, err := e.httpsoInformer.Lister().HTTPScaledObjects(sor.Namespace).Get(httpScaledObjectName)
199206
if err != nil {
200-
lggr.Error(err, "unable to get HTTPScaledObject", "name", httpScaledObjectName, "namespace", sor.Namespace)
207+
lggr.Error(err, "unable to get HTTPScaledObject", "name", httpScaledObjectName, "namespace", sor.Namespace, "httpScaledObjectName", httpScaledObjectName)
201208
return nil, err
202209
}
203210

‎scaler/handlers_test.go‎

Lines changed: 69 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -470,25 +470,31 @@ func TestGetMetricSpecTable(t *testing.T) {
470470
name: "valid host as single host value in scaler metadata",
471471
defaultTargetMetric: 0,
472472
newInformer: func(t *testing.T, ctrl *gomock.Controller) *informersexternalversionshttpv1alpha1mock.MockHTTPScaledObjectInformer {
473-
informer, _, namespaceLister := newMocks(ctrl, nil)
473+
namespaceLister := listershttpv1alpha1mock.NewMockHTTPScaledObjectNamespaceLister(ctrl)
474+
lister := listershttpv1alpha1mock.NewMockHTTPScaledObjectLister(ctrl)
475+
informer := informersexternalversionshttpv1alpha1mock.NewMockHTTPScaledObjectInformer(ctrl)
476+
477+
informer.EXPECT().
478+
Lister().
479+
Return(lister).
480+
AnyTimes()
481+
lister.EXPECT().
482+
HTTPScaledObjects(ns).
483+
Return(namespaceLister).
484+
AnyTimes()
474485

475486
httpso := &httpv1alpha1.HTTPScaledObject{
476-
ObjectMeta: metav1.ObjectMeta{
477-
Namespace: ns,
478-
Name: t.Name(),
479-
},
487+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: validHTTPScaledObjectName},
480488
Spec: httpv1alpha1.HTTPScaledObjectSpec{
481-
ScaleTargetRef: httpv1alpha1.ScaleTargetRef{
482-
Name: "testdepl",
483-
Service: "testsrv",
484-
Port: 8080,
485-
},
489+
ScaleTargetRef: httpv1alpha1.ScaleTargetRef{Name: "testdepl", Service: "testsrv", Port: 8080},
486490
TargetPendingRequests: ptr.To[int32](123),
487491
},
488492
}
489-
namespaceLister.EXPECT().
490-
Get(httpso.GetName()).
491-
Return(httpso, nil)
493+
namespaceLister.
494+
EXPECT().
495+
Get(validHTTPScaledObjectName).
496+
Return(httpso, nil).
497+
Times(1)
492498

493499
return informer
494500
},
@@ -497,28 +503,36 @@ func TestGetMetricSpecTable(t *testing.T) {
497503
r := require.New(t)
498504
r.NoError(err)
499505
r.NotNil(res)
500-
r.Equal(1, len(res.MetricSpecs))
506+
r.Len(res.MetricSpecs, 1)
501507
spec := res.MetricSpecs[0]
502508
r.Equal(MetricName(&types.NamespacedName{Namespace: ns, Name: t.Name()}), spec.MetricName)
503509
r.Equal(int64(123), spec.TargetSize)
504510
},
511+
scalerMetadata: map[string]string{
512+
k8s.HTTPScaledObjectKey: validHTTPScaledObjectName,
513+
},
505514
},
506515
{
507516
name: "valid hosts as multiple hosts value in scaler metadata",
508517
defaultTargetMetric: 0,
509518
newInformer: func(t *testing.T, ctrl *gomock.Controller) *informersexternalversionshttpv1alpha1mock.MockHTTPScaledObjectInformer {
510-
informer, _, namespaceLister := newMocks(ctrl, nil)
519+
namespaceLister := listershttpv1alpha1mock.NewMockHTTPScaledObjectNamespaceLister(ctrl)
520+
lister := listershttpv1alpha1mock.NewMockHTTPScaledObjectLister(ctrl)
521+
informer := informersexternalversionshttpv1alpha1mock.NewMockHTTPScaledObjectInformer(ctrl)
522+
523+
informer.EXPECT().
524+
Lister().
525+
Return(lister).
526+
AnyTimes()
527+
lister.EXPECT().
528+
HTTPScaledObjects(ns).
529+
Return(namespaceLister).
530+
AnyTimes()
511531

512532
httpso := &httpv1alpha1.HTTPScaledObject{
513-
ObjectMeta: metav1.ObjectMeta{
514-
Namespace: ns,
515-
Name: t.Name(),
516-
},
533+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: validHTTPScaledObjectName},
517534
Spec: httpv1alpha1.HTTPScaledObjectSpec{
518-
Hosts: []string{
519-
"validHost1",
520-
"validHost2",
521-
},
535+
Hosts: []string{"validHost1", "validHost2"},
522536
ScaleTargetRef: httpv1alpha1.ScaleTargetRef{
523537
Name: "testdepl",
524538
Service: "testsrv",
@@ -527,9 +541,11 @@ func TestGetMetricSpecTable(t *testing.T) {
527541
TargetPendingRequests: ptr.To[int32](123),
528542
},
529543
}
530-
namespaceLister.EXPECT().
531-
Get(httpso.GetName()).
532-
Return(httpso, nil)
544+
namespaceLister.
545+
EXPECT().
546+
Get(validHTTPScaledObjectName).
547+
Return(httpso, nil).
548+
Times(1)
533549

534550
return informer
535551
},
@@ -538,23 +554,31 @@ func TestGetMetricSpecTable(t *testing.T) {
538554
r := require.New(t)
539555
r.NoError(err)
540556
r.NotNil(res)
541-
r.Equal(1, len(res.MetricSpecs))
557+
r.Len(res.MetricSpecs, 1)
542558
spec := res.MetricSpecs[0]
543559
r.Equal(MetricName(&types.NamespacedName{Namespace: ns, Name: t.Name()}), spec.MetricName)
544560
r.Equal(int64(123), spec.TargetSize)
545561
},
562+
scalerMetadata: map[string]string{
563+
k8s.HTTPScaledObjectKey: validHTTPScaledObjectName,
564+
},
546565
},
547566
{
548567
name: "interceptor",
549568
defaultTargetMetric: 0,
550569
newInformer: func(t *testing.T, ctrl *gomock.Controller) *informersexternalversionshttpv1alpha1mock.MockHTTPScaledObjectInformer {
551-
informer, _, namespaceLister := newMocks(ctrl, nil)
552-
553-
namespaceLister.EXPECT().
554-
Get(gomock.Any()).
555-
DoAndReturn(func(name string) (*httpv1alpha1.HTTPScaledObject, error) {
556-
return nil, errors.NewNotFound(httpv1alpha1.Resource("httpscaledobject"), name)
557-
})
570+
namespaceLister := listershttpv1alpha1mock.NewMockHTTPScaledObjectNamespaceLister(ctrl)
571+
lister := listershttpv1alpha1mock.NewMockHTTPScaledObjectLister(ctrl)
572+
informer := informersexternalversionshttpv1alpha1mock.NewMockHTTPScaledObjectInformer(ctrl)
573+
574+
informer.EXPECT().
575+
Lister().
576+
Return(lister).
577+
AnyTimes()
578+
lister.EXPECT().
579+
HTTPScaledObjects(ns).
580+
Return(namespaceLister).
581+
AnyTimes()
558582

559583
return informer
560584
},
@@ -563,7 +587,7 @@ func TestGetMetricSpecTable(t *testing.T) {
563587
r := require.New(t)
564588
r.NoError(err)
565589
r.NotNil(res)
566-
r.Equal(1, len(res.MetricSpecs))
590+
r.Len(res.MetricSpecs, 1)
567591
spec := res.MetricSpecs[0]
568592
r.Equal(MetricName(&types.NamespacedName{Namespace: ns, Name: t.Name()}), spec.MetricName)
569593
r.Equal(int64(1000), spec.TargetSize)
@@ -574,40 +598,31 @@ func TestGetMetricSpecTable(t *testing.T) {
574598
},
575599
}
576600

577-
for i, c := range cases {
578-
testName := fmt.Sprintf("test case #%d: %s", i, c.name)
579-
// capture tc in scope so that we can run the below test
580-
// in parallel
581-
testCase := c
601+
for i, tc := range cases {
602+
testName := fmt.Sprintf("test case #%d: %s", i, tc.name)
582603
t.Run(testName, func(t *testing.T) {
583604
ctrl := gomock.NewController(t)
584605
defer ctrl.Finish()
585606

586607
ctx := context.Background()
587608
t.Parallel()
588609
lggr := logr.Discard()
589-
informer := testCase.newInformer(t, ctrl)
610+
informer := tc.newInformer(t, ctrl)
611+
590612
ticker, pinger, err := newFakeQueuePinger(lggr)
591613
if err != nil {
592-
t.Fatalf(
593-
"error creating new fake queue pinger and related components: %s",
594-
err,
595-
)
614+
t.Fatalf("error creating new fake queue pinger: %s", err)
596615
}
597616
defer ticker.Stop()
598-
hdl := newImpl(
599-
lggr,
600-
pinger,
601-
informer,
602-
testCase.defaultTargetMetric,
603-
)
617+
618+
hdl := newImpl(lggr, pinger, informer, tc.defaultTargetMetric)
604619
scaledObjectRef := externalscaler.ScaledObjectRef{
605620
Namespace: ns,
606621
Name: t.Name(),
607-
ScalerMetadata: testCase.scalerMetadata,
622+
ScalerMetadata: tc.scalerMetadata,
608623
}
609-
ret, err := hdl.GetMetricSpec(ctx, &scaledObjectRef)
610-
testCase.checker(t, ret, err)
624+
res, err := hdl.GetMetricSpec(ctx, &scaledObjectRef)
625+
tc.checker(t, res, err)
611626
})
612627
}
613628
}

0 commit comments

Comments
 (0)