Prevent running two HPA cachers for different API versions

[serathius]

/kind regression

NONE

Reuse single REST for different HPA versions, reducing resource usage.
Allocating REST is not free as if watch cache is enabled, each will LIST and WATCH etcd independently and cache all objects in memory.

/assign @jpbetz

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

[serathius]

/cc @deads2k

[jpbetz]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f2421c7febc6ee4a403d8957149ab3f589a9f8cd

[serathius]

/assign @smarterclayton

[serathius]

/cc @liggitt

[liggitt]

I don't think this is a regression, this has always been this way, right?

[serathius]

I don't think this is a regression, this has always been this way, right?

Right, it dates to introduction of v2 horizontalpodautoscalers API in K8s 1.6.

/kind feature
/remove-kind regression

[liggitt]

we really want the API group/version setup to be isolated / independent... combining these two into a single function makes it easier to accidentally reuse shared variables incorrectly. If the thing we want to accomplish is only constructing HPA storage once, let's structure the code to accomplish that without losing isolated version setup:

var (
	hpaOnce sync.Once
	hpaStorage *REST
	hpaStatusStorage *StatusREST
	hpaStorageErr error
)
hpaStorageGetter := func() (*REST, *StatusREST, error) {
  hpaOnce.Do(func(){
    hpaStorage, hpaStatusStorage, hpaStorageErr := horizontalpodautoscalerstore.NewREST(restOptionsGetter)
  })
  return hpaStorage, hpaStatusStorage, hpaStorageErr
}

if storageMap, err := p.v2Storage(apiResourceConfigSource, hpaStorageGetter); err != nil {
	return genericapiserver.APIGroupInfo{}, err
} else if len(storageMap) > 0 {
	apiGroupInfo.VersionedResourcesStorageMap[autoscalingapiv2.SchemeGroupVersion.Version] = storageMap
}

if storageMap, err := p.v1Storage(apiResourceConfigSource, hpaStorageGetter); err != nil {
	return genericapiserver.APIGroupInfo{}, err
} else if len(storageMap) > 0 {
	apiGroupInfo.VersionedResourcesStorageMap[autoscalingapiv1.SchemeGroupVersion.Version] = storageMap
}

[liggitt]

the existing code is also ~identical to the versioned setup code used for ~every other API group currently ... this isn't an issue isolated to HPA, it just happens to only manifest with default config for HPA because it's the only thing that has two GA API versions of the same group

it would be ideal to fix this systemically and consistently so we don't have the same issue everywhere

edit: actually, I think events has the same issue in default configs, constructing twice to serve via events/v1 and core/v1

[serathius]

Makes sense, I didn't notice the events as it doesn't have watch cache enabled. I noticed that we create 2 cachers for serviceaccounts but we shutdown one of them.

[serathius]

Done

[liggitt]

Making these local means that on subsequent invocations, this function will return nil, nil, nil. These variables have to live outside the closure.

[serathius]

Right

[liggitt]

Suggested change

	func (p RESTStorageProvider) v1Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter hpaStorageGetter) (map[string]rest.Storage, error) {
	func (p RESTStorageProvider) v1Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, storageGetter hpaStorageGetter) (map[string]rest.Storage, error) {

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 42dee923e5c40eeeee1d8c312c0dd493baa9837a

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, liggitt, serathius

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:

• pkg/registry/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pacoxu]

/sig autoscaling api-machinery