Change eviction manager to manage one single local storage resource #50889

NickrenREN · 2017-08-18T05:34:49Z

What this PR does / why we need it:
We decided to manage one single resource name, eviction policy should be modified too.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): part of #50818

Special notes for your reviewer:

Release note:

Change eviction manager to manage one single local ephemeral storage resource

/assign @jingxu97

NickrenREN · 2017-08-18T05:35:49Z

cc @ddysher

jingxu97 · 2017-08-18T18:31:19Z

pkg/kubelet/eviction/eviction_manager.go

-		m.dedicatedImageFs = &hasImageFs
-		m.resourceToRankFunc = buildResourceToRankFunc(hasImageFs)
-		m.resourceToNodeReclaimFuncs = buildResourceToNodeReclaimFuncs(m.imageGC, m.containerGC, hasImageFs)
+	if len(m.resourceToRankFunc) == 0 || len(m.resourceToNodeReclaimFuncs) == 0 {


I don't think you should change the code here. HasDedicatedImageFs is need to build for resourceToNodeReclaimFunc. Before my change, the eviction manager monitors both nodefs and imagefs usage and perform eviction when there is outofdisk condition. This part should not be changed.

thanks for your review. imagefs is still managed ?

jingxu97 · 2017-08-18T18:51:23Z

pkg/kubelet/eviction/eviction_manager.go

 			evicted = append(evicted, pod)
 		}
 	}

 	return evicted
 }

+func getPodContainersTotalStorageLimits(pod *v1.Pod) resource.Quantity {
+	containersTotalStorageLimits := resource.Quantity{Format: resource.BinarySI}


I think you can resue function PodRequestsAndLimits in helpers.go to get the pod-level resources

jingxu97 · 2017-08-18T18:54:25Z

pkg/kubelet/eviction/eviction_manager.go

 			if used != nil && size.Sign() == 1 && used.Cmp(size) > 0 {
 				// the emptyDir usage exceeds the size limit, evict the pod
 				return m.evictPod(pod, v1.ResourceName("EmptyDir"), fmt.Sprintf("emptyDir usage exceeds the limit %q", size.String()))
 			}
 		}
 	}
+	containersTotalStorageLimits := getPodContainersTotalStorageLimits(pod)
+	if !containersTotalStorageLimits.IsZero() && containersTotalStorageLimits.Cmp(emptyDirTotalUsed) < 0 {


This pod-level check should include all containers usage and emptyDir usage, not just only total emptyDir usage right?
To make it more clear, maybe have a separate function podLimitEviction()..

jingxu97 · 2017-08-18T19:02:41Z

pkg/kubelet/eviction/helpers.go

@@ -1001,26 +987,15 @@ func isHardEvictionThreshold(threshold evictionapi.Threshold) bool {
 }

 // buildResourceToRankFunc returns ranking functions associated with resources
-func buildResourceToRankFunc(withImageFs bool) map[v1.ResourceName]rankFunc {
+func buildResourceToRankFunc() map[v1.ResourceName]rankFunc {


this part should not be changed

jingxu97 · 2017-08-18T19:03:19Z

pkg/kubelet/eviction/helpers.go

@@ -1030,24 +1005,13 @@ func PodIsEvicted(podStatus v1.PodStatus) bool {
 }

 // buildResourceToNodeReclaimFuncs returns reclaim functions associated with resources.
-func buildResourceToNodeReclaimFuncs(imageGC ImageGC, containerGC ContainerGC, withImageFs bool) map[v1.ResourceName]nodeReclaimFuncs {
+func buildResourceToNodeReclaimFuncs(imageGC ImageGC, containerGC ContainerGC) map[v1.ResourceName]nodeReclaimFuncs {


should not be changed

jingxu97 · 2017-08-18T19:03:54Z

pkg/kubelet/eviction/helpers.go

-			} else {
-				storageScratchAllocatable.Sub(usage[resourceDisk])
-			}
+			storageScratchAllocatable.Sub(usage[resourceDisk])


should not be changed. This is for node allocatable feature.

NickrenREN · 2017-08-22T08:50:31Z

/retest

NickrenREN · 2017-08-22T10:02:40Z

/retest

Seems not related to the code changed

jingxu97 · 2017-08-23T21:12:06Z

cmd/kubelet/app/server.go

@@ -742,9 +742,9 @@ func parseResourceList(m kubeletconfiginternal.ConfigurationMap) (v1.ResourceLis
 			if q.Sign() == -1 {
 				return nil, fmt.Errorf("resource quantity for %q cannot be negative: %v", k, v)
 			}
-			// storage specified in configuration map is mapped to ResourceStorageScratch API
+			// storage specified in configuration map is mapped to ResourceEphemeralStorage API
 			if v1.ResourceName(k) == v1.ResourceStorage {


I think we should change this too
if v1.ResourceName(k) == v1.ResourceEphemeralStorage {

jingxu97 · 2017-08-23T21:17:27Z

pkg/kubelet/eviction/helpers.go

+		podVolumeUsed[volume.Name] = resource.NewQuantity(int64(*volume.UsedBytes), resource.BinarySI)
+	}
+	for _, vol := range pod.Spec.Volumes {
+		if vol.EmptyDir != nil && vol.EmptyDir.Medium != v1.StorageMediumMemory && vol.EmptyDir.SizeLimit.Sign() == 1 {


why you need to check vol.EmptyDir.SizeLimit.Sign() == 1 here?

yeah, actually do not need.

jingxu97 · 2017-08-23T21:21:20Z

pkg/kubelet/eviction/eviction_manager.go

-		if overlayThreshold, ok := thresholdsMap[containerStat.Name]; ok {
-			if overlayThreshold.Cmp(*rootfs) < 0 {
-				return m.evictPod(pod, v1.ResourceName("containerOverlay"), fmt.Sprintf("container's overlay usage exceeds the limit %q", overlayThreshold.String()))
+		containerUsed := diskUsage(containerStat.Rootfs)


containerStat.Rootfs could comes from imagefs, and should not be counted here, right?

jingxu97 · 2017-08-23T21:32:04Z

pkg/kubelet/eviction/eviction_manager.go

+
+	podEphemeralStorageTotalUsage := &resource.Quantity{}
+	podEphemeralStorageTotalUsage.Add(*getTotalEmptyDirUsed(podStats, pod))
+	podEphemeralStorageTotalUsage.Add(*getTotalContainerEphemeralStorageUsed(podStats))


To get the total storage usage for a pod, please also check podDiskUsage() in helpers.go

NickrenREN · 2017-08-24T06:55:09Z

/retest

jingxu97 · 2017-08-24T17:43:03Z

cc @dashpole

dashpole

couple of nits. Looks good overall.

dashpole · 2017-08-24T18:53:11Z

pkg/kubelet/eviction/eviction_manager.go

-			if overlayThreshold.Cmp(*rootfs) < 0 {
-				return m.evictPod(pod, v1.ResourceName("containerOverlay"), fmt.Sprintf("container's overlay usage exceeds the limit %q", overlayThreshold.String()))
+		containerUsed := &resource.Quantity{}
+		if *m.dedicatedImageFs {


nit: simplify this statement:
containerUsed := diskUsage(containerStat.Logs)
if !*m.dedicatedImageFs {
containerUsed.Add(*diskUsage(containerStat.Rootfs))
}

Yeah,thanks, done

dashpole · 2017-08-24T18:57:55Z

pkg/kubelet/eviction/eviction_manager.go

+		}
+		if ephemeralStorageThreshold, ok := thresholdsMap[containerStat.Name]; ok {
+			if ephemeralStorageThreshold.Cmp(*containerUsed) < 0 {
+				return m.evictPod(pod, v1.ResourceName("containerEphemeralStorage"), fmt.Sprintf("container's ephemeral local storage usage exceeds the limit %q", ephemeralStorageThreshold.String()))


I dont think we need seperate resource types for pod and container ephemeral storage. I would prefer they both used: ResourceEphemeralStorage.

We can differentiate via the message portion.

sorry for not being specific. I meant v1.RessourceEphemeralStorage, not a new resource type.

dashpole · 2017-08-25T02:56:14Z

pkg/kubelet/eviction/eviction_manager.go

+
+		if ephemeralStorageThreshold, ok := thresholdsMap[containerStat.Name]; ok {
+			if ephemeralStorageThreshold.Cmp(*containerUsed) < 0 {
+				return m.evictPod(pod, v1.ResourceName("ResourceEphemeralStorage"), fmt.Sprintf("container's ephemeral local storage usage exceeds the limit %q", ephemeralStorageThreshold.String()))


Sorry, I wasnt specific. I meant use v1.ResourceEphemeralStorage

My bad. Modified

dashpole

LGTM

NickrenREN · 2017-08-25T03:46:39Z

/retest

jingxu97 · 2017-08-25T06:01:12Z

/lgtm

NickrenREN · 2017-08-25T06:07:38Z

/assign @smarterclayton @thockin

thanks @jingxu97

NickrenREN · 2017-08-25T06:09:11Z

ping @smarterclayton @thockin for approval

thockin · 2017-08-25T06:58:59Z

/approve

jingxu97 · 2017-08-25T16:31:20Z

pkg/kubelet/eviction/eviction_manager.go

+		glog.Errorf("eviction manager: error getting pod disk usage %v", err)
+		return false
+	}
+	podEphemeralStorageTotalUsage.Add(podUsage[resourceDisk])


seem like you can directly use podUsage[resourceDisk] instead of assigning to podEphemeralStorageTotalUsage? But it is also ok

jingxu97 · 2017-08-25T16:35:53Z

pkg/kubelet/eviction/helpers.go

@@ -746,14 +746,14 @@ func makeSignalObservations(summaryProvider stats.SummaryProvider, capacityProvi
 				diskUsageP := &diskUsage


I think here we should use the similar as you did in eviction manager to calculate ephemeralStorageAllocatable. That is more clear

if *m.dedicatedImageFs {

fsStatsSet = []fsStatsType{fsStatsLogs, fsStatsLocalVolumeSource}

} else {

fsStatsSet = []fsStatsType{fsStatsRoot, fsStatsLogs, fsStatsLocalVolumeSource}

}

jingxu97 · 2017-08-25T21:22:35Z

/lgtm

k8s-github-robot · 2017-08-25T21:23:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NickrenREN, jingxu97, thockin

Associated issue: 50818

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

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

~~cmd/kubelet/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~pkg/kubelet/OWNERS~~ [thockin]
~~test/e2e_node/OWNERS~~ [thockin]

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

NickrenREN · 2017-08-25T23:18:04Z

/retest

NickrenREN · 2017-08-26T02:42:25Z

/retest

k8s-github-robot · 2017-08-26T03:44:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

NickrenREN · 2017-08-26T05:03:50Z

/retest

k8s-github-robot · 2017-08-26T05:43:25Z

Automatic merge from submit-queue (batch tested with PRs 50889, 51347, 50582, 51297, 51264)

ddysher · 2017-08-28T01:31:13Z

pkg/kubelet/eviction/eviction_manager.go

+	podEphemeralStorageTotalUsage := &resource.Quantity{}
+	fsStatsSet := []fsStatsType{}
+	if *m.dedicatedImageFs {
+		fsStatsSet = []fsStatsType{fsStatsLogs, fsStatsLocalVolumeSource}


fsStatsLocalVolumeSource also contains hostPath, are we counting host path usage to Pod? e.g. if we limit Pod usage to 500M and the pod mount a hostpath which is about 1Gi, are we gonna evict the pod?

oh, my bad. I did not notice that localVolumeNames contains other volume sources
cc @jingxu97

Thinking further, if hostPath something outside rootfs, e.g. /etc, then do we calculate it at all?

Yes, I think hostapath should not be counted since it is considered as persistent storage. But secrets, configumap, gitrepo, downwardapi should be counted because they are basically a wrapper of emptydir. We might need to have a separate podLocalEphemeralStorageUsage function

ddysher · 2017-08-28T01:43:19Z

pkg/kubelet/eviction/eviction_manager.go

+
+	podEphemeralStorageTotalUsage := &resource.Quantity{}
+	fsStatsSet := []fsStatsType{}
+	if *m.dedicatedImageFs {


sorry if this is already discussed somewhere; i have two questions regarding dedicated image fs:

why do we still count fsStatsLogs if we have dedicated imagefs? IIUC, it is yet to decide how to handle container logs, it's possible that imagefs provides storage for logs?

are we simply ignoring eviction for imagefs? e.g. if i have imagefs setup and a container keeps writing to writable layer, it can use up all storage in imagefs?

We only manage one partition(root) now. If we do not have imagefs partition, then writable layer is satisfied by root partition, we need to calculate its usage, if we have imagefs partition, do not manage it now.

which means this is true? if i have imagefs setup and a container keeps writing to writable layer, it can use up all storage in imagefs?

We only manage one partition(root) now. If we do not have imagefs partition, then writable layer is satisfied by root partition, we need to calculate its usage, if we have imagefs partition, do not manage it now.

@ddysher yes, that is true..

k8s-ci-robot assigned jingxu97 Aug 18, 2017

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

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 18, 2017

jingxu97 reviewed Aug 18, 2017

View reviewed changes

NickrenREN force-pushed the local-storage-eviction branch 2 times, most recently from 38ba12d to ba7fe66 Compare August 22, 2017 07:07

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 22, 2017

NickrenREN force-pushed the local-storage-eviction branch from ba7fe66 to 6a4a6e0 Compare August 22, 2017 07:58

NickrenREN force-pushed the local-storage-eviction branch from 6a4a6e0 to 6d4d9e2 Compare August 23, 2017 05:32

jingxu97 reviewed Aug 23, 2017

View reviewed changes

NickrenREN force-pushed the local-storage-eviction branch from 6d4d9e2 to fc0eef9 Compare August 24, 2017 04:45

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 24, 2017

NickrenREN force-pushed the local-storage-eviction branch from fc0eef9 to e1ee299 Compare August 24, 2017 04:56

NickrenREN force-pushed the local-storage-eviction branch from e1ee299 to c7d9c67 Compare August 24, 2017 07:25

jingxu97 requested a review from dashpole August 24, 2017 17:42

dashpole suggested changes Aug 24, 2017

View reviewed changes

dashpole reviewed Aug 25, 2017

View reviewed changes

NickrenREN force-pushed the local-storage-eviction branch from b438abc to 810008a Compare August 25, 2017 03:03

dashpole approved these changes Aug 25, 2017

View reviewed changes

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

k8s-ci-robot assigned smarterclayton and thockin Aug 25, 2017

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

jingxu97 reviewed Aug 25, 2017

View reviewed changes

jingxu97 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2017

NickrenREN added 2 commits August 26, 2017 05:14

Change eviction policy to manage one single local storage resource

27901ad

Change validation for local ephemeral storage

9730e3d

NickrenREN force-pushed the local-storage-eviction branch from 810008a to 9730e3d Compare August 25, 2017 21:15

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

k8s-github-robot merged commit 76c520c into kubernetes:master Aug 26, 2017

NickrenREN deleted the local-storage-eviction branch August 26, 2017 06:09

ddysher reviewed Aug 28, 2017

View reviewed changes

NickrenREN mentioned this pull request Aug 29, 2017

Pod local ephemeral storage usage bug #51489

Closed

		@@ -746,14 +746,14 @@ func makeSignalObservations(summaryProvider stats.SummaryProvider, capacityProvi
		diskUsageP := &diskUsage

Change eviction manager to manage one single local storage resource #50889

Change eviction manager to manage one single local storage resource #50889

Conversation

NickrenREN commented Aug 18, 2017 • edited Loading

NickrenREN commented Aug 18, 2017

Choose a reason for hiding this comment

NickrenREN Aug 19, 2017 • edited Loading

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

NickrenREN commented Aug 22, 2017

NickrenREN commented Aug 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN commented Aug 24, 2017

jingxu97 commented Aug 24, 2017

dashpole 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole left a comment

Choose a reason for hiding this comment

NickrenREN commented Aug 25, 2017

jingxu97 commented Aug 25, 2017

NickrenREN commented Aug 25, 2017 • edited Loading

NickrenREN commented Aug 25, 2017

thockin commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

NickrenREN commented Aug 25, 2017

NickrenREN commented Aug 26, 2017

k8s-github-robot commented Aug 26, 2017

NickrenREN commented Aug 26, 2017

k8s-github-robot commented Aug 26, 2017

Choose a reason for hiding this comment

NickrenREN Aug 28, 2017 • edited Loading

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

NickrenREN commented Aug 18, 2017 •

edited

Loading

NickrenREN Aug 19, 2017 •

edited

Loading

NickrenREN commented Aug 25, 2017 •

edited

Loading

NickrenREN Aug 28, 2017 •

edited

Loading