Add local storage (scratch space) allocatable support #46456

jingxu97 · 2017-05-25T19:41:36Z

This PR adds the support for allocatable local storage (scratch space).
This feature is only for root file system which is shared by kubernetes
componenets, users' containers and/or images. User could use
--kube-reserved flag to reserve the storage for kube system components.
If the allocatable storage for user's pods is used up, some pods will be
evicted to free the storage resource.

This feature is part of local storage capacity isolation and described in the proposal kubernetes/community#306

Release note:

This feature exposes local storage capacity for the primary partitions, and supports & enforces storage reservation in Node Allocatable

dashpole

First pass. I'll look into what changes to machineInfo it would take to be able to determine rootfs capacity.

dashpole · 2017-05-26T00:35:10Z

pkg/kubelet/cm/node_container_manager.go

 func (cm *containerManagerImpl) GetNodeAllocatableReservation() v1.ResourceList {
 	evictionReservation := hardEvictionReservation(cm.HardEvictionThresholds, cm.capacity)
+	if !hasStorageInfoInCapacity(cm.capacity) {


you dont need a helper function here, as it is just one line:
if _, ok := cm.capacity[v1.ResourceStorage]; !ok {

dashpole · 2017-05-26T00:38:24Z

pkg/kubelet/eviction/api/types.go

@@ -38,6 +38,8 @@ const (
 	SignalImageFsInodesFree Signal = "imagefs.inodesFree"
 	// SignalAllocatableMemoryAvailable is amount of memory available for pod allocation (i.e. allocatable - workingSet (of pods), in bytes.
 	SignalAllocatableMemoryAvailable Signal = "allocatableMemory.available"
+	// SignalAllocatableStorageScratchAvailable is amount of storage available for pod allocation
+	SignalAllocatableStorageScratchAvailable Signal = "allocatableStorageScratch.available"


While we have begun calling rootfs "Scratch" space, it would probably be easier to read the code if it was named similarly to the Node-Level signal. Something like SignalAllocatableNodeFsAvailable.

dashpole · 2017-05-26T00:40:57Z

pkg/kubelet/eviction/eviction_manager.go

 	// build the ranking functions (if not yet known)
 	// TODO: have a function in cadvisor that lets us know if global housekeeping has completed
 	if len(m.resourceToRankFunc) == 0 || len(m.resourceToNodeReclaimFuncs) == 0 {
 		// this may error if cadvisor has yet to complete housekeeping, so we will just try again in next pass.
-		hasDedicatedImageFs, err := diskInfoProvider.HasDedicatedImageFs()
-		if err != nil {
+		if errImage != nil {


should we move the error check next to the call? Why only check for errors inside the if statement?

From the existing code, I think the logic is if resourceToRankFunc and resourceToNodeReclaimFuncs are already defined, it does not matter whether errImage returns nil or not.
I modified the code here a little more. Instead of keeping calling HasDedicatedImageFs in every sync call, I put it in a map. PTAL

dashpole · 2017-05-26T00:42:31Z

pkg/kubelet/eviction/eviction_manager.go

@@ -391,10 +391,13 @@ func (m *managerImpl) reclaimNodeLevelResources(resourceToReclaim v1.ResourceNam
 			glog.Errorf("eviction manager: unable to find value associated with signal %v", signal)
 			continue
 		}
-		value.available.Add(*reclaimed)
+		if reclaimed != nil {


We discussed this offline. reclaimed cannot be nil here.

dashpole · 2017-05-26T00:44:04Z

pkg/kubelet/eviction/eviction_manager.go


 		// evaluate all current thresholds to see if with adjusted observations, we think we have met min reclaim goals
 		if len(thresholdsMet(m.thresholdsMet, observations, true)) == 0 {
+			glog.V(3).Infof("eviction manager: reclaim goal reached for resource %v: ", resourceToReclaim)


We already log something in synchronize on line 311 if we are able to reclaim enough resources. This is not needed.

dashpole · 2017-05-26T00:46:18Z

pkg/kubelet/eviction/helpers.go

@@ -83,6 +86,7 @@ func init() {
 	signalToResource[evictionapi.SignalImageFsInodesFree] = resourceImageFsInodes
 	signalToResource[evictionapi.SignalNodeFsAvailable] = resourceNodeFs
 	signalToResource[evictionapi.SignalNodeFsInodesFree] = resourceNodeFsInodes
+	signalToResource[evictionapi.SignalAllocatableStorageScratchAvailable] = resourceNodeFs
 	resourceToSignal = map[v1.ResourceName]evictionapi.Signal{}


are you still planning to remove resourceToSignal?

Yes, I will do it in a separate PR, basically changing from 1-1 mapping from resource to signal, we use map 1-many with a list of signals

dashpole · 2017-05-26T00:57:39Z

test/e2e_node/localStorageAllocatable_eviction_test.go

+}
+
+// Pass podTestSpecsP as references so that it could be set up in the first BeforeEach clause
+func runLocalStorageEvictionTest(f *framework.Framework, conditionType v1.NodeConditionType, testCondition string, podTestSpecsP *[]podTestSpec, evictionTestTimeout time.Duration,


can you reuse the version in the inode eviction test suite? If not, can we refactor them to avoid duplication?

I tried a few things, only by passing the reference works for me. I plan to refactor this code in a separate PR, is that ok?

sure, that can happen after code freeze, if neccessary.

dashpole · 2017-05-26T00:59:53Z

pkg/kubelet/kubelet_cadvisor.go

@@ -100,3 +100,15 @@ func (kl *Kubelet) GetCachedMachineInfo() (*cadvisorapi.MachineInfo, error) {
 	}
 	return kl.machineInfo, nil
 }
+
+// GetCachedRootFsInfo assumes that the rootfs info can't change without a reboot
+func (kl *Kubelet) GetCachedRootFsInfo() (cadvisorapiv2.FsInfo, error) {


My thinking right now is that it would make more sense to change cAdvisor to include the information we need in MachineInfo. IIRC, the only piece we are missing is the mountpoint for the disks, which would let us determine which filesystem is the RootFs. I am happy to look into this in the next couple days to see if it is feasible. That would greatly simplify this PR.

thanks! I will check that too, but we might also need to add this after code freeze.

Yep, that is fine.

derekwaynecarr

requested a few changes.

derekwaynecarr · 2017-05-31T17:45:26Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

+	//Account for storage requested by emptydir volumes
+	for _, vol := range pod.Spec.Volumes {
+		if vol.EmptyDir != nil {
+			result.StorageScratch += vol.EmptyDir.SizeLimit.Value()


this needs to exclude empty dir that is backed by memory.

derekwaynecarr · 2017-05-31T17:46:27Z

plugin/pkg/scheduler/algorithm/predicates/predicates_test.go

+						Name: "emptyDirVolumeName",
+						VolumeSource: v1.VolumeSource{
+							EmptyDir: &v1.EmptyDirVolumeSource{
+								SizeLimit: *resource.NewQuantity(scratch, resource.BinarySI),


set medium to not memory

derekwaynecarr · 2017-05-31T17:46:55Z

plugin/pkg/scheduler/algorithm/predicates/predicates_test.go

+		reasons  []algorithm.PredicateFailureReason
+	}{
+		{
+			pod: newResourcePod(schedulercache.Resource{MilliCPU: 1, Memory: 1, StorageOverlay: 1}),


add a test for memory backed empty dir.

added a test for memory medium

derekwaynecarr · 2017-05-31T17:47:35Z

plugin/pkg/scheduler/schedulercache/node_info.go

+	//Account for storage requested by emptydir volumes
+	for _, vol := range pod.Spec.Volumes {
+		if vol.EmptyDir != nil {
+			res.StorageScratch += vol.EmptyDir.SizeLimit.Value()


it would be good if we have a utility functoin for this as this loop is repeated in multiple places.

also need to ignore medium memory empty dir.

fixed. this loop only appeared in predicates.go and node_info.go, could we consider add a utility if more places need it?

I'd imagine tests also using this utility function.

jingxu97 · 2017-05-31T21:52:21Z

addressed the comments. @derekwaynecarr @dashpole PTAL

dashpole · 2017-05-31T22:19:05Z

eviction manager changes lgtm

vishh · 2017-05-31T22:23:19Z

test/e2e_node/localStorageAllocatable_eviction_test.go

@@ -0,0 +1,322 @@
+/*
+Copyright 2016 The Kubernetes Authors.


This is an odd file name. I think go recommends using _ instead of camelcase

I changed the file name

vishh · 2017-05-31T22:26:13Z

pkg/kubelet/kubelet_node_status.go

+	if hasDedicatedImageFs, err := kl.HasDedicatedImageFs(); err != nil && hasDedicatedImageFs {
+		imagesfs, err := kl.ImagesFsInfo()
+		if err != nil {
+			node.Status.Capacity[v1.ResourceStorageOverlay] = resource.MustParse("0Gi")


Shouldn't we fail kubelet in this case? Why would kubelet lie that imagefs doesn't exist when it fails to collect stats about it?

I followed the logic in GetCachedMachineInfo. Should we use different behavior here?

davidopp · 2017-06-01T20:56:44Z

/approve

k8s-github-robot · 2017-06-01T20:57:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,vishh]
~~pkg/apis/componentconfig/OWNERS~~ [thockin,vishh]
~~pkg/kubelet/OWNERS~~ [thockin,vishh]
~~plugin/pkg/scheduler/OWNERS~~ [davidopp,thockin]
~~test/e2e_node/OWNERS~~ [thockin,vishh]

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

fejta · 2017-06-01T21:04:31Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref kubernetes/test-infra#2931

jingxu97 · 2017-06-01T22:23:37Z

@k8s-bot pull-kubernetes-unit test this

This PR adds the support for allocatable local storage (scratch space). This feature is only for root file system which is shared by kubernetes componenets, users' containers and/or images. User could use --kube-reserved flag to reserve the storage for kube system components. If the allocatable storage for user's pods is used up, some pods will be evicted to free the storage resource.

This PR adds the check for local storage request when admitting pods. If the local storage request exceeds the available resource, pod will be rejected.

jingxu97 · 2017-06-01T22:59:11Z

@k8s-bot pull-kubernetes-unit test this

jingxu97 · 2017-06-01T23:53:35Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

k8s-github-robot · 2017-06-03T07:24:29Z

Automatic merge from submit-queue

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

jingxu97 assigned dashpole and derekwaynecarr May 25, 2017

jingxu97 requested review from vishh and dashpole May 25, 2017 19:42

jingxu97 unassigned derekwaynecarr May 25, 2017

jingxu97 requested review from dashpole and derekwaynecarr and removed request for dashpole May 25, 2017 19:42

dashpole suggested changes May 26, 2017

View reviewed changes

jingxu97 force-pushed the May/allocatable branch from 4f2a0d9 to d509806 Compare May 31, 2017 17:11

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 31, 2017

jingxu97 force-pushed the May/allocatable branch from d509806 to fb56070 Compare May 31, 2017 17:15

derekwaynecarr suggested changes May 31, 2017

View reviewed changes

jingxu97 force-pushed the May/allocatable branch 5 times, most recently from f0e5ded to 0235873 Compare May 31, 2017 21:51

jingxu97 force-pushed the May/allocatable branch from 0235873 to ca45ed2 Compare May 31, 2017 21:56

vishh reviewed May 31, 2017

View reviewed changes

dashpole mentioned this pull request May 31, 2017

Add EmptyDir volume and container overlay capacity isolation #45686

Merged

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

jingxu97 force-pushed the May/allocatable branch from 0195c58 to ea56c35 Compare June 1, 2017 18:52

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

jingxu97 force-pushed the May/allocatable branch from ea56c35 to 9606bd4 Compare June 1, 2017 18:54

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

jingxu97 force-pushed the May/allocatable branch 2 times, most recently from 491253e to 19c5b3c Compare June 1, 2017 19:26

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

jingxu97 force-pushed the May/allocatable branch from 19c5b3c to c4ab895 Compare June 1, 2017 19:51

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

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

jingxu97 force-pushed the May/allocatable branch from c4ab895 to c6f374e Compare June 1, 2017 21:55

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

jingxu97 added 2 commits June 1, 2017 15:57

Add predicates check for local storage request

943fc53

This PR adds the check for local storage request when admitting pods. If the local storage request exceeds the available resource, pod will be rejected.

jingxu97 force-pushed the May/allocatable branch from c6f374e to 943fc53 Compare June 1, 2017 22:57

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017

vishh approved these changes Jun 2, 2017

View reviewed changes

vishh mentioned this pull request Jun 2, 2017

[Storage] Add support for capacity isolation of shared partitions #43607

Open

19 tasks

k8s-github-robot merged commit b8c9ee8 into kubernetes:master Jun 3, 2017

ddysher mentioned this pull request Jun 9, 2017

Fix local isolation for pod requesting only overlay or scratch #47179

Merged

vishh mentioned this pull request Jun 20, 2017

Storage Allocatable does not take eviction thresholds into account #47808

Closed

yujuhong mentioned this pull request Jun 26, 2017

kubelet race condition: concurrent map writes #48045

Closed

Add local storage (scratch space) allocatable support #46456

Add local storage (scratch space) allocatable support #46456

Conversation

jingxu97 commented May 25, 2017 • edited Loading

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

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

derekwaynecarr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented May 31, 2017 • edited Loading

dashpole commented May 31, 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

davidopp commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

fejta commented Jun 1, 2017

jingxu97 commented Jun 1, 2017

jingxu97 commented Jun 1, 2017

jingxu97 commented Jun 1, 2017

k8s-github-robot commented Jun 3, 2017

jingxu97 commented May 25, 2017 •

edited

Loading

jingxu97 commented May 31, 2017 •

edited

Loading