kubectl: Use metrics-server for kubectl top commands #56206

brancz · 2017-11-22T11:05:18Z

What this PR does / why we need it:

This PR implements support for the kubectl top commands to use the metrics-server as an aggregated API, instead of requesting the metrics from heapster directly. If the metrics.k8s.io API is not served by the apiserver, then this still falls back to the previous behavior.

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 #55489

Special notes for your reviewer:

As utilizing heapster as well as the v1alpha1 version of the metrics API is discouraged, I intentionally implemented the support very separated, so that once it is decided, that support is entirely removed, this will make it easy.

Release note:

Support metrics API in `kubectl top` commands.

/cc @kubernetes/sig-instrumentation-pr-reviews @DirectXMan12 @fgrzadkowski @piosz

brancz · 2017-11-22T11:07:48Z

/cc @kubernetes/sig-cli-maintainers

fgrzadkowski · 2017-11-22T14:18:39Z

@piosz

brancz · 2017-11-23T10:03:44Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

DirectXMan12

overall looks fine. I'd add a comment to the code about why the getNodeMetricsFromMetricsAPI isn't just implementing the same interface as the existing client (your logic makes sense, but you should probably leave a note).

Can you simplify the testing by using a fake metrics client instead of stubbing out the HTTP client?

DirectXMan12 · 2017-11-27T19:56:41Z

pkg/kubectl/cmd/testing/fake.go

+	fakeClient := f.tf.Client.(*fake.RESTClient)
+	clientset := metricsclientset.NewForConfigOrDie(f.tf.ClientConfig)
+
+	clientset.MetricsV1alpha1().RESTClient().(*restclient.RESTClient).Client = fakeClient.Client


we've got fake clientsets generated for the metrics clients. Do you really need to do this?

Tried that and getting this error:

error: no kind "NodeMetricsList" is registered for version "metrics/v1beta1"

Any idea?

hmm... not off the top of my head. I'll have to try and reproduce locally

I'll prepare a branch to share to show you what I mean.

Not sure what I did wrong there, but I managed to get it to work. Do you mind re-reviewing @DirectXMan12 ? :) (oh and happy new year! 🎉)

rootfs · 2017-11-30T16:20:07Z

/unassign

brancz · 2017-12-10T15:24:56Z

Due to KubeCon and now vacation this was delayed a bit, but I'm hoping I will get back to it the week before Christmas.

brancz · 2017-12-31T14:31:20Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

brancz · 2018-01-02T09:57:04Z

Now using the fake metrics client in tests as suggested by @DirectXMan12. PTAL @fgrzadkowski @piosz @DirectXMan12 .

DirectXMan12

I think we're almost there. A couple more issues with the tests, and the discovery mechanics may need tweaking.

DirectXMan12 · 2018-01-04T16:26:38Z

pkg/kubectl/cmd/top_node_test.go

+	tf.Printer = &testPrinter{}
+	tf.Client = &fake.RESTClient{
+		NegotiatedSerializer: ns,
+		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {


if this is for discovery, this should be structured so that you can use a fake version of the discovery client to accomplish this. You should basically never have to use a fake HTTP client except for when simulating a Heapster response

It is, but the problem is that the underlying testFactory deeply integrates only with the fake rest client. My feeling is that changing that is out of scope for this PR, it would have a lot of impact on the kubectl code, and those changes would be much larger then the PR so far.

ack, we'll put that off until later.

DirectXMan12 · 2018-01-04T16:26:44Z

pkg/kubectl/cmd/top_node.go

+		if group.Name == metricsapi.GroupName {
+			metricsAPIAvailable = true
+		}
+	}


We might want to check version as well

Alternatively, we could try the metrics API client out of the box, and then retrying with the Heapster client if it returns a NotFound error. I'm conflicted as to which is the correct approach

Yes true, I'll make sure to add a version check.

piosz · 2018-01-11T21:49:18Z

@kubernetes/kubectl-maintainers

piosz · 2018-01-11T21:55:46Z

/lgtm
Many thanks for doing this.
Apologies for the huge delay.

soltysh · 2018-01-18T13:34:54Z

/retest

/assign @pwittrock
for approval

brancz · 2018-01-23T07:22:44Z

/test pull-kubernetes-e2e-kops-aws

piosz

/lgtm
/approve

DirectXMan12 · 2018-01-23T18:43:34Z

/lgtm

just waiting on someone from the kubectl owners.
ping @kubernetes/sig-cli-pr-reviews can someone take a look and approve?

pwittrock · 2018-01-23T20:05:54Z

/approve no-issue

k8s-ci-robot · 2018-01-23T20:06:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, DirectXMan12, piosz, pwittrock, soltysh

Associated issue: #55489

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [pwittrock]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-01-23T21:17:31Z

Automatic merge from submit-queue (batch tested with PRs 56206, 58525). If you want to cherry-pick this change to another branch, please follow the instructions here.

Since kubernetes#56206, `kubectl top` uses the Resource Metrics API (metrics.k8s.io) and falls back to Heapster, which is deprecated. This commit updates the description of command `kubectl top` to reflect this behavior.

k8s-github-robot assigned rootfs and shiywang Nov 22, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2017

brancz force-pushed the top-metrics-s branch from 0569a4d to 95d5b33 Compare November 23, 2017 08:57

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2017

DirectXMan12 suggested changes Nov 27, 2017

View reviewed changes

DirectXMan12 mentioned this pull request Nov 27, 2017

OpenAPI spec does not exists kubernetes-sigs/metrics-server#27

Closed

k8s-ci-robot unassigned rootfs Nov 30, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017

brancz force-pushed the top-metrics-s branch from 95d5b33 to 20a9ea9 Compare December 26, 2017 11:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 26, 2017

kubernetes deleted a comment from k8s-github-robot Dec 26, 2017

brancz force-pushed the top-metrics-s branch from 20a9ea9 to 1597c70 Compare December 31, 2017 12:40

DirectXMan12 suggested changes Jan 4, 2018

View reviewed changes

piosz assigned piosz and DirectXMan12 and unassigned shiywang Jan 11, 2018

k8s-ci-robot assigned pwittrock Jan 18, 2018

brancz force-pushed the top-metrics-s branch from 1597c70 to 4647230 Compare January 22, 2018 09:00

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2018

brancz mentioned this pull request Jan 22, 2018

Display PVC capacity usage using kubectl kubernetes/enhancements#497

Closed

kubectl: Use metrics-server for kubectl top commands

b1448ad

brancz force-pushed the top-metrics-s branch from 4647230 to b1448ad Compare January 22, 2018 20:48

piosz approved these changes Jan 23, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018

DirectXMan12 approved these changes Jan 23, 2018

View reviewed changes

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jan 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2018

k8s-github-robot merged commit 3cbb62b into kubernetes:master Jan 23, 2018

brancz deleted the top-metrics-s branch January 23, 2018 21:21

DirectXMan12 mentioned this pull request Feb 9, 2018

Kubectl top no doesn't wotk when using HPA without heapster #59438

Closed

x13n mentioned this pull request Mar 12, 2018

Add Troubleshooting sections to Heapster and Metrics Server addons documentation #60979

Merged

MaciekPytel mentioned this pull request May 11, 2018

hpa problem on k8s 1.9.0 #57673

Closed

neerfri mentioned this pull request Sep 27, 2018

Metrics Server not working awslabs/amazon-eks-ami#55

Closed

olivierlemasle mentioned this pull request Jul 30, 2019

Fix kubectl top description #80752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl: Use metrics-server for kubectl top commands #56206

kubectl: Use metrics-server for kubectl top commands #56206

brancz commented Nov 22, 2017 •

edited

Loading

brancz commented Nov 22, 2017

fgrzadkowski commented Nov 22, 2017

brancz commented Nov 23, 2017

DirectXMan12 left a comment

DirectXMan12 Nov 27, 2017

brancz Nov 30, 2017

DirectXMan12 Dec 1, 2017

brancz Dec 18, 2017

brancz Jan 2, 2018

rootfs commented Nov 30, 2017

brancz commented Dec 10, 2017

brancz commented Dec 31, 2017

brancz commented Jan 2, 2018

DirectXMan12 left a comment

DirectXMan12 Jan 4, 2018

brancz Jan 20, 2018

DirectXMan12 Jan 23, 2018

DirectXMan12 Jan 4, 2018

DirectXMan12 Jan 4, 2018

brancz Jan 20, 2018

piosz commented Jan 11, 2018

piosz commented Jan 11, 2018

soltysh commented Jan 18, 2018

brancz commented Jan 23, 2018

piosz left a comment

DirectXMan12 commented Jan 23, 2018

pwittrock commented Jan 23, 2018

k8s-ci-robot commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

kubectl: Use metrics-server for kubectl top commands #56206

kubectl: Use metrics-server for kubectl top commands #56206

Conversation

brancz commented Nov 22, 2017 • edited Loading

brancz commented Nov 22, 2017

fgrzadkowski commented Nov 22, 2017

brancz commented Nov 23, 2017

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Nov 30, 2017

brancz commented Dec 10, 2017

brancz commented Dec 31, 2017

brancz commented Jan 2, 2018

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piosz commented Jan 11, 2018

piosz commented Jan 11, 2018

soltysh commented Jan 18, 2018

brancz commented Jan 23, 2018

piosz left a comment

Choose a reason for hiding this comment

DirectXMan12 commented Jan 23, 2018

pwittrock commented Jan 23, 2018

k8s-ci-robot commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

brancz commented Nov 22, 2017 •

edited

Loading