add configurable metrics for kubelet cadvisor interface #88841

KielChan · 2020-03-05T10:21:32Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Now in kubelet, we can only use hard-code metrics.
case 1: If user want to use advanced tcp stats (contents like netstat -s)in cadvisor v0.36.0, we can set it advtcp enabled.
case 2: In complex production env, there are 250 pods on one node, kubelet will hang by syscall if all metrics provided are scanning cgroups file background. And to relieve this pain, we can disable some metrics like network, tcp, udp and so on.
case 3: If user don't care about metrics, this feature are disabled default.
case 4: for #68522 , we can pre-remove some metrics from default configuration now.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Signed-off-by: ChenQingya <qingya.chen520@gmail.com>

k8s-ci-robot · 2020-03-05T10:21:34Z

@KielChan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2020-03-05T10:21:41Z

Hi @KielChan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

fejta-bot · 2020-03-05T11:24:58Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

dashpole · 2020-03-05T16:35:04Z

/assign

szuecs · 2020-03-05T19:46:09Z

pkg/features/kube_features.go

@@ -659,6 +665,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
 	DefaultIngressClass:                            {Default: true, PreRelease: featuregate.Beta},
 	HugePageStorageMediumSize:                      {Default: false, PreRelease: featuregate.Alpha},
 	AnyVolumeDataSource:                            {Default: false, PreRelease: featuregate.Alpha},
+	EnableCustomCadvisorMetrics:                    {Default: false, PreRelease: featuregate.Alpha},


I would suggest CustomCadvisorMetrics instead

szuecs · 2020-03-05T19:46:45Z

pkg/kubelet/apis/config/types.go

+	// Only if feature gates CustomCadvisorMetrics set to be true, it will accept custom configuration like:
+	// cpu,sched,memory,disk,network,tcp
+	// +optional
+	CadvisorMetricsEnabled []string


I would suggest CadvisorMetricsinstead

fine, I'll fix it

szuecs · 2020-03-05T19:47:58Z

@KielChan nice feature, but can you please add some tests?
Additionally alpha features need to be guarded as far as I know and in your changes I don't see that you use

if utilfeature.DefaultFeatureGate.Enabled(features.EnableCustomCadvisorMetrics)

Signed-off-by: ChenQingya <qingya.chen520@gmail.com>

KielChan · 2020-03-06T06:28:12Z

@KielChan nice feature, but can you please add some tests?
Additionally alpha features need to be guarded as far as I know and in your changes I don't see that you use
if utilfeature.DefaultFeatureGate.Enabled(features.EnableCustomCadvisorMetrics)

I didn't use if before in server.go, just used Enabled as a flag to determine whether to acquire custom metrics from configuration. And now, I have fixed it and add some UT for default or custom cadivsor metrics set.

szuecs · 2020-03-06T09:04:27Z

pkg/kubelet/cadvisor/util_test.go

@@ -55,3 +56,45 @@ func TestCapacityFromMachineInfoWithHugePagesEnable(t *testing.T) {
 func TestCrioSocket(t *testing.T) {
 	assert.EqualValues(t, CrioSocket, crio.CrioSocket, "CrioSocket in this package must equal the one in github.com/google/cadvisor/container/crio/client.go")
 }
+
+func TestDefaultCadvisorMetricSet(t *testing.T) {
+	usingLegacyStatsTable := []bool{false, true}


Thanks for creating tests!
What about true,true and false,false?

usingLegacyStats just depends on kubelet runtime or remote runtime endpoint, and its value affects whether to collect disk stats in cadvisor.
Its combination is just true or false, when set custom cadivsor metrics if CustomCadvisorMetrics feature gate enabled or just like before, i.e. Default cadvisor metric set. So true,ture or false,false makes no sense.

mattjmcnaughton · 2020-03-06T18:50:15Z

I defer to @dashpole here, but given the scope of this change and the fact that it adds new functionality/changes the API, I'm wondering if it would be a good candidate for a Kubernetes Enhancement Proposal (KEP)? Thoughts? :)

dashpole · 2020-03-12T00:15:36Z

The guidance we have been giving is to run cAdvisor as a daemonset with the metrics you are interested in turned on, and the rest disabled. There is some resource overhead to doing this, but it gives users complete control over all cAdvisor settings.

KielChan · 2020-03-12T03:39:51Z

The guidance we have been giving is to run cAdvisor as a daemonset with the metrics you are interested in turned on, and the rest disabled. There is some resource overhead to doing this, but it gives users complete control over all cAdvisor settings.

Totally agree with you.

In my opinion, kubelet cadvisor and cadvisor daemonset are two aspects. Cadvisor daemonset does expose any metric set provided. As for kubelet cadvisor interface work background, it is out of control 'cos only hard code metric set can be exposed.

And for user do not care about what metric set exposed, it is just like before. We hope to do everything we can better than do nothing.

k8s-ci-robot · 2020-04-11T18:11:49Z

@KielChan: PR needs rebase.

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.

fejta-bot · 2020-07-10T19:06:29Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

KielChan · 2020-08-03T04:20:13Z

Due to cadvisor v0.37.0 has released, I will re-write another PR to remove hard code like AllMetricsSet in kubelet.

fejta-bot · 2020-09-02T04:54:01Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-10-02T05:36:42Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-10-02T05:36:55Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

yuzhiquan · 2022-04-08T02:13:28Z

@KielChan would you like move on this pr, i think this feature can fix some issue in our cluster?

yuzhiquan · 2022-04-08T02:13:48Z

/reopen

k8s-ci-robot · 2022-04-08T02:14:03Z

@yuzhiquan: Reopened this PR.

In response to this:

/reopen

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.

k8s-ci-robot · 2022-04-08T02:14:09Z

@KielChan: 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/test-infra repository.

k8s-ci-robot · 2022-04-08T02:18:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KielChan
To complete the pull request process, please assign derekwaynecarr, smarterclayton after the PR has been reviewed.
You can assign the PR to them by writing /assign @derekwaynecarr @smarterclayton in a comment when ready.

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

Needs approval from an approver in each of these files:

api/OWNERS
cmd/kubelet/OWNERS
pkg/features/OWNERS
pkg/kubelet/OWNERS
pkg/kubelet/apis/config/OWNERS
staging/src/k8s.io/kubelet/config/OWNERS
test/e2e_node/OWNERS

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

ehashman · 2022-04-12T21:32:08Z

@yuzhiquan feel free to open a new PR based on the commits contained here, but I want to leave this PR closed unless it has been rebased and is ready for review.

This change, with API updates, will need a KEP. Another option is to go the approach I am testing in #109398.

/close

k8s-ci-robot · 2022-04-12T21:32:24Z

@ehashman: Closed this PR.

In response to this:

@yuzhiquan feel free to open a new PR based on the commits contained here, but I want to leave this PR closed unless it has been rebased and is ready for review.

This change, with API updates, will need a KEP. Another option is to go the approach I am testing in #109398.

/close

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.

add configurable metrics for kubelet cadvisor interface

83e6c60

Signed-off-by: ChenQingya <qingya.chen520@gmail.com>

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 5, 2020

KielChan mentioned this pull request Mar 5, 2020

add configurable metrics for kubelet cadvisor interface #88838

Closed

k8s-ci-robot requested review from dashpole and dchen1107 March 5, 2020 10:25

k8s-ci-robot assigned dashpole Mar 5, 2020

szuecs reviewed Mar 5, 2020

View reviewed changes

add unit test cases for default and custom cadivsor metric set handler

a897901

Signed-off-by: ChenQingya <qingya.chen520@gmail.com>

szuecs reviewed Mar 6, 2020

View reviewed changes

KielChan mentioned this pull request Mar 16, 2020

refactor AllMetrics to support external reference google/cadvisor#2429

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2020

RenaudWasTaken mentioned this pull request Jun 4, 2020

Return a NoopManager if metricset does not container the accelerator value google/cadvisor#2574

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 10, 2020

KielChan mentioned this pull request Aug 3, 2020

make cadvisor metrics set configurable in kubelet #93650

Closed

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 2, 2020

k8s-ci-robot closed this Oct 2, 2020

k8s-ci-robot reopened this Apr 8, 2022

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 8, 2022

k8s-ci-robot closed this Apr 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add configurable metrics for kubelet cadvisor interface #88841

add configurable metrics for kubelet cadvisor interface #88841

KielChan commented Mar 5, 2020

k8s-ci-robot commented Mar 5, 2020

k8s-ci-robot commented Mar 5, 2020

fejta-bot commented Mar 5, 2020

dashpole commented Mar 5, 2020

szuecs Mar 5, 2020

szuecs Mar 5, 2020

KielChan Mar 6, 2020

szuecs commented Mar 5, 2020

KielChan commented Mar 6, 2020

szuecs Mar 6, 2020

KielChan Mar 6, 2020

mattjmcnaughton commented Mar 6, 2020

dashpole commented Mar 12, 2020

KielChan commented Mar 12, 2020

k8s-ci-robot commented Apr 11, 2020

fejta-bot commented Jul 10, 2020

KielChan commented Aug 3, 2020

fejta-bot commented Sep 2, 2020

fejta-bot commented Oct 2, 2020

k8s-ci-robot commented Oct 2, 2020

yuzhiquan commented Apr 8, 2022

yuzhiquan commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

ehashman commented Apr 12, 2022

k8s-ci-robot commented Apr 12, 2022

add configurable metrics for kubelet cadvisor interface #88841

add configurable metrics for kubelet cadvisor interface #88841

Conversation

KielChan commented Mar 5, 2020

k8s-ci-robot commented Mar 5, 2020

k8s-ci-robot commented Mar 5, 2020

fejta-bot commented Mar 5, 2020

dashpole commented Mar 5, 2020

szuecs Mar 5, 2020

Choose a reason for hiding this comment

szuecs Mar 5, 2020

Choose a reason for hiding this comment

KielChan Mar 6, 2020

Choose a reason for hiding this comment

szuecs commented Mar 5, 2020

KielChan commented Mar 6, 2020

szuecs Mar 6, 2020

Choose a reason for hiding this comment

KielChan Mar 6, 2020

Choose a reason for hiding this comment

mattjmcnaughton commented Mar 6, 2020

dashpole commented Mar 12, 2020

KielChan commented Mar 12, 2020

k8s-ci-robot commented Apr 11, 2020

fejta-bot commented Jul 10, 2020

KielChan commented Aug 3, 2020

fejta-bot commented Sep 2, 2020

fejta-bot commented Oct 2, 2020

k8s-ci-robot commented Oct 2, 2020

yuzhiquan commented Apr 8, 2022

yuzhiquan commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

ehashman commented Apr 12, 2022

k8s-ci-robot commented Apr 12, 2022