fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) #96675

pacoxu · 2020-11-18T12:11:24Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/

--volume-stats-agg-period duration Default: 1m0s

| Specifies interval for kubelet to calculate and cache the volume disk usage for all pods and volumes. To disable volume calculations, set to 0. (DEPRECATED: This parameter should be set via the config file specified by the Kubelet's --config flag. See https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/ for more information.)

the default value of volume-stats-agg-period is 1m0s
However when it is set to be 0, it means disable.

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/server/stats/fs_resource_analyzer.go#L60-62

nil means default, 1m0s.
0 means disable

I tested it. It will be disabled only if we set it to be '-1s'

I tried to fix it but found that the metav1.Duration default value is zeroDuration, and it cannot decide whether it is 0s or none(not set, which should use default value).

Current logic: 1m0s will be used even you set it to 0s or 0.
So I fix it as a doc issue.

Which issue(s) this PR fixes:

Fixes #96674

Special notes for your reviewer:
I tested it. It will be disabled only if we set it to be '-1s'

Cause:

kubernetes/cmd/kubelet/app/options/options.go

Line 484 in 71331d8

fs.DurationVar(&c.VolumeStatsAggPeriod.Duration, "volume-stats-agg-period", c.VolumeStatsAggPeriod.Duration, "Specifies interval for kubelet to calculate and cache the volume disk usage for all pods and volumes. To disable volume calculations, set to 0.")
    value, _ := time.ParseDuration("")
    value0, _ := time.ParseDuration("0s")
    fmt.Println(value == value0)
flag use ParseDuration and DurationVar which cannot distinguish no input and 0s.

Does this PR introduce a user-facing change?:

Set kubelet option `--volume-stats-agg-period` to negative value to disable volume calculations.

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

None

It is very confused to make it different for none and 0. So I think doc fix would be a better way IMO.

…disable when it is negative like -1s. Signed-off-by: pacoxu <paco.xu@daocloud.io>

pacoxu · 2020-12-07T05:17:18Z

@kubernetes/sig-node-pr-reviews

This should be cherry-picked to 1.19 or older versions as well.

pacoxu · 2020-12-21T01:39:36Z

@derekwaynecarr Have you had a chance to look at this?

pacoxu · 2021-01-06T03:31:27Z

/triage accepted

ehashman · 2021-01-06T20:14:57Z

I'm a bit confused. In the linked code in the first comment, it should be disabled for 0 or any negative number (logic is x <= 0, has not been changed in 5 years):

kubernetes/pkg/kubelet/server/stats/fs_resource_analyzer.go

Lines 58 to 63 in e456b45

    
           func (s *fsResourceAnalyzer) Start() { 
        
           	s.startOnce.Do(func() { 
        
           		if s.calcPeriod <= 0 { 
        
           			klog.Info("Volume stats collection disabled.") 
        
           			return 
        
           		}

Where is the = 0 case getting overridden to a default? According to #96674 (comment) the behaviour is different, but the linked code doesn't suggest it would be. cc @pacoxu

ehashman · 2021-01-06T20:15:47Z

/priority important-longterm

pacoxu · 2021-01-07T01:43:44Z

I'm a bit confused. In the linked code in the first comment, it should be disabled for 0 or any negative number (logic is x <= 0, has not been changed in 5 years):

kubernetes/pkg/kubelet/server/stats/fs_resource_analyzer.go

Lines 58 to 63 in e456b45

func (s *fsResourceAnalyzer) Start() {

s.startOnce.Do(func() {

if s.calcPeriod <= 0 {

klog.Info("Volume stats collection disabled.")

return

}

Where is the = 0 case getting overridden to a default? According to #96674 (comment) the behaviour is different, but the linked code doesn't suggest it would be. cc @pacoxu

kubernetes/cmd/kubelet/app/options/options.go

Line 484 in 71331d8

    
           fs.DurationVar(&c.VolumeStatsAggPeriod.Duration, "volume-stats-agg-period", c.VolumeStatsAggPeriod.Duration, "Specifies interval for kubelet to calculate and cache the volume disk usage for all pods and volumes.  To disable volume calculations, set to 0.")

@ehashman 0s will be treated as default 1 minute indeed.
Before running into fs_resource_analyzer.go, the value was already converted to 1m.

ehashman · 2021-01-07T20:56:42Z

Okay I found where this gets overridden from 0 to 1m:

kubernetes/pkg/kubelet/apis/config/v1beta1/defaults.go

Lines 141 to 143 in 5150d2f

    
           if obj.VolumeStatsAggPeriod == zeroDuration { 
        
           	obj.VolumeStatsAggPeriod = metav1.Duration{Duration: time.Minute} 
        
           }

/lgtm

mrunalp

/approve

k8s-ci-robot · 2021-01-20T04:58:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, pacoxu

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:

~~cmd/kubelet/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-ci-robot requested review from dchen1107 and derekwaynecarr November 18, 2020 12:12

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. area/kubelet and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 18, 2020

pacoxu changed the title ~~volume stats agg period should be disabled when value is 0~~ Doc error: volume stats agg period should be disabled when value is negative Nov 19, 2020

pacoxu force-pushed the fix/volume-stats-agg-period branch from 886fd50 to 952e964 Compare November 19, 2020 08:31

pacoxu changed the title ~~Doc error: volume stats agg period should be disabled when value is negative~~ bug or doc error: volume stats agg period should be disabled when value is negative(not 0) Nov 19, 2020

k8s-ci-robot assigned yujuhong Nov 19, 2020

pacoxu mentioned this pull request Nov 23, 2020

volume stats agg period should be disabled when value is negative kubernetes/website#25182

Closed

pacoxu changed the title ~~bug or doc error: volume stats agg period should be disabled when value is negative(not 0)~~ doc error: volume stats agg period should be disabled when value is negative(not 0) Nov 24, 2020

pacoxu force-pushed the fix/volume-stats-agg-period branch from 952e964 to fbcf1e3 Compare November 27, 2020 05:56

pacoxu changed the title ~~doc error: volume stats agg period should be disabled when value is negative(not 0)~~ volume stats agg period should be disabled when value is 0, and default value is 1m Nov 27, 2020

pacoxu force-pushed the fix/volume-stats-agg-period branch from fbcf1e3 to 475b751 Compare November 27, 2020 07:07

This comment has been minimized.

Sign in to view

pacoxu changed the title ~~volume stats agg period should be disabled when value is 0, and default value is 1m~~ wip： volume stats disabled when value is 0, and default value is 1m Nov 27, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2020

pacoxu changed the title ~~wip： volume stats disabled when value is 0, and default value is 1m~~ Comments/Doc fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) Dec 7, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2020

pacoxu force-pushed the fix/volume-stats-agg-period branch from 475b751 to 5ab8207 Compare December 7, 2020 03:41

fix doc/comments error: volume stats agg period default value 1m and …

4901855

…disable when it is negative like -1s. Signed-off-by: pacoxu <paco.xu@daocloud.io>

pacoxu force-pushed the fix/volume-stats-agg-period branch from 5ab8207 to 4901855 Compare December 7, 2020 03:47

k8s-ci-robot assigned Random-Liu Dec 15, 2020

pacoxu changed the title ~~Comments/Doc fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m)~~ fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) Dec 21, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 4, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 6, 2021

ehashman added this to Needs Reviewer in SIG Node PR Triage Jan 6, 2021

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 6, 2021

k8s-ci-robot assigned ehashman Jan 7, 2021

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

ehashman moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Jan 7, 2021

This comment has been minimized.

Sign in to view

ehashman mentioned this pull request Jan 13, 2021

Request: Add ehashman as sig-node-reviewer #98036

Merged

13 tasks

mrunalp approved these changes Jan 20, 2021

View reviewed changes

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

k8s-ci-robot merged commit 0da0d18 into kubernetes:master Jan 20, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Jan 20, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 20, 2021

github-actions bot mentioned this pull request Jan 26, 2021

Week Ending January 24, 2021 dev-obs/actus#329

Open

ehashman mentioned this pull request Feb 11, 2021

fix kubelet flag description: --event-qps and --event-burst #98869

Merged

pacoxu deleted the fix/volume-stats-agg-period branch June 23, 2021 05:35

pacoxu mentioned this pull request Sep 15, 2021

Improve unit tests for Kubelet configuration APIs #105029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) #96675

fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) #96675

pacoxu commented Nov 18, 2020 •

edited

This comment has been minimized.

pacoxu commented Dec 7, 2020 •

edited

pacoxu commented Dec 21, 2020

pacoxu commented Jan 6, 2021

ehashman commented Jan 6, 2021

ehashman commented Jan 6, 2021

pacoxu commented Jan 7, 2021 •

edited

ehashman commented Jan 7, 2021

This comment has been minimized.

mrunalp left a comment

k8s-ci-robot commented Jan 20, 2021

fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) #96675

fix: volume stats disabled when value is negative.(Currently, 0 means defualt 1m) #96675

Conversation

pacoxu commented Nov 18, 2020 • edited

--volume-stats-agg-period duration Default: 1m0s

This comment has been minimized.

pacoxu commented Dec 7, 2020 • edited

pacoxu commented Dec 21, 2020

pacoxu commented Jan 6, 2021

ehashman commented Jan 6, 2021

ehashman commented Jan 6, 2021

pacoxu commented Jan 7, 2021 • edited

ehashman commented Jan 7, 2021

This comment has been minimized.

mrunalp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 20, 2021

pacoxu commented Nov 18, 2020 •

edited

--volume-stats-agg-period duration Default: `1m0s`

pacoxu commented Dec 7, 2020 •

edited

pacoxu commented Jan 7, 2021 •

edited