Make autoscaling configuration available to the HPA reconciler. #4237
Conversation
The HPA reconciler used to not need this configuration but future changes will rely on the autoscaler config. In particular, the default target knobs will need to be able to the HPA reconciler to be able to correctly determine the target concurrency for the HPA spec once /metric=concurrency gets plumbed into the HPA as well.
knative-prow-robot
added
the
size/M
There was a problem hiding this comment.
@markusthoemmes: 0 warnings.
In response to this:
Proposed Changes
The HPA reconciler used to not need this configuration but future changes will rely on the autoscaler config. In particular, the default target knobs will need to be able to the HPA reconciler to be able to correctly determine the target concurrency for the HPA spec once /metric=concurrency gets plumbed into the HPA as well.
Release Note
NONE
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
approved
|
The following is the coverage report on pkg/.
|
|
/assign @vagababov |
| } | ||
| } | ||
|
|
||
| type testConfigStore struct { |
There was a problem hiding this comment.
Perhaps just type testConfigStore *config.Config?
There was a problem hiding this comment.
As discussed in Slack: This gets kinda hairy as I need cannot use the pointer type as the aliased type. I'd end up with type testConfigStore config.Config. All of the functions will remain pointer receivers though.
In the ToContext method I'll need to cast the value so the compiler eats it. On the caller side I'll need to define extra variables to be able to get a pointer to this new type.
Here's what the diff would look like:
diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go
index 0af6c524..74de5a22 100644
--- a/pkg/reconciler/autoscaling/hpa/hpa_test.go
+++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go
@@ -415,13 +415,14 @@ func TestReconcile(t *testing.T) {
}}
defer logtesting.ClearAll()
+ configStore := testConfigStore(*defaultConfig())
table.Test(t, MakeFactory(func(listers *Listers, opt reconciler.Options) controller.Reconciler {
return &Reconciler{
Base: reconciler.NewBase(opt, controllerAgentName),
paLister: listers.GetPodAutoscalerLister(),
sksLister: listers.GetServerlessServiceLister(),
hpaLister: listers.GetHorizontalPodAutoscalerLister(),
- configStore: &testConfigStore{config: defaultConfig()},
+ configStore: &configStore,
}
}))
}
@@ -506,12 +507,10 @@ func defaultConfig() *config.Config {
}
}
-type testConfigStore struct {
- config *config.Config
-}
+type testConfigStore config.Config
func (t *testConfigStore) ToContext(ctx context.Context) context.Context {
- return config.ToContext(ctx, t.config)
+ return config.ToContext(ctx, (*config.Config)(t))
}
func (t *testConfigStore) WatchConfigs(w configmap.Watcher) {}
|
/lgtm |
…ive#4237) The HPA reconciler used to not need this configuration but future changes will rely on the autoscaler config. In particular, the default target knobs will need to be able to the HPA reconciler to be able to correctly determine the target concurrency for the HPA spec once /metric=concurrency gets plumbed into the HPA as well.
Proposed Changes
The HPA reconciler used to not need this configuration but future changes will rely on the autoscaler config. In particular, the default target knobs will need to be able to the HPA reconciler to be able to correctly determine the target concurrency for the HPA spec once /metric=concurrency gets plumbed into the HPA as well.
Release Note