Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Volume Scheduling Limits #77595

Merged
merged 5 commits into from Jun 26, 2019

Conversation

@bertinatto
Copy link
Member

commented May 8, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR implements the changes proposed by the KEP Volume Scheduling Limits.

Special notes for your reviewer:

Commits are grouped to make review easier, will squash them later.

Does this PR introduce a user-facing change?:

Integrated volume limits for in-tree and CSI volumes into one scheduler predicate.

CC @msau42, @saad-ali, @davidz627, @jsafrane, @gnufied

@bertinatto

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

/sig storage

Show resolved Hide resolved pkg/apis/storage/types.go
Show resolved Hide resolved pkg/apis/storage/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/storage/v1beta1/types.go Outdated
Show resolved Hide resolved staging/src/k8s.io/api/storage/v1beta1/types.go
@fejta-bot

This comment has been minimized.

Copy link

commented May 8, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@gnufied

This comment has been minimized.

Copy link
Member

commented May 8, 2019

/label api-review

@bertinatto bertinatto force-pushed the bertinatto:volume_limits branch from 0528077 to 3a9fd7b May 9, 2019

@bertinatto bertinatto force-pushed the bertinatto:volume_limits branch from 3a9fd7b to 19d4894 May 9, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented May 9, 2019

/assign @thockin

@liggitt liggitt added this to Assigned in API Reviews May 9, 2019

@bertinatto bertinatto force-pushed the bertinatto:volume_limits branch 2 times, most recently from 35fbb65 to 0178cbd May 9, 2019

Show resolved Hide resolved pkg/apis/storage/types.go Outdated
Show resolved Hide resolved pkg/apis/storage/types.go Outdated
Show resolved Hide resolved pkg/apis/storage/types.go Outdated
Show resolved Hide resolved pkg/apis/storage/types.go
Show resolved Hide resolved pkg/apis/storage/validation/validation.go
Show resolved Hide resolved pkg/apis/storage/validation/validation_test.go
Show resolved Hide resolved pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go
Show resolved Hide resolved pkg/volume/csi/nodeinfomanager/nodeinfomanager.go
Show resolved Hide resolved test/e2e/storage/csi_mock_volume.go Outdated
@msau42

This comment has been minimized.

Copy link
Member

commented May 10, 2019

https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/storage/csinode/strategy.go should also be modified to drop fields based on feature gates

@bertinatto bertinatto force-pushed the bertinatto:volume_limits branch from 745219b to 6abc04d Jun 25, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 25, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@bertinatto: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e 3a9fd7b link /test pull-kubernetes-local-e2e
pull-kubernetes-cross cf9ce5b link /test pull-kubernetes-cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@bertinatto

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

/hold cancel

@bertinatto

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

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

@bertinatto

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

/retest

@davidz627

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@bsalamat I believe your comments are addressed and tests are passing, could you re-lgtm
👏 for everyone's massive effort on this!

@msau42

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

/priority important-soon

@bsalamat
Copy link
Member

left a comment

/lgtm
/approve

Thanks, @bertinatto!

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 25, 2019

@k8s-ci-robot k8s-ci-robot merged commit 22fb6fd into kubernetes:master Jun 26, 2019

23 checks passed

cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
newCSINode := obj.(*storage.CSINode)
oldCSINode := old.(*storage.CSINode)

inUse := getAllocatablesInUse(oldCSINode)

This comment has been minimized.

Copy link
@tedyu

tedyu Jun 26, 2019

Contributor

It seems this call should be moved inside the feature gate check.

if !ok {
return fmt.Errorf("node %v is not found", csiNode.Name)
}
n.info.SetCSINode(nil)

This comment has been minimized.

Copy link
@tedyu

tedyu Jun 26, 2019

Contributor

If n, the NodeInfoListItem, was created on line 579, shouldn't cache.removeNodeInfoFromList be called ?

d := n.csiNode.Spec.Drivers[i]
if d.Allocatable != nil && d.Allocatable.Count != nil {
// TODO: drop GetCSIAttachLimitKey once we don't get values from Node object
k := v1.ResourceName(volumeutil.GetCSIAttachLimitKey(d.Name))

This comment has been minimized.

Copy link
@tedyu

tedyu Jun 26, 2019

Contributor

Can we establish map from ResourceName to int64 once (inside NodeInfo#SetCSINode) so that we don't perform the calculation here every time ?

@tedyu

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@jsafrane @msau42
what do you think of the proposal of caching csiVolumeLimits inside NodeInfo ?

diff --git a/pkg/scheduler/nodeinfo/node_info.go b/pkg/scheduler/nodeinfo/node_info.go
index 82c7d49333..aaae4d953b 100644
--- a/pkg/scheduler/nodeinfo/node_info.go
+++ b/pkg/scheduler/nodeinfo/node_info.go
@@ -85,6 +85,7 @@ type NodeInfo struct {
        // Whenever NodeInfo changes, generation is bumped.
        // This is used to avoid cloning it if the object didn't change.
        generation int64
+       csiVolumeLimits map[v1.ResourceName]int64
 }

 //initializeNodeTransientInfo initializes transient information pertaining to node.
@@ -490,13 +491,8 @@ func (n *NodeInfo) VolumeLimits() map[v1.ResourceName]int64 {
        }

        if n.csiNode != nil {
-               for i := range n.csiNode.Spec.Drivers {
-                       d := n.csiNode.Spec.Drivers[i]
-                       if d.Allocatable != nil && d.Allocatable.Count != nil {
-                               // TODO: drop GetCSIAttachLimitKey once we don't get values from Node object
-                               k := v1.ResourceName(volumeutil.GetCSIAttachLimitKey(d.Name))
-                               volumeLimits[k] = int64(*d.Allocatable.Count)
-                       }
+               for k, v := range n.csiVolumeLimits {
+                       volumeLimits[k] = v
                }
        }

@@ -673,6 +669,15 @@ func (n *NodeInfo) RemoveNode(node *v1.Node) error {
 // SetCSINode sets the overall CSI-related node information.
 func (n *NodeInfo) SetCSINode(csiNode *storagev1beta1.CSINode) {
        n.csiNode = csiNode
+       n.csiVolumeLimits = map[v1.ResourceName]int64{}
+       for i := range n.csiNode.Spec.Drivers {
+               d := n.csiNode.Spec.Drivers[i]
+               if d.Allocatable != nil && d.Allocatable.Count != nil {
+                       // TODO: drop GetCSIAttachLimitKey once we don't get values from Node object
+                       k := v1.ResourceName(volumeutil.GetCSIAttachLimitKey(d.Name))
+                       n.csiVolumeLimits[k] = int64(*d.Allocatable.Count)
+               }
+       }
 }

 // FilterOutPods receives a list of pods and filters out those whose node names
@msau42

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@tedyu the number of different drivers installed on a node are not expected to be very high. I'm not sure if we will see much performance benefit from this. You can try running the benchmarks that @jsafrane developed to see. #77397

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.