Evenly spread volumes of a StatefuleSet across nodes based on topology #151

ddebroy · 2018-10-16T08:39:15Z

If immediate volume binding mode is set and the PVC follows StatefulSet naming format, then the provisioner will choose, as the first segment in preferred topology, a segment from requisite topology based on the relevant parts of the PVC name that ensures an even spread of topology across the StatefulSet's volumes.

The logic around determining the StatefulSet naming format and obtaining a hash and index is identical to the logic in pkg/volume/util/util.go Kubernetes main tree.

Fixes: #143

cc @msau42 @verult

ddebroy · 2018-10-16T08:40:39Z

/assign @jsafrane

msau42 · 2018-10-16T17:49:33Z

pkg/controller/topology_test.go

+		// ordering of segments in preferred array is sensitive to statefulset name: testset
+		// if statefuleset name is changed, make sure expectedPreferred is kept in sync
+		// pvc prefix in pvcName does not have any effect on segment ordering
+		"pvc 1a": {


Also a test case when allowedTopologies is specified?. Do we also need to to convert the old zone/zones to AllowedTopologies for migration?

Good point - I can add an additional sanity test with allowedTopologies specified for completeness. TestAllowedTopologies already covers several scenarios for trimming down the requisiteTerms based on allowedTopologies but it will be good to have a sanity case here as well.

Added point 4b in https://docs.google.com/document/d/15Q4wXVqbalrVgf_jlGis4QFXN1RY1GMiY-Ek-hpCr-o/edit# for the migration work around topology.

Extra test cases with allowedTopologies added in latest commit.

msau42 · 2018-10-17T02:36:07Z

/assign

verult · 2018-10-18T18:52:53Z

/assign

k8s-ci-robot · 2018-10-18T18:52:55Z

@verult: GitHub didn't allow me to assign the following users: verult.

Note that only kubernetes-csi members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign

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.

verult

First pass. Thanks Deep!

verult · 2018-10-18T18:53:52Z

pkg/controller/topology.go

@@ -120,6 +123,14 @@ func GenerateAccessibilityRequirements(
 			return nil, fmt.Errorf("topology %v from selected node %q is not in requisite", selectedTopology, selectedNode.Name)
 		}

+		requirement.Preferred = toCSITopology(preferredTerms)


Can pull this out of the if branch

verult · 2018-10-18T18:55:36Z

pkg/controller/topology.go

@@ -120,6 +123,14 @@ func GenerateAccessibilityRequirements(
 			return nil, fmt.Errorf("topology %v from selected node %q is not in requisite", selectedTopology, selectedNode.Name)
 		}

+		requirement.Preferred = toCSITopology(preferredTerms)
+	} else {


nit: if selectedNode == nil branch above the other; positive branch first

verult · 2018-10-18T18:56:05Z

pkg/controller/topology.go

@@ -120,6 +123,14 @@ func GenerateAccessibilityRequirements(
 			return nil, fmt.Errorf("topology %v from selected node %q is not in requisite", selectedTopology, selectedNode.Name)
 		}

+		requirement.Preferred = toCSITopology(preferredTerms)
+	} else {
+		hash, index := getPVCNameHashAndIndexOffset(pvcName)


Comment on why all the logic in this else branch is needed

verult · 2018-10-18T18:58:03Z

pkg/controller/topology.go

+		requirement.Preferred = toCSITopology(preferredTerms)
+	} else {
+		hash, index := getPVCNameHashAndIndexOffset(pvcName)
+		sort.Slice(requisiteTerms, func(i, j int) bool {


Could we potentially reuse sortAndShift() here? Would make it easier to read if the runtime loss isn't significant

Done. Added an an extra parameter to sortAndShift() - shiftIndex - which will be considered if the topologyTerm parameter is nil.

verult · 2018-10-18T18:59:39Z

pkg/controller/topology.go

@@ -367,3 +378,52 @@ func toCSITopology(terms []topologyTerm) []*csi.Topology {
 	}
 	return out
 }
+
+// identical to logic in in-tree pkg/volume/util/util.go


Does it make sense to have the full URL to the master branch version of this file?

verult · 2018-10-18T19:07:36Z

pkg/controller/topology_test.go

+		pvcName           string
+		expectedPreferred []*csi.Topology
+	}{
+		// ordering of segments in preferred array is sensitive to statefulset name: testset


nit: Maybe move this comment outside? At first glance I thought this comment was for a specific test case

verult · 2018-10-18T19:09:54Z

pkg/controller/topology_test.go

+		expectedPreferred []*csi.Topology
+	}{
+		// ordering of segments in preferred array is sensitive to statefulset name: testset
+		// if statefuleset name is changed, make sure expectedPreferred is kept in sync


statefuleset -> statefulset

verult · 2018-10-18T19:19:55Z

pkg/controller/topology_test.go

+		// ordering of segments in preferred array is sensitive to statefulset name: testset
+		// if statefuleset name is changed, make sure expectedPreferred is kept in sync
+		// pvc prefix in pvcName does not have any effect on segment ordering
+		"pvc 1a": {


nit: Use a test name that describes what the case is testing

ddebroy · 2018-10-19T07:16:02Z

Thanks for the detailed review @verult and @msau42. Addressed the code review comments so far.

verult

Just some minor comments now, otherwise looks good!

pkg/controller/topology.go

ddebroy · 2018-10-24T07:28:47Z

@verult comments fixed.

verult · 2018-10-24T20:37:24Z

/lgtm

k8s-ci-robot · 2018-10-24T20:37:31Z

@verult: changing LGTM is restricted to assignees, and only kubernetes-csi/external-provisioner repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

ddebroy · 2018-10-24T20:53:30Z

/assign @verult

ddebroy · 2018-10-24T20:54:09Z

@verult assigned you so that lgtm is accepted :-)

verult · 2018-10-24T22:56:01Z

/lgtm

ddebroy · 2018-10-25T00:35:00Z

@jsafrane can you PTAL?

jsafrane · 2018-10-26T07:38:16Z

/approve

k8s-ci-robot · 2018-10-26T07:38:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, jsafrane

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:

~~OWNERS~~ [jsafrane]

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

jsafrane · 2018-10-26T07:38:37Z

/hold

please squash the last commit

ddebroy · 2018-10-26T07:43:03Z

@jsafrane squashed all commits.

Signed-off-by: Deep Debroy <[email protected]>

jsafrane · 2018-10-26T07:48:57Z

/hold cancel
/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2018

k8s-ci-robot requested review from davidz627 and sbezverk October 16, 2018 08:39

ddebroy requested a review from msau42 October 16, 2018 08:40

k8s-ci-robot assigned jsafrane Oct 16, 2018

msau42 reviewed Oct 16, 2018

View reviewed changes

k8s-ci-robot assigned msau42 Oct 17, 2018

verult suggested changes Oct 18, 2018

View reviewed changes

verult reviewed Oct 22, 2018

View reviewed changes

pkg/controller/topology.go Outdated Show resolved Hide resolved

pkg/controller/topology.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned verult Oct 24, 2018

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

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2018

ddebroy force-pushed the ddebroy-spread1 branch from ab6ca85 to 5b60039 Compare October 26, 2018 07:41

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

Evenly spread volumes of a StatefuleSet across nodes based on topology

14cd3e4

Signed-off-by: Deep Debroy <[email protected]>

ddebroy force-pushed the ddebroy-spread1 branch from 5b60039 to 14cd3e4 Compare October 26, 2018 07:46

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 26, 2018

k8s-ci-robot merged commit a8d9d8d into kubernetes-csi:master Oct 26, 2018

ddebroy mentioned this pull request Feb 8, 2019

REQUEST: New membership for ddebroy kubernetes/org#461

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evenly spread volumes of a StatefuleSet across nodes based on topology #151

Evenly spread volumes of a StatefuleSet across nodes based on topology #151

ddebroy commented Oct 16, 2018

ddebroy commented Oct 16, 2018

msau42 Oct 16, 2018

ddebroy Oct 17, 2018

ddebroy Oct 19, 2018

msau42 commented Oct 17, 2018

verult commented Oct 18, 2018

k8s-ci-robot commented Oct 18, 2018

verult left a comment

verult Oct 18, 2018

verult Oct 18, 2018

verult Oct 18, 2018

verult Oct 18, 2018

ddebroy Oct 19, 2018

verult Oct 18, 2018

verult Oct 18, 2018

verult Oct 18, 2018

verult Oct 18, 2018

ddebroy commented Oct 19, 2018

verult left a comment

ddebroy commented Oct 24, 2018

verult commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

ddebroy commented Oct 24, 2018

ddebroy commented Oct 24, 2018

verult commented Oct 24, 2018

ddebroy commented Oct 25, 2018

jsafrane commented Oct 26, 2018

k8s-ci-robot commented Oct 26, 2018

jsafrane commented Oct 26, 2018

ddebroy commented Oct 26, 2018 •

edited

Loading

jsafrane commented Oct 26, 2018

Evenly spread volumes of a StatefuleSet across nodes based on topology #151

Evenly spread volumes of a StatefuleSet across nodes based on topology #151

Conversation

ddebroy commented Oct 16, 2018

ddebroy commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Oct 17, 2018

verult commented Oct 18, 2018

k8s-ci-robot commented Oct 18, 2018

verult 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

ddebroy commented Oct 19, 2018

verult left a comment

Choose a reason for hiding this comment

ddebroy commented Oct 24, 2018

verult commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

ddebroy commented Oct 24, 2018

ddebroy commented Oct 24, 2018

verult commented Oct 24, 2018

ddebroy commented Oct 25, 2018

jsafrane commented Oct 26, 2018

k8s-ci-robot commented Oct 26, 2018

jsafrane commented Oct 26, 2018

ddebroy commented Oct 26, 2018 • edited Loading

jsafrane commented Oct 26, 2018

ddebroy commented Oct 26, 2018 •

edited

Loading