add volume kubelet_volume_stats_health_abnormal to kubelet #105585

fengzixu · 2021-10-09T06:35:47Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR add kubelet_volume_stats_health_abnormal metrics to kubelet based on this kep

Which issue(s) this PR fixes:

Fixes kubernetes/enhancements#2900

Special notes for your reviewer:

Does this PR introduce a user-facing change?

add one metrics(`kubelet_volume_stats_health_abnormal`) of volume health state to kubelet

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

https://github.com/kubernetes/enhancements/pull/2900

k8s-ci-robot · 2021-10-09T06:35:55Z

Hi @fengzixu. 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.

k8s-ci-robot · 2021-10-09T06:36:00Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

fengzixu · 2021-10-09T06:38:28Z

/ok-to-test

k8s-ci-robot · 2021-10-09T06:38:41Z

@fengzixu: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

fengzixu · 2021-10-09T06:39:03Z

/assign @xing-yang

fengzixu · 2021-10-09T06:39:20Z

/assign @fengzixu

pacoxu · 2021-10-09T10:27:44Z

/ok-to-test

pacoxu · 2021-10-09T10:29:22Z

/sig storage
/priority important-soon
it should be in 1.23 scope as kubernetes/enhancements#2900.
/cc msau42 gnufied

fengzixu · 2022-01-23T03:57:01Z

/retest

dgrisonnet · 2022-01-26T09:51:02Z

Looks good from my side in terms of instrumentation. Even though the metric that is introduced has two unbounded dimensions (namespace and PVC), the cardinality is limited to the number of PVC created in the cluster, so it is fine by me.

/lgtm

xing-yang · 2022-01-27T18:45:32Z

/assign @dashpole
@dashpole Can you review this?

dashpole · 2022-02-10T19:25:18Z

staging/src/k8s.io/kubelet/pkg/apis/stats/v1alpha1/types.go

+
+	// VolumeHealthStats contains data about volume health
+	// +optional
+	VolumeHealthStats *VolumeHealthStats `json:"volumeHealthStats,omitempty"`


Does this need to be included in the summary API? In general, the summary API contains metrics about resource usage, whereas the kubelet's /metrics endpoint contains the kubelet's operational metrics (e.g. health, latency, etc). Based on usage i'm aware of, people tend to scrape either the summary API or /metrics/cadvisor, and then also scrape /metrics as well. For users that do this, they would end up with duplicate metrics.

Hi @dashpole, let me know if I missed anything.
I see that Summary API contains PodStats:
https://github.com/kubernetes/kubernetes/blob/v1.24.0-alpha.3/staging/src/k8s.io/kubelet/pkg/apis/stats/v1alpha1/types.go#L28
PodStats contains VolumeStats:
https://github.com/kubernetes/kubernetes/blob/v1.24.0-alpha.3/staging/src/k8s.io/kubelet/pkg/apis/stats/v1alpha1/types.go#L127
VolumeHealthStats is added in VolumeStats here. Should this be sufficient?

I'm saying you should probably add these metrics to either /metrics or the summary API, but probably not both

I see... looks we've already done that for a bunch of volume metrics. You can resolve this comment in that case.

dashpole · 2022-03-09T20:44:34Z

/approve

gnufied · 2022-03-09T20:50:39Z

/assign @mrunalp

k8s-ci-robot · 2022-03-14T21:49:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, fengzixu, jonyhy96, mrunalp

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:

~~pkg/kubelet/metrics/OWNERS~~ [dashpole,mrunalp]
~~pkg/kubelet/server/OWNERS~~ [mrunalp]
~~staging/src/k8s.io/kubelet/OWNERS~~ [mrunalp]
~~test/e2e_node/OWNERS~~ [mrunalp]

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

k8s-triage-robot · 2022-03-15T02:45:18Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2022-03-15T06:36:19Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2022-03-15T10:57:19Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

mborsz · 2022-03-16T12:43:52Z

pkg/kubelet/metrics/collectors/volume_stats.go

@@ -120,7 +126,16 @@ func (collector *volumeStatsCollector) CollectWithStability(ch chan<- metrics.Me
 			addGauge(volumeStatsInodesDesc, pvcRef, float64(*volumeStat.Inodes))
 			addGauge(volumeStatsInodesFreeDesc, pvcRef, float64(*volumeStat.InodesFree))
 			addGauge(volumeStatsInodesUsedDesc, pvcRef, float64(*volumeStat.InodesUsed))
+			addGauge(volumeStatsHealthAbnormalDesc, pvcRef, convertBoolToFloat64(volumeStat.VolumeHealthStats.Abnormal))


This generates

panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1d5d77b] goroutine 2564 [running]: k8s.io/kubernetes/pkg/kubelet/metrics/collectors.(*volumeStatsCollector).CollectWithStability(0x0?, 0xc0016df860) pkg/kubelet/metrics/collectors/volume_stats.go:129 +0x89b

#108715 (comment)

Well that makes sense since volumeStat.VolumeHealthStats looks like an optional field:

// +optional VolumeHealthStats *VolumeHealthStats `json:"volumeHealthStats,omitempty"`

I'd suggest reverting this.

@fengzixu Can you take a look at this?

Sure. Let me check

File this PR to fix: #108758

…-health add volume kubelet_volume_stats_health_abnormal to kubelet

re-push "add volume kubelet_volume_stats_health_abnormal to kubelet #105585"

…-health add volume kubelet_volume_stats_health_abnormal to kubelet

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 9, 2021

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. area/kubelet labels Oct 9, 2021

k8s-ci-robot requested review from resouer and SergeyKanzhelev October 9, 2021 06:36

k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 9, 2021

fengzixu force-pushed the improvement-volume-health branch from b05c338 to 3bff32b Compare October 9, 2021 06:37

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 9, 2021

k8s-ci-robot assigned xing-yang Oct 9, 2021

k8s-ci-robot assigned fengzixu Oct 9, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2021

k8s-ci-robot requested a review from gnufied October 9, 2021 10:29

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

k8s-ci-robot assigned dashpole Jan 27, 2022

dashpole reviewed Feb 10, 2022

View reviewed changes

k8s-ci-robot assigned mrunalp Mar 9, 2022

mrunalp approved these changes Mar 14, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2022

k8s-ci-robot merged commit 1a5abe5 into kubernetes:master Mar 15, 2022

mborsz mentioned this pull request Mar 16, 2022

[Failing test] gce-cos-master-scalability-100 #108715

Closed

mborsz reviewed Mar 16, 2022

View reviewed changes

mborsz mentioned this pull request Mar 16, 2022

Revert "add volume kubelet_volume_stats_health_abnormal to kubelet" #108731

Merged

fengzixu pushed a commit to fengzixu/kubernetes that referenced this pull request Mar 17, 2022

Merge pull request kubernetes#105585 from fengzixu/improvement-volume…

5cb6fab

…-health add volume kubelet_volume_stats_health_abnormal to kubelet

github-actions bot mentioned this pull request Mar 22, 2022

Week Ending March 20, 2022 dev-obs/actus#403

Open

k8s-ci-robot added a commit that referenced this pull request Mar 30, 2022

Merge pull request #108758 from fengzixu/improvement-volume-health

b0254c8

re-push "add volume kubelet_volume_stats_health_abnormal to kubelet #105585"

muyangren2 pushed a commit to muyangren2/kubernetes that referenced this pull request Jul 14, 2022

Merge pull request kubernetes#105585 from fengzixu/improvement-volume…

300be85

…-health add volume kubelet_volume_stats_health_abnormal to kubelet

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add volume kubelet_volume_stats_health_abnormal to kubelet #105585

add volume kubelet_volume_stats_health_abnormal to kubelet #105585

fengzixu commented Oct 9, 2021

k8s-ci-robot commented Oct 9, 2021

k8s-ci-robot commented Oct 9, 2021

fengzixu commented Oct 9, 2021

k8s-ci-robot commented Oct 9, 2021

fengzixu commented Oct 9, 2021

fengzixu commented Oct 9, 2021

pacoxu commented Oct 9, 2021

pacoxu commented Oct 9, 2021

fengzixu commented Jan 23, 2022

dgrisonnet commented Jan 26, 2022

xing-yang commented Jan 27, 2022

dashpole Feb 10, 2022

xing-yang Feb 24, 2022

dashpole Mar 9, 2022

dashpole Mar 9, 2022 •

edited

Loading

dashpole commented Mar 9, 2022

gnufied commented Mar 9, 2022

k8s-ci-robot commented Mar 14, 2022

k8s-triage-robot commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

mborsz Mar 16, 2022

logicalhan Mar 16, 2022

logicalhan Mar 16, 2022

xing-yang Mar 16, 2022

fengzixu Mar 16, 2022

fengzixu Mar 17, 2022

add volume kubelet_volume_stats_health_abnormal to kubelet #105585

add volume kubelet_volume_stats_health_abnormal to kubelet #105585

Conversation

fengzixu commented Oct 9, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

k8s-ci-robot commented Oct 9, 2021

k8s-ci-robot commented Oct 9, 2021

fengzixu commented Oct 9, 2021

k8s-ci-robot commented Oct 9, 2021

fengzixu commented Oct 9, 2021

fengzixu commented Oct 9, 2021

pacoxu commented Oct 9, 2021

pacoxu commented Oct 9, 2021

fengzixu commented Jan 23, 2022

dgrisonnet commented Jan 26, 2022

xing-yang commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

dashpole commented Mar 9, 2022

gnufied commented Mar 9, 2022

k8s-ci-robot commented Mar 14, 2022

k8s-triage-robot commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

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

dashpole Mar 9, 2022 •

edited

Loading