Enable HPA metrics REST clients by default #52553
Conversation
Since metrics-server now is deployable and supports metrics.k8s.io/v1beta1, we should enable usage of the REST clients by default.
This commit disables using the HPA metrics REST clients in favor of the legacy clients if metrics-server is not deployed.
k8s-ci-robot
added
the
size/S
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DirectXMan12 Assign the PR to them by writing Associated issue: 52548 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
release-note
|
Test failures look real. |
There was a problem hiding this comment.
Kind of LGTM, but the other PR has to be merged first.
Also I want to know what the upgrade scenario looks like.
Can this break any user in any condition?
cc @kubernetes/sig-autoscaling-pr-reviews @kubernetes/sig-auth-pr-reviews (for RBAC changes)
@kubernetes/sig-api-machinery-pr-reviews (for the API group changes)
FYI @kubernetes/kubernetes-release-managers
Since we weren't running the HPA with metrics REST clients by default, we had no bootstrap policy enabling the HPA controller to talk to the metrics APIs. This adds permissions for the HPA controller to talk list pods.metrics.k8s.io, and list any resource in custom.metrics.k8s.io.
DirectXMan12
force-pushed
the
feature/enable-hpa-rest-clients-by-default
branch
from
9e87e19 to
f4bc71b
Compare
k8s-ci-robot
added
size/M
|
So, the story on upgrades is this: previously, since this flag was off by default, if you didn't pass this flag explicitly, your HPA controller would fetch metrics by directly connecting to Heapster over legacy API paths. After this PR, by default, the HPA controller tries to look for metrics by checking the API at metrics.k8s.io, via the "main" API server (which in turn uses aggregation). Thus, while doing the upgrade, you'd either have to start up metrics server (now run by default in the 1.8 kube-up), or explicitly set this flag to false. If you don't do one of those two, your HPA won't work. |
|
We'd meant to actually put this PR in earlier in the release, but were delayed by some repository synchronization issues -- in order for things to pass tests, we needed a properly updated version of metrics server. |
This sounds like a breaking change. Were there notices in the last release about deprecating the current behavior? |
@ericchiang The intention was there in the original PR that introduced this functionality, but unfortunately, looking over the previous iteration's release notes, the wording did not actually get into the release notes :-/. I'd love to have this enabled by default this release, but I understand if we have to wait until next. |
|
/retest |
|
Okay, per our deprecation policy it sounds likes we need to announce the existing behavior as deprecated for a release before changing the default. https://kubernetes.io/docs/reference/deprecation-policy/ Removing this from the milestone and adding #52572 to the milestone in its place. |
|
I also agree with @ericchiang (although I'd like to see this personally) |
Done :-) |
|
What is the status of this PR? |
|
/retest |
|
@DirectXMan12 You broke out some of this code to other PRs, right? |
|
@DirectXMan12: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
@DirectXMan12 What's the status of this PR? |
|
This got enabled as part of #53205. Closing this PR |
Now that the metrics APIs are in beta and metrics-server has the appropriate support (as of #52548), we should enable the REST metrics clients by default instead of using the legacy clients.