Implement dynamic volume limits #64154

gnufied · 2018-05-22T14:35:57Z

Implement dynamic volume limits depending on node type.

xref kubernetes/community#2051

Add Alpha support for dynamic volume limits based on node type

gnufied · 2018-05-24T02:46:16Z

/retest

jsafrane · 2018-05-24T11:56:13Z

pkg/features/kube_features.go

+	//
+	// Add support for volume plugins to report node specific
+	// volume limits
+	DynamicVolumeThreshold utilfeature.Feature = "DynamicVolumeThreshold"


I am not sure about the feature name. DynamicVolumeLimits? AttachableVolumeLimits (= the predicate name)? There is no "Threshold" used in implementation of the feature.

Limit is basically another word for threshold right? I just copied what was in specs. It should be pretty easy to rename the feature flag but I don't feel very strongly about this either ways.

Since we use "Limit" in other places, I will vote for "limit". Also agree on "AttachableVolume" part

jsafrane · 2018-05-24T11:58:41Z

pkg/scheduler/algorithm/predicates/predicates.go

+
+	// a map of volume unique name and volume limit key
+	newVolumes := make(map[string]string)
+	if err := c.filterAttachableVolumes(pod.Spec.Volumes, pod.Namespace, newVolumes); err != nil {


newVolumes, err = c.filterAttachableVolumes(pod.Spec.Volumes, pod.Namespace) ?

ah, now I know why it is written like that. It is because second execution of this function(line#452) adds volumes in a loop, we will have merge maps and iterate result again to do that. This saves some execution time.

jsafrane · 2018-05-24T12:09:17Z

pkg/volume/aws_ebs/aws_ebs.go

+
+	instanceType, err := instances.InstanceType(context.TODO(), plugin.host.GetNodeName())
+	if err != nil {
+		glog.V(3).Infof("Failed to get instance type from AWS cloud provider")


log the error

jsafrane · 2018-05-24T12:15:45Z

pkg/volume/plugins.go

+// volumes that can be attached to a node.
+type VolumePluginWithAttachLimits interface {
+	VolumePlugin
+	// Return number of volumes attachable on a node from the plugin


Please add some description what is the return map - what's the key and what's value.

I'd emphasize that it's called by kubelet and it returns limit for the node where this function is called.

jsafrane · 2018-05-24T12:17:19Z

pkg/volume/plugins.go

+	//    - storage-limits-aws-ebs
+	//    - storage-limits-csi-cinder
+	// The key should respect character limit of ResourceName type
+	VolumeLimitKey(spec *Spec) string


Add a note that this function is called by scheduler (or can be called by any Kubernetes component, not just kubelet).

derekwaynecarr · 2018-05-24T15:07:26Z

i will help review kubelet changes.

/assign @derekwaynecarr

jingxu97 · 2018-05-24T17:43:55Z

pkg/scheduler/algorithm/predicates/predicates.go

@@ -415,6 +415,136 @@ func (c *MaxPDVolumeCountChecker) filterVolumes(volumes []v1.Volume, namespace s
 	return nil
 }

+func (c *MaxPDVolumeCountChecker) attachableLimitPredicate(


Add comment for this function?

jingxu97 · 2018-05-24T18:09:16Z

cc @bsalamat Could you please help take a look at the scheduler change? Thanks!

gnufied · 2018-05-24T19:01:28Z

For scheduler

/assign @bsalamat @aveshagarwal

for api and other misc. changes

/assign @liggitt

gnufied · 2018-05-24T20:20:52Z

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-kops-aws

msau42 · 2018-06-01T20:54:11Z

/lgtm

k8s-github-robot · 2018-06-01T20:54:50Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@aveshagarwal @bsalamat @derekwaynecarr @gnufied @liggitt @msau42

Pull Request Labels

sig/scheduling: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/feature: New functionality.

Help

gnufied · 2018-06-01T22:26:39Z

/test pull-kubernetes-e2e-gce

bsalamat

Scheduler changes look good to me. Just a minor comment.

bsalamat · 2018-06-01T22:42:29Z

pkg/apis/core/v1/helper/helpers.go

@@ -92,10 +92,14 @@ func IsOvercommitAllowed(name v1.ResourceName) bool {
 		!IsHugePageResourceName(name)
 }

+func IsStorageLimitResourceName(name v1.ResourceName) bool {


I would rename this to IsVolumeLimitResourceName to be consistent with the rest of the code.

define alpha feature and make api changes

Add tests for checking node limits

msau42 · 2018-06-01T23:28:29Z

/lgtm

bsalamat

/lgtm

for scheduler changes

k8s-ci-robot · 2018-06-01T23:58:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, derekwaynecarr, gnufied, liggitt, msau42, saad-ali

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/apis/core/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [bsalamat,derekwaynecarr,liggitt,saad-ali]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr]
~~pkg/scheduler/OWNERS~~ [bsalamat]
~~pkg/volume/OWNERS~~ [gnufied,saad-ali]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

fejta-bot · 2018-06-02T02:29:38Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-06-02T13:30:17Z

Automatic merge from submit-queue (batch tested with PRs 64613, 64596, 64573, 64154, 64639). If you want to cherry-pick this change to another branch, please follow the instructions here.

andyzhangx · 2018-06-04T08:50:13Z

pkg/volume/azure_dd/azure_dd.go

+		return nil, fmt.Errorf("Expected Azure cloudprovider, got %s", cloud.ProviderName())
+	}
+
+	volumeLimits := map[string]int64{


Question: @gnufied it looks like I need to write code here to query volume limits according to azure node type , and assign new value to volumeLimits in azure_dd.go, right?

yes that is correct. I did not know how we can get those limits for Azure Disk and hence I went with existing defaults.

yujuhong · 2018-06-04T17:53:02Z

This broke the node e2e alpha suite: https://k8s-testgrid.appspot.com/sig-node-kubelet#kubelet-alpha

Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: E0602 14:36:19.366284    1727 runtime.go:66] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:72
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:65
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:51
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /usr/local/go/src/runtime/asm_amd64.s:573
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /usr/local/go/src/runtime/panic.go:502
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /usr/local/go/src/runtime/panic.go:63
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /usr/local/go/src/runtime/signal_unix.go:388
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/azure_dd/azure_dd.go:116
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:341
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:779
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:1117
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:1111
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:1099
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:324
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:73
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.go:362
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet.go:1367
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88
Jun 02 14:36:19 tmp-node-e2e-53507b65-cos-stable-63-10032-71-0 kubelet[1727]: /usr/local/go/src/runtime/asm_amd64.s:2361

gnufied · 2018-06-04T17:57:56Z

@yuanying looking. ty

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 22, 2018

k8s-ci-robot requested review from caesarxuchao and davidopp May 22, 2018 14:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels May 22, 2018

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

gnufied force-pushed the impelemnt-volume-count branch 4 times, most recently from 3676b0c to 2ec50f1 Compare May 23, 2018 20:24

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 23, 2018

gnufied changed the title ~~Impelemnt dynamic volume limits~~ Implement dynamic volume limits May 23, 2018

jsafrane reviewed May 24, 2018

View reviewed changes

gnufied force-pushed the impelemnt-volume-count branch 2 times, most recently from 6da2ccb to 9a790ce Compare May 24, 2018 14:54

k8s-ci-robot assigned derekwaynecarr May 24, 2018

gnufied force-pushed the impelemnt-volume-count branch from 9a790ce to 75576df Compare May 24, 2018 15:38

jingxu97 reviewed May 24, 2018

View reviewed changes

gnufied force-pushed the impelemnt-volume-count branch 2 times, most recently from 9fb7a39 to a5330e3 Compare May 24, 2018 18:52

k8s-ci-robot assigned aveshagarwal, bsalamat and liggitt May 24, 2018

gnufied force-pushed the impelemnt-volume-count branch from efe08a7 to b660bb3 Compare June 1, 2018 19:41

k8s-ci-robot assigned msau42 Jun 1, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018

bsalamat reviewed Jun 1, 2018

View reviewed changes

bsalamat approved these changes Jun 1, 2018

View reviewed changes

gnufied added 4 commits June 1, 2018 19:17

Implement API changes needed for dynamic volume limits

45fb31e

define alpha feature and make api changes

Implement kubelet side changes for writing volume limit to node

1f9404d

Add tests for checking node limits

Implement scheduler changes for volume limits

a0a9ccf

Implement volume plugin changes for volume limits

cf28220

gnufied force-pushed the impelemnt-volume-count branch from b660bb3 to 1f9404d Compare June 1, 2018 23:17

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018

bsalamat reviewed Jun 1, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2018

k8s-github-robot merged commit e5686a3 into kubernetes:master Jun 2, 2018

andyzhangx reviewed Jun 4, 2018

View reviewed changes

yujuhong mentioned this pull request Jun 4, 2018

Node e2e alpha test suite is broken #64704

Closed

This was referenced Jun 7, 2018

add generic node resource capacity and allocatable kubernetes/kube-state-metrics#466

Merged

node collector should expose dynamic volume limits kubernetes/kube-state-metrics#474

Closed

tengqm mentioned this pull request Jun 17, 2018

Docs needed for limiting attachable volumes kubernetes/website#9105

Closed

1 task

andyzhangx mentioned this pull request Sep 3, 2018

Enable dynamic configue max number of PDs allowed on a node based on machine type #53461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dynamic volume limits #64154

Implement dynamic volume limits #64154

gnufied commented May 22, 2018 •

edited

Loading

gnufied commented May 24, 2018

jsafrane May 24, 2018

gnufied May 24, 2018

jingxu97 May 24, 2018

jsafrane May 24, 2018

gnufied May 24, 2018

jsafrane May 24, 2018

jsafrane May 24, 2018

jsafrane May 24, 2018

derekwaynecarr commented May 24, 2018

jingxu97 May 24, 2018

gnufied May 24, 2018

jingxu97 commented May 24, 2018

gnufied commented May 24, 2018

gnufied commented May 24, 2018

msau42 commented Jun 1, 2018

k8s-github-robot commented Jun 1, 2018

gnufied commented Jun 1, 2018

bsalamat left a comment

bsalamat Jun 1, 2018

gnufied Jun 1, 2018

msau42 commented Jun 1, 2018

bsalamat left a comment

k8s-ci-robot commented Jun 1, 2018

fejta-bot commented Jun 2, 2018

k8s-github-robot commented Jun 2, 2018

andyzhangx Jun 4, 2018

gnufied Jun 4, 2018

yujuhong commented Jun 4, 2018 •

edited

Loading

gnufied commented Jun 4, 2018

Implement dynamic volume limits #64154

Implement dynamic volume limits #64154

Conversation

gnufied commented May 22, 2018 • edited Loading

gnufied commented May 24, 2018

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 commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented May 24, 2018

gnufied commented May 24, 2018

gnufied commented May 24, 2018

msau42 commented Jun 1, 2018

k8s-github-robot commented Jun 1, 2018

gnufied commented Jun 1, 2018

bsalamat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jun 1, 2018

bsalamat left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 1, 2018

fejta-bot commented Jun 2, 2018

k8s-github-robot commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Jun 4, 2018 • edited Loading

gnufied commented Jun 4, 2018

gnufied commented May 22, 2018 •

edited

Loading

yujuhong commented Jun 4, 2018 •

edited

Loading