KEP-5325: HPA - Improve pod selection accuracy across workload types

[omerap12]

• One-line PR description: HPA - Improve pod selection accuracy across workload types

• Issue link: HPA: Improve pod selection accuracy across workload types #5325

/sig autoscaling

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[omerap12]

This is a work in progress - looking for feedback and opinions.

[adrianmoisey]

Some low hanging changes

[soltysh]

I focused mostly on the design details, including the API surface. But once this gets opt-ed into the release I'll look carefully for the remaining bits, including the PRR portion of the doc.

[soltysh]

Nit: you're missing keps/prod-readiness/sig-autoscaling/5325.yaml based of https://github.com/kubernetes/enhancements/blob/master/keps/prod-readiness/template/nnnn.yaml, feel free to add my name as the PRR approver for alpha.

[soltysh]

Also, make sure to get a lead opt-in #5325 into 1.34 release.

[soltysh]

Nit: please make sure to break lines, since this makes the reviews easier to read. This applies to entire document.

[soltysh]

Based on https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md, specifically this part:

Think twice about bool fields. Many ideas start as boolean but eventually trend towards 
a small set of mutually exclusive options. Plan for future expansions by describing the 
policy options explicitly as a string type alias (e.g. TerminationMessagePolicy).

When we were introducing new mechanism to PDBs we went with UnhealthyPodEvictionPolicy. You'll notice that pattern used also heavily across apps/v1 types.

In your case that'd mean introducing something like PodSelectionType with two initial values Selector or Labels being the default, and backward compatible, and OwnerRefs. This will ensure you can easily extend the mechanism in the future.

[omerap12]

TIL, thank you for the reference! :)

[soltysh]

You'll want to list integration tests you're planning to add.

[adrianmoisey]

Would selectionStrategy be a shorter more concise phrase for what we're trying to achieve?

[omerap12]

Agree.

[omerap12]

WIP here: kubernetes/kubernetes#132018

[adrianmoisey]

Suggested change

	metrics:
	- my_feature_metric
	metrics: []

How necessary is it to make a metric? Taking a look at other similar features, I found an example that added a metric that would be the count of the number of times that the new API field is used. Is that necessary?

(I guess I'm asking @soltysh that question)

[raywainman]

super nit - perhaps OwnerReference is better here since HPA would only use one reference?

[omerap12]

Yeah makes sense

[adrianmoisey]

Nitty counter point: Sometimes there are multiple hops to get from the Pod to the targetRef. So even though Pod -> ReplicaSet and ReplicaSet -> Deployment is 1 OwnerRef each, in total it's 2 OwnerRefs.

I'm personally not strongly opinionated on this and can go either direction.

[raywainman]

How high will it go? Will it stop when the owner matches the targetRef in the HPA?

[omerap12]

I think the correct direction is down (since we start from the targetRef and go down to the pods it owns), so we just get the pods by looking at their ownerReference.

[raywainman]

As it goes up the chain, the HPA Controller will need to fetch controllers, will we cache these too?

Otherwise you may be fetching the same controller objects every 15s for every pod :(

(In VPA we have a whole bunch of heuristics here too to reduce load on API Server: https://github.com/kubernetes/autoscaler/blob/9220470d9f5154eefc82f53f39de4efb76befe77/vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher.go#L175)

[omerap12]

Yeah I forgot about this. You're right - I originally implemented the fetcher to work from the top down, starting at the Deployment and traversing down to the Pod. I agree we should reverse this approach: start from the Pod and cache intermediate controllers along the way to avoid repeatedly fetching the same objects every 15s.

[omerap12]

Currently we don't do it for every pod, but I agree it can be better (https://github.com/kubernetes/kubernetes/pull/132018/files#diff-1d52b688f694faeedda046d7d6fb75c3a43ad69511db4195c370ece15657a434R127-R164 )

[omerap12]

Modified as follows: kubernetes/kubernetes@387fbbb

[omerap12]

In my opinion, the cache felt a bit excessive. It does reduce some API calls, but is it really worth it?

[raywainman]

One additional comment here - do you know if there is other prior work anywhere else in Kubernetes for this problem?

If I understand correctly, this label overlapping use-case is actually quite prevalent everywhere throughout the ecosystem - Service is one that I can think about right off the top of my head.

If there is, it would be great to tackle that in a consistent way.

[adrianmoisey]

One additional comment here - do you know if there is other prior work anywhere else in Kubernetes for this problem?

If I understand correctly, this label overlapping use-case is actually quite prevalent everywhere throughout the ecosystem - Service is one that I can think about right off the top of my head.

If there is, it would be great to tackle that in a consistent way.

I don't believe this is a general problem.

The thing with the HPA is that it has a targetRef, and not a selector.

$ kubectl explain Horizontalpodautoscaler.spec.scaleTargetRef
GROUP:      autoscaling
KIND:       HorizontalPodAutoscaler
VERSION:    v2

FIELD: scaleTargetRef <CrossVersionObjectReference>


DESCRIPTION:
    scaleTargetRef points to the target resource to scale, and is used to the
    pods for which metrics should be collected, as well as to actually change
    the replica count.
    CrossVersionObjectReference contains enough information to let you identify
    the referred resource.

FIELDS:
  apiVersion	<string>
    apiVersion is the API version of the referent

  kind	<string> -required-
    kind is the kind of the referent; More info:
    https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds

  name	<string> -required-
    name is the name of the referent; More info:
    https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names

As a user I read this as "The HPA will use the target's Pods and Metrics to scale my workload". But it's actually saying "The HPA will scale this workload, and it will take the label selector from this workload to find Pods/Metrics".

A Service is different, it just takes in a selector:

$ kubectl explain Service.spec.selector
KIND:       Service
VERSION:    v1

FIELD: selector <map[string]string>


DESCRIPTION:
    Route service traffic to pods with label keys and values matching this
    selector. If empty or not present, the service is assumed to have an
    external process managing its endpoints, which Kubernetes will not modify.
    Only applies to types ClusterIP, NodePort, and LoadBalancer. Ignored if type
    is ExternalName. More info:
    https://kubernetes.io/docs/concepts/services-networking/service/

So as a user I'm more likely to worry about overlapping workloads with the Service than I am with an HPA.