search

kubernetes-sigs / metrics-server

Scalable and efficient source of container resource metrics for Kubernetes built-in autoscaling pipelines.
https://kubernetes.io/docs/tasks/debug-application-cluster/resource-metrics-pipeline/
Apache License 2.0
5.72k stars 1.86k forks source link

Fix kubectl get PodMetrics/NodeMetrics #1436

Closed dgrisonnet closed 6 months ago

dgrisonnet commented 6 months ago

What this PR does / why we need it:

This PR fixes a bug that was introduced by https://github.com/kubernetes-sigs/metrics-server/commit/e056f912f332869924eaed2ad5bd97da2661d904 when bumping Kubernetes dependencies to 1.27+ which broke kubectl get PodMetrics/NodeMetrics commands.

With the 1.27 Kubernetes upgrade, a new GetSingularName method needed to be implemented by the metrics rest.Storage. While doing it, we set a value for the singular name of the resources when it used to be empty. Due to that, the kube-apiserver stop guessing the resource name based on its Kind, which broken kubectl get PodMetrics/NodeMetrics.

To fix that, we need to blank out the singular name of the resources, otherwise we will be stuck with the following commands:

- kubectl get pod.metric
- kubectl get node.metric

Instead of the more commonly used:

- kubectl get PodMetrics
- kubectl get NodeMetrics

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #

k8s-ci-robot commented 6 months ago

This issue is currently awaiting triage.

If metrics-server contributors determine 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
dgrisonnet commented 6 months ago

Seems like update the resource name to nodemetrics and podmetrics in https://github.com/kubernetes-sigs/metrics-server/blob/master/pkg/api/install.go#L48-L49 would break the current clientsets https://github.com/kubernetes/metrics/blob/master/pkg/client/clientset/versioned/typed/metrics/v1beta1/podmetrics.go#L82. I guess the only way to fix the issue would then be to blank out the singularName. I'll try that out.

dgrisonnet commented 6 months ago

/retest-required

slashpai commented 6 months ago

/retest-required

k8s-ci-robot commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, yangjunmyfm192085

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/metrics-server/blob/master/OWNERS)~~ [dgrisonnet] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
slashpai commented 6 months ago

Should we have a test as well for this to make sure this command works always?

yangjunmyfm192085 commented 6 months ago

/lgtm

yangjunmyfm192085 commented 6 months ago

@dgrisonnet Do we need cherrypick to release 0.6 and 0.7 branches?

yangjunmyfm192085 commented 6 months ago

Should we have a test as well for this to make sure this command works always?

Maybe we can consider adding e2e test cases for this?

dgrisonnet commented 6 months ago

I'll cherry-pick to release-0.7