Fix csi attach limit #67731

gnufied · 2018-08-22T22:18:28Z

Add support for volume limits for CSI.

xref: kubernetes/community#2051

Add support for volume attach limits for CSI volumes

gnufied · 2018-08-22T22:18:38Z

/sig storage

Create a new predicate to count CSI volumes

neolit123 · 2018-08-23T01:10:31Z

pkg/scheduler/algorithm/predicates/csi_volume_predicate.go

+
+	for _, vol := range volumes {
+		// CSI volumes can only be used as persistent volumes
+		if vol.PersistentVolumeClaim != nil {


can be optimized to:

if vol.PersistentVolumeClaim == nil { continue }

gnufied · 2018-08-23T15:47:08Z

/sig storage

msau42 · 2018-08-24T01:02:37Z

pkg/scheduler/algorithm/predicates/csi_volume_predicate.go

+			}
+
+			pvName := pvc.Spec.VolumeName
+			if pvName == "" {


Add a TODO here that this is not going to work with late binding

msau42 · 2018-08-24T01:06:03Z

pkg/volume/csi/nodeupdater/nodeupdater.go

+
+// Package nodeupdater includes internal functions used to add/delete labels to
+// kubernetes nodes for corresponding CSI drivers
+package nodeupdater // import "k8s.io/kubernetes/pkg/volume/csi/nodeupdater"


cc @verult

You guys are both renaming/refactoring this file. Please sync up.

yeah I will address this when I rebase against #67684

msau42 · 2018-08-24T01:08:22Z

pkg/volume/csi/nodeupdater/nodeupdater.go

+		if labelErr != nil {
+			return labelErr
+		}
+		node = addMaxAttachLimitToNode(node, driverName, maxLimit)


Does this need to be feature gated?

msau42 · 2018-08-24T01:09:30Z

pkg/volume/util/attach_limit_test.go

+	// When driver is longer than 39 chars
+	csiLimitKeyLonger := GetCSIAttachLimitKey("com.amazon.kubernetes.eks.ec2.ebs/csi-driver")
+	fmt.Println(csiLimitKeyLonger)
+	if !v1helper.IsAttachableVolumeResourceName(v1.ResourceName(csiLimitKeyLonger)) {


Should you check for the expected hash in this test?

msau42 · 2018-08-24T01:11:00Z

pkg/scheduler/algorithm/predicates/csi_volume_predicate.go

+	return c.attachableLimitPredicate
+}
+
+func (c *CSIMaxVolumeLimitChecker) attachableLimitPredicate(


Could we not have reused the existing MaxPD predicate and just define a new filter for CSI?

May be - we can, but it will be ugly because underlying data structures are different. The existing MaxPD* predicates filter volumes based on hardcoded type. For example - "how many EBS volumes are attached to the node", but since there can be multiple CSI volumes on a node, the CSI predicate has to count - "How many volumes of each type are attached to the node" and same thing applies to uniqueness check and etc.

At some point - I would like to consolidate all the predicates in one when AttachLimit feature becomes GA, because then all the volume limits will come from node itself and we can remove these older and hardcoded predicates.

Thanks for the explanation, it makes sense to keep them separate for now

added some tests too btw

verult · 2018-08-24T17:23:03Z

/cc

msau42 · 2018-09-04T22:50:39Z

pkg/scheduler/algorithm/predicates/csi_volume_predicate.go

+	volumes []v1.Volume, namespace string, result map[string]string) error {
+
+	for _, vol := range volumes {
+		// CSI volumes can only be used as persistent volumes


Do we need to revisit this for inline csi volumes?

I am not 100% sure yet but I thought plan was to keep inline CSI volumes non-attachable. cc @vladimirvivien is that still true?

discussed this offline. The handling of inline CSI volumes is out of scope for this PR. Currently inline CSI volumes are being proposed as an alpha feature.

msau42 · 2018-09-04T22:52:46Z

pkg/scheduler/algorithm/predicates/csi_volume_predicate.go

+		pvc, err := c.pvcInfo.GetPersistentVolumeClaimInfo(namespace, pvcName)
+
+		if err != nil {
+			glog.Errorf("Unable to look up PVC info for %s/%s", namespace, pvcName)


These messages have the potential to spam the logs if a pod spec is not constructed correctly. Can these logs be changed to Info at level 4?

msau42 · 2018-09-04T22:56:43Z

pkg/volume/util/attach_limit.go

@@ -26,4 +31,25 @@ const (
 	AzureVolumeLimitKey = "attachable-volumes-azure-disk"
 	// GCEVolumeLimitKey stores resource name that will store volume limits for GCE node
 	GCEVolumeLimitKey = "attachable-volumes-gce-pd"


Do the in-tree limit keys need to be changed to be compatible with the csi driver equivalent?

You mean - once migration happens? I think we will stick to the plan of carrying around a map for in-tree plugins when we start shipping those.

Yes, I mean, now that this is going beta, we need to support these limits keys for two releases. So there will be a period of time where we'll need some in-tree volume key to csi translation function

gnufied · 2018-09-05T00:01:01Z

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-kubemark-e2e-gce-big

msau42 · 2018-09-05T00:15:27Z

/lgtm

gnufied · 2018-09-05T03:04:19Z

/retest

bsalamat

/lgtm

The changes to the scheduler look fine to, but I am not familiar with the details of CSI volume operations. My assumption is that that part is checked by the SIG storage folks.

bsalamat · 2018-09-05T05:46:18Z

pkg/scheduler/factory/factory.go

@@ -488,6 +488,10 @@ func (c *configFactory) invalidatePredicatesForPv(pv *v1.PersistentVolume) {
 		invalidPredicates.Insert(predicates.MaxAzureDiskVolumeCountPred)
 	}

+	if pv.Spec.CSI != nil && utilfeature.DefaultFeatureGate.Enabled(features.AttachVolumeLimit) {


I don't want to hold this PR, but please send a follow-up PR to add tests for these invalidation cases.

bsalamat · 2018-09-05T05:47:57Z

pkg/scheduler/algorithmprovider/defaults/compatibility_test.go

+			{"name": "MaxEBSVolumeCount"},
+			{"name": "MaxGCEPDVolumeCount"},
+			{"name": "MaxAzureDiskVolumeCount"},
+                        {"name": "MaxCSIVolumeCountPred"},


nit: fix the indentation

gnufied · 2018-09-05T16:33:56Z

/priority critical-urgent

eparis · 2018-09-05T16:33:58Z

/approve

eparis · 2018-09-05T16:34:09Z

/lgtm

k8s-ci-robot · 2018-09-05T16:34:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, eparis, gnufied, msau42

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:

~~hack/OWNERS~~ [eparis]
~~pkg/features/OWNERS~~ [bsalamat]
~~pkg/scheduler/OWNERS~~ [bsalamat]
~~pkg/volume/csi/OWNERS~~ [gnufied]
~~pkg/volume/util/OWNERS~~ [gnufied]
~~test/integration/scheduler/OWNERS~~ [bsalamat,eparis]

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

k8s-github-robot · 2018-09-05T21:51:53Z

Automatic merge from submit-queue (batch tested with PRs 68161, 68023, 67909, 67955, 67731). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

k8s-ci-robot requested review from msau42 and rootfs August 22, 2018 22:18

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2018

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 22, 2018

gnufied added 2 commits August 22, 2018 19:36

Implement support for updating volume limits

4b17a48

Create a new predicate to count CSI volumes

Move volume limit feature to beta

8e4b33d

gnufied force-pushed the fix-csi-attach-limit branch from 0d33b5c to 31cdc1b Compare August 22, 2018 23:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2018

neolit123 reviewed Aug 23, 2018

View reviewed changes

gnufied force-pushed the fix-csi-attach-limit branch from 31cdc1b to e4c9bae Compare August 23, 2018 11:50

gnufied force-pushed the fix-csi-attach-limit branch from e4c9bae to d19fb19 Compare August 23, 2018 17:00

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 Aug 23, 2018

msau42 reviewed Aug 24, 2018

View reviewed changes

k8s-ci-robot requested a review from verult August 24, 2018 17:23

jsafrane added this to the v1.12 milestone Aug 28, 2018

gnufied force-pushed the fix-csi-attach-limit branch from d19fb19 to 11cc5e9 Compare September 4, 2018 22:21

k8s-ci-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 Sep 4, 2018

gnufied force-pushed the fix-csi-attach-limit branch from 4dc54ef to 7710dd6 Compare September 4, 2018 22:36

msau42 reviewed Sep 4, 2018

View reviewed changes

gnufied force-pushed the fix-csi-attach-limit branch from 7710dd6 to fdf8a5e Compare September 5, 2018 00:04

k8s-ci-robot assigned msau42 Sep 5, 2018

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

bsalamat reviewed Sep 5, 2018

View reviewed changes

k8s-ci-robot assigned bsalamat Sep 5, 2018

Fix compatibility tests for scheduler

fc61620

gnufied force-pushed the fix-csi-attach-limit branch from fdf8a5e to fc61620 Compare September 5, 2018 16:29

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

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 5, 2018

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

k8s-ci-robot assigned eparis Sep 5, 2018

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

k8s-github-robot merged commit ca43f00 into kubernetes:master Sep 5, 2018

gnufied mentioned this pull request Sep 7, 2018

CSI volume limit tidy up #68425

Closed

Fix csi attach limit #67731

Fix csi attach limit #67731

Conversation

gnufied commented Aug 22, 2018 • edited Loading

gnufied commented Aug 22, 2018

Choose a reason for hiding this comment

gnufied commented Aug 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Sep 4, 2018 • 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

gnufied commented Sep 5, 2018

msau42 commented Sep 5, 2018

gnufied commented Sep 5, 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

Choose a reason for hiding this comment

gnufied commented Sep 5, 2018

eparis commented Sep 5, 2018

eparis commented Sep 5, 2018

k8s-ci-robot commented Sep 5, 2018

k8s-github-robot commented Sep 5, 2018

gnufied commented Aug 22, 2018 •

edited

Loading

gnufied Sep 4, 2018 •

edited

Loading

gnufied Sep 4, 2018 •

edited

Loading