Fix issue when setting fileysystem capacity in container manager #48567

jingxu97 · 2017-07-06T22:01:15Z

In Container manager, we set up the capacity by retrieving information
from cadvisor. However unlike machineinfo, filesystem information is
available at a later unknown time. This PR uses a go routine to keep
retriving the information until it is avaialble or timeout.
This PR fixes issue #48452

Fix issue with setting filesystem capacity in container manager.

dashpole · 2017-07-06T22:23:54Z

pkg/kubelet/cm/container_manager_linux.go

+	defer timeoutTimer.Stop()
+	for {
+		select {
+		case <-timeoutTimer.C:


Would it be possible to use wait.Until? That would make the code more readable.

changed to use wait.Until, but this also removed timeout

vishh · 2017-07-06T22:40:00Z

pkg/kubelet/cm/container_manager_linux.go

+			glog.Warningf("[ContainerManager] Fail to get rootfs information %v", err)
+			continue
+		}
+		for rName, rCap := range cadvisor.StorageScratchCapacityFromFsInfo(rootfs) {


Don't you want a lock around both scratch and image fs capacity updates? Why not cache values and update them in one go?

@jingxu97 this is yet to be resolved

is the new code ok? Or you prefer with one lock and put two for loops (cadvisor.StorageScratchCapacityFromFsInfo and cadvisor.StorageOverlayCapacityFromFsInfo) inside the lock?

vishh · 2017-07-06T22:40:35Z

pkg/kubelet/cm/container_manager_linux.go

@@ -551,9 +533,55 @@ func (cm *containerManagerImpl) Start(node *v1.Node, activePods ActivePodsFunc)
 		}, 5*time.Minute, wait.NeverStop)
 	}

+	// Local storage filesystem information from `RootFsInfo` and `ImagesFsInfo` is availalbe at a later time
+	// depending on the time when cadvisor manager updates container stats. Therefore use a go routine to keep
+	// retrieving the information until it is avaialble within 5 minutes timeout.


What if the runtimes are down for more than five minutes?

same concern here. I think it's better to keep retrying if they are still nil

removed the timeout. But will this cause any thread leakage?

why would it cause leakage?

I was thinking multiple go routine might running without stops (each time kubelet restarts, a new go rountine for this will be started)

feiskyer · 2017-07-06T23:00:37Z

Cool. The PR should fix #48452.

feiskyer · 2017-07-06T23:01:47Z

pkg/kubelet/cm/container_manager_linux.go

@@ -551,9 +533,55 @@ func (cm *containerManagerImpl) Start(node *v1.Node, activePods ActivePodsFunc)
 		}, 5*time.Minute, wait.NeverStop)
 	}

+	// Local storage filesystem information from `RootFsInfo` and `ImagesFsInfo` is availalbe at a later time


s/availalbe/available

feiskyer · 2017-07-06T23:02:09Z

pkg/kubelet/cm/container_manager_linux.go

@@ -219,30 +219,12 @@ func NewContainerManager(mountUtil mount.Interface, cadvisorInterface cadvisor.I
 	var capacity = v1.ResourceList{}
 	// It is safe to invoke `MachineInfo` on cAdvisor before logically initializing cAdvisor here because
 	// machine info is computed and cached once as part of cAdvisor object creation.
+	// But `RootFsInfo` and `ImagesFsInfo` are not avaialble at this moment so they will be called later during manager starts


s/avaialble/available

jingxu97 · 2017-07-07T21:30:42Z

/test pull-kubernetes-unit

jingxu97 · 2017-07-07T21:32:53Z

/test pull-kubernetes-e2e-gce-etcd3

jingxu97 · 2017-07-07T21:33:10Z

/test pull-kubernetes-federation-e2e-gce

dashpole · 2017-07-07T23:17:52Z

/lgtm

feiskyer · 2017-07-08T10:40:42Z

/release-note-none

vishh · 2017-07-10T23:33:56Z

/lgtm
/approve

k8s-github-robot · 2017-07-10T23:34:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dashpole, jingxu97, vishh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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/kubelet/OWNERS~~ [vishh]

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

In Container manager, we set up the capacity by retrieving information from cadvisor. However unlike machineinfo, filesystem information is available at a later unknown time. This PR uses a go routine to keep retriving the information until it is avaialble or timeout.

jingxu97 · 2017-07-11T17:37:55Z

/test pull-kubernetes-e2e-kops-aws

fejta · 2017-07-11T20:19:14Z

/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2017-07-12T07:10:17Z

Automatic merge from submit-queue (batch tested with PRs 47232, 48625, 48613, 48567, 39173)

feiskyer · 2017-07-13T08:38:50Z

@jingxu97 Should this cherry-pick to v1.7? Seems this is a serious bug fix.

jingxu97 · 2017-07-13T16:48:22Z

Yes, this and #48636 together can fix the local storage allocatable feature

dashpole · 2017-07-25T15:03:29Z

I still see this message occasionally in localStorageAllocatableEviction test logs:
helpers.go:771] Could not find capacity information for resource storage.kubernetes.io/scratch
helpers.go:782] eviction manager: no observation found for eviction signal allocatableNodeFs.available

It only seems to happen every minute or so, so not nearly as often as #48703. Still something we should look into.

dashpole · 2017-07-25T17:41:55Z

@jingxu97 confirmed that the message I found was only during node startup, when capacity information was not yet available.

…67-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #48567 Cherry pick of #48567 on release-1.7. #48567: Fix issue when setting fileysystem capacity in container

k8s-cherrypick-bot · 2017-07-26T11:48:24Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2017

jingxu97 requested review from vishh and dashpole July 6, 2017 22:01

k8s-github-robot assigned feiskyer and dashpole Jul 6, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 6, 2017

dashpole reviewed Jul 6, 2017

View reviewed changes

vishh reviewed Jul 6, 2017

View reviewed changes

feiskyer mentioned this pull request Jul 6, 2017

start cadvisor manager after initialization #48485

Closed

feiskyer reviewed Jul 6, 2017

View reviewed changes

jingxu97 force-pushed the July/getcapacity branch 2 times, most recently from 6360f55 to 9fdba6a Compare July 7, 2017 17:18

jingxu97 mentioned this pull request Jul 7, 2017

Fix issues for local storage allocatable feature #48636

Merged

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

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 7, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 8, 2017

jingxu97 force-pushed the July/getcapacity branch from 9fdba6a to 27b0660 Compare July 10, 2017 22:48

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2017

k8s-ci-robot assigned vishh Jul 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2017

jingxu97 force-pushed the July/getcapacity branch from 27b0660 to 9606a54 Compare July 10, 2017 23:43

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2017

jingxu97 added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jul 10, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2017

k8s-github-robot merged commit dbb4283 into kubernetes:master Jul 12, 2017

jingxu97 added this to the v1.7 milestone Jul 13, 2017

feiskyer mentioned this pull request Jul 19, 2017

storage.kubernetes.io/scratch is always kept 0 #48452

Closed

jingxu97 added the cherrypick-candidate label Jul 25, 2017

jingxu97 mentioned this pull request Jul 25, 2017

Automated cherry pick of #48567 #49578

Merged

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 26, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 26, 2017

xiangpengzhao mentioned this pull request Aug 8, 2017

kubelet-v1.7.0: storage capacity is 0 #50322

Closed

jackfrancis mentioned this pull request Jun 4, 2021

kubelet: more resilient node allocatable ephemeral-storage data getter #101882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue when setting fileysystem capacity in container manager #48567

Fix issue when setting fileysystem capacity in container manager #48567

jingxu97 commented Jul 6, 2017 •

edited by wojtek-t

dashpole Jul 6, 2017

jingxu97 Jul 7, 2017

vishh Jul 6, 2017

vishh Jul 8, 2017

jingxu97 Jul 10, 2017

vishh Jul 6, 2017

feiskyer Jul 6, 2017

jingxu97 Jul 7, 2017

vishh Jul 8, 2017

jingxu97 Jul 10, 2017

feiskyer commented Jul 6, 2017

feiskyer Jul 6, 2017

jingxu97 Jul 7, 2017

feiskyer Jul 6, 2017

jingxu97 Jul 7, 2017

jingxu97 commented Jul 7, 2017

jingxu97 commented Jul 7, 2017

jingxu97 commented Jul 7, 2017

dashpole commented Jul 7, 2017

feiskyer commented Jul 8, 2017

vishh commented Jul 10, 2017

k8s-github-robot commented Jul 10, 2017

jingxu97 commented Jul 11, 2017

fejta commented Jul 11, 2017

k8s-github-robot commented Jul 12, 2017

feiskyer commented Jul 13, 2017

jingxu97 commented Jul 13, 2017

dashpole commented Jul 25, 2017

dashpole commented Jul 25, 2017

k8s-cherrypick-bot commented Jul 26, 2017

Fix issue when setting fileysystem capacity in container manager #48567

Fix issue when setting fileysystem capacity in container manager #48567

Conversation

jingxu97 commented Jul 6, 2017 • edited by wojtek-t

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Jul 6, 2017

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

jingxu97 commented Jul 7, 2017

jingxu97 commented Jul 7, 2017

jingxu97 commented Jul 7, 2017

dashpole commented Jul 7, 2017

feiskyer commented Jul 8, 2017

vishh commented Jul 10, 2017

k8s-github-robot commented Jul 10, 2017

jingxu97 commented Jul 11, 2017

fejta commented Jul 11, 2017

k8s-github-robot commented Jul 12, 2017

feiskyer commented Jul 13, 2017

jingxu97 commented Jul 13, 2017

dashpole commented Jul 25, 2017

dashpole commented Jul 25, 2017

k8s-cherrypick-bot commented Jul 26, 2017

jingxu97 commented Jul 6, 2017 •

edited by wojtek-t