Topology beta #238

msau42 · 2019-03-02T04:14:45Z

Move from alpha CRDs to beta in-tree CSINode object.

I am temporarily copying the new API files into vendor

k8s-ci-robot · 2019-03-02T04:14:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [msau42]

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

msau42 · 2019-03-02T04:14:58Z

/assign @jsafrane @xing-yang

README.md

msau42 · 2019-03-13T23:49:27Z

pkg/features/features.go

@@ -31,5 +32,5 @@ func init() {
 // defaultKubernetesFeatureGates consists of all known feature keys specific to external-provisioner.
 // To add a new feature, define a key for it above and add it here.
 var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureSpec{
-	Topology: {Default: false, PreRelease: utilfeature.Alpha},
+	Topology: {Default: true, PreRelease: utilfeature.Beta},


Right now the sidecar errors if the CSINode object doesn't exist + the topology feature is enabled.

This may break backwards compatibility if you use a newer sidecar version on 1.13 where csi topology is disabled.

We could either:

Say that this sidecar version is not supported on versions < 1.14

Fallback to topology disabled behavior if CSINode doesn't exist

I'm leaning towards the latter, but that also means if we're on 1.14 and there's actually a problem with CSINode/driver registration, then we will continue to successfully provision a volume in a potentially bad zone.

wdyt? @jsafrane @verult @davidz627

actually now that I think about it more, is that what we want? If the api server is temporarily having problems, then we end up provisioning in the wrong zone.

another option is that we leave topology off by default (and we offer no compatibility guarantees from alpha->beta, including k/k alpha->beta).

The new sidecar would work on version <1.14 with Topology enabled though right? So this is not a compatibility breaking thing. More of a "did you enable the feature on one component but not enable it on another" type problem. Maybe this is ok if we give a good error message making it very clear what is wrong in this case and how to solve it

If the user on 1.13 has delayed binding enabled on the storageclass in a single zone cluster, then with the old provisioner, provisioning still succeeds. If they upgrade the sidecar, then provisioning will fail with an error.

Now it's questionable what benefit they would get by enabling delayed binding in a single zone cluster, but there's currently no harm in doing so.

I'm going to make all the APIs call return an error if they fail (no fallback), but set topology feature gate to false. Drivers will need to explicitly enable the topology feature gate after upgrading to 1.14.

For drivers with topology disabled, upgrading the sidecar on 1.13 should not break.

For drivers with topology enabled, upgrading the sidecar on 1.13 will break. I think this is ok given it was an alpha feature.

Discussed with @jsafrane some more. To simplify our documentation and testing matrix, we should just require 1.14 for this new version of the sidecar. So then I can have topology enabled by default

Makes sense. We then need to maintain the older release branch for those people who use CSI with 1.13.

The Prow-based CI is ready for testing against two different Kubernetes releases. My proposal in https://github.com/kubernetes-csi/csi-driver-host-path/pull/16/commits is to maintain one or more deployments in the master branch, so users (and the CI) will always find what they look for there.

davidz627

mostly lgtm

deploy/kubernetes/rbac.yaml

pkg/controller/topology.go

verult

first (partial) pass

pkg/controller/topology.go

pkg/controller/topology_test.go

msau42 · 2019-03-19T22:25:29Z

I made substantial changes in order to fail appropriately while still preserving backwards compatibility with topology disabled behavior. A summary of the changes:

Immediate binding:
- If a beta CSINode object that registers topology keys is found, at least one node has been upgraded, fail appropriately if needed
- Otherwise, fallback to "topology disabled" behavior
Delayed binding:
- If selected node doesn't have beta CSINode object and the node is < 1.14, fallback to "topology disabled" behavior
- Otherwise require that topology keys/labels are set.

Most of the test cases are under TestTopologyAggregation.

davidz627

LGTM. kind of skimmed the test cases though so if someone else can look at those more carefully. The comments are amazing!! They really help with understanding

pkg/controller/topology.go

pkg/controller/controller_test.go

msau42 · 2019-03-25T21:09:43Z

pkg/controller/topology.go

+			// CSINode beta feature on K8s apiserver must also be enabled.
+			// We don't want to fallback and provision in the wrong topology if there's some temporary
+			// error with the API server.
+			return nil, fmt.Errorf("error listing CSINodes: %v", err)


If we need to support running newer external provisioner on 1.13, then I think we need to disable topology by default. Otherwise, provisioning on 1.13 will start to hit this error (if the driver also supports csi topology capability).

msau42 · 2019-03-25T23:39:11Z

I've addressed comments and also wrapped the logic to lookup the CSINode object to reduce duplication of error handling. I've also turned topology off by default again.

pkg/controller/topology.go

verult · 2019-03-25T22:00:12Z

pkg/controller/topology.go

-		// In either case, provisioning needs to be allowed to proceed.
-		return nil, nil
+		// Even though selectedNode is set, we still need to aggregate topology values across
+		// all nodes in order to find secondary topologies.


"in order to compute a priority ordering of topologies for preferredTopologies"

Strictly speaking, it's only limited to a "secondary" in the case of regional PDs

There's some desire to be able to limit the number of topologies returned, especially for most plugins that only have a single primary topology: #221

So I wanted to add in some more explicit comments here explaining the reason why we need to return all possible topologies even though a plugin may only need one.

verult · 2019-03-25T22:06:21Z

pkg/controller/topology.go

@@ -88,6 +91,13 @@ func GenerateAccessibilityRequirements(
 		requisiteTerms = flatten(allowedTopologies)
 	}

+	// It might be possible to reach here if:


nit: At the beginning of this function, could you describe the high-level compatibility considerations we discussed offline? I found it hard to see the high-level picture reading through the different levels of conditional branches.

Would it also be helpful to label which pieces of logic can be removed after 1.13 is no longer a supported version?

I added a giant comment at the top. Let me know if this format works or you prefer something else

It's super clear. Thanks!

pkg/controller/topology_test.go

verult · 2019-03-25T23:14:18Z

pkg/controller/topology_test.go

 			topologyKeys:      nil,
 			expectedRequisite: nil,
 		},
+		// Driver on node has not been updated to report topology keys
 		"random node: missing keys": {


Should we have a test case where there's only a single CSINode with keys, in which there should be no errors?

Do the existing "same keys and values across cluster" and "different values across cluster" cases address this?

Not quite. I'm thinking about the case where k8s nodes are partially upgraded to 1.14, so some nodes have keys but others don't

Something like:

nodeLabels: []map[string]string{ {"com.example.csi/zone": "zone1"}, {"com.example.csi/zone": "zone2"}, {"com.example.csi/zone": "zone2"}, }, topologyKeys: []map[string][]string{ {testDriverName: []string{"com.example.csi/zone"}}, },

in which the requisite should only have "com.example.csi/zone": "zone1", right?

Added 2 more test cases.

So actually, we in your case above, requisite has both zone1 and zone2. even though the other nodes don't have csiNode. It's because csiNode is only used to determine which topology keys to use, but we just iterate over all nodes that have the topology key.

pkg/controller/topology.go

msau42 · 2019-03-26T01:21:27Z

addressed latest comments

verult · 2019-03-26T21:57:41Z

pkg/features/features.go

@@ -31,5 +32,5 @@ func init() {
 // defaultKubernetesFeatureGates consists of all known feature keys specific to external-provisioner.
 // To add a new feature, define a key for it above and add it here.
 var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureSpec{
-	Topology: {Default: false, PreRelease: utilfeature.Alpha},
+	Topology: {Default: false, PreRelease: utilfeature.Beta},


Do we need an issue tracking when to enable this by default?

I'm unsure if we need actually need to. It's important that kubernetes enables topology by default since it needs to be able to support all kinds of drivers, but since this flag is only controlling a specific driver implementation, I'm not sure if it actually gives much benefit being enabled by default.

verult · 2019-03-26T22:02:09Z

pkg/controller/topology.go

@@ -88,6 +91,13 @@ func GenerateAccessibilityRequirements(
 		requisiteTerms = flatten(allowedTopologies)
 	}

+	// It might be possible to reach here if:


It's super clear. Thanks!

verult · 2019-03-26T22:07:42Z

pkg/controller/topology_test.go

 			topologyKeys:      nil,
 			expectedRequisite: nil,
 		},
+		// Driver on node has not been updated to report topology keys
 		"random node: missing keys": {


Not quite. I'm thinking about the case where k8s nodes are partially upgraded to 1.14, so some nodes have keys but others don't

msau42 · 2019-03-26T22:33:33Z

/hold
debugging e2e test failures

msau42 · 2019-03-26T23:09:27Z

E2E tests are passing (I had to explicitly enable topology in the driver because it's now turned it off by default).

I'll keep the hold to squash commits after review.

verult · 2019-03-27T21:02:33Z

/lgtm

msau42 · 2019-03-27T22:00:08Z

Squashed commits
/hold cancel

verult · 2019-03-27T22:01:01Z

/lgtm

…19rc Rework to klog/v2

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 2, 2019

k8s-ci-robot requested review from davidz627 and jsafrane March 2, 2019 04:14

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2019

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 2, 2019

k8s-ci-robot assigned jsafrane and xing-yang Mar 2, 2019

msau42 mentioned this pull request Mar 3, 2019

CSINodeInfo and CSIDriver Controller Changes kubernetes/kubernetes#74283

Merged

msau42 force-pushed the topology-beta branch 2 times, most recently from f95c396 to d8335fc Compare March 3, 2019 06:06

msau42 commented Mar 3, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

msau42 force-pushed the topology-beta branch from d8335fc to 1d9d7a7 Compare March 3, 2019 06:09

This was referenced Mar 6, 2019

e2e test updates for CSIDriver and CSINode beta kubernetes/kubernetes#74993

Merged

Update lib-external-provisioner and cleanup constraints #244

Merged

msau42 force-pushed the topology-beta branch from 1d9d7a7 to ccd1dea Compare March 13, 2019 23:26

msau42 commented Mar 13, 2019

View reviewed changes

msau42 force-pushed the topology-beta branch from c17cf6f to 476d990 Compare March 15, 2019 05:43

msau42 changed the title ~~WIP Topology beta~~ Topology beta Mar 15, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2019

msau42 force-pushed the topology-beta branch from 476d990 to 2bcb9c4 Compare March 15, 2019 20:24

davidz627 reviewed Mar 16, 2019

View reviewed changes

deploy/kubernetes/rbac.yaml Outdated Show resolved Hide resolved

pkg/controller/topology.go Show resolved Hide resolved

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

verult reviewed Mar 16, 2019

View reviewed changes

pkg/controller/topology.go Show resolved Hide resolved

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

pkg/controller/topology_test.go Show resolved Hide resolved

pkg/controller/topology_test.go Show resolved Hide resolved

msau42 force-pushed the topology-beta branch from 2bcb9c4 to 7a2c53f Compare March 16, 2019 00:23

msau42 changed the title ~~Topology beta~~ WIP: Topology beta Mar 18, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2019

msau42 force-pushed the topology-beta branch from 7a2c53f to 298f25a Compare March 19, 2019 22:12

msau42 changed the title ~~WIP: Topology beta~~ Topology beta Mar 19, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2019

davidz627 reviewed Mar 22, 2019

View reviewed changes

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

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

msau42 commented Mar 25, 2019

View reviewed changes

msau42 force-pushed the topology-beta branch from 298f25a to 7087da0 Compare March 25, 2019 23:25

msau42 added 2 commits March 25, 2019 16:33

Use beta CSINode

75ad906

vendor updates

eb8490d

msau42 force-pushed the topology-beta branch from 7087da0 to dcc668c Compare March 25, 2019 23:34

verult reviewed Mar 25, 2019

View reviewed changes

verult reviewed Mar 26, 2019

View reviewed changes

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

verult reviewed Mar 26, 2019

View reviewed changes

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

k8s-ci-robot assigned verult Mar 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2019

Require CSINode if topology is enabled. Fallback if Node is too old

b1f1db4

msau42 force-pushed the topology-beta branch from d6894b7 to b1f1db4 Compare March 27, 2019 21:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2019

k8s-ci-robot merged commit c8043f0 into kubernetes-csi:master Mar 27, 2019

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#238 from jsafrane/bump-client-go-0.…

c8128b1

…19rc Rework to klog/v2

Topology beta #238

Topology beta #238

Conversation

msau42 commented Mar 2, 2019

k8s-ci-robot commented Mar 2, 2019

msau42 commented Mar 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 Mar 14, 2019 • 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

Choose a reason for hiding this comment

davidz627 left a comment

Choose a reason for hiding this comment

verult left a comment

Choose a reason for hiding this comment

msau42 commented Mar 19, 2019

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Mar 25, 2019

Choose a reason for hiding this comment

msau42 Mar 26, 2019 • 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

Choose a reason for hiding this comment

verult Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Mar 26, 2019

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

verult Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

msau42 commented Mar 26, 2019

msau42 commented Mar 26, 2019 • edited Loading

verult commented Mar 27, 2019

msau42 commented Mar 27, 2019

verult commented Mar 27, 2019

davidz627 Mar 14, 2019 •

edited

Loading

msau42 Mar 26, 2019 •

edited

Loading

verult Mar 26, 2019 •

edited

Loading

verult Mar 26, 2019 •

edited

Loading

msau42 commented Mar 26, 2019 •

edited

Loading