distributed provisioning: allowed topologies + immediate binding #612

pohly · 2021-04-19T13:31:55Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

When using immediate binding with allowed topologies set in the storage class together with distributed provisioning, node instances must ignore PVCs that do not fit their node.

Which issue(s) this PR fixes:
Fixes #611

Special notes for your reviewer:

Includes some drive-by test enhancements.

Does this PR introduce a user-facing change?:

Fixes an issue when using immediate binding, allowed topologies and distributed provisioning, where PVCs may have been assigned to nodes which did not fit the allowed topologies, leading to permanent `error generating accessibility requirements` failures.

/cc @chrishenzie

pohly · 2021-04-19T14:15:13Z

/test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master

k8s-ci-robot · 2021-04-19T14:34:16Z

@pohly: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master	`cfabf52`	link	`/test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master`

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.

pohly · 2021-04-19T16:00:04Z

The whole pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master job can be ignored. It still uses the v1.6.0 release of the hostpath driver. I just wasn't sure anymore and thought I give it a try...

chrishenzie · 2021-04-19T16:58:11Z

/assign @verult

chrishenzie · 2021-04-19T16:58:57Z

pkg/controller/controller.go

@@ -1293,6 +1293,32 @@ func (p *csiProvisioner) checkNode(ctx context.Context, claim *v1.PersistentVolu
 			return false, nil
 		}

+		// If the storage class as AllowedTopologies set, then


verult · 2021-04-19T19:23:29Z

pkg/controller/controller.go

+		// If the storage class has AllowedTopologies set, then
+		// it must match our own. We can find out by trying to
+		// create accessibility requirements.  If that fails,
+		// we should become the owner.


we should not become the owner

Duh. Of course. Fixed.

verult · 2021-04-19T19:40:43Z

pkg/controller/controller.go

+				claim.Name,
+				sc.AllowedTopologies,
+				node,
+				p.strictTopology,


If strictTopology is false, does GenerateAccessibilityRequirements() return an error if node isn't part of AllowedTopologies?

Yes, it should. The "strict topology" case is just a simpler version of that existing, older check.

Sg. I haven't been able to find the old logic but since we have test coverage this should be great. I'll find the logic eventually 😛

I actually still couldn't find the logic for this when strictTopology is false. If you happen to know off-hand could you point me to it?

The logic must exist somewhere because if it doesn't, GenerateAccessibilityRequirements would return a topology and no error. checkNode() then proceeds and tries to becomeOwner(), in which case either

it succeeds and selected node is updated, or

it fails and returns an error, and the caller, ShouldProvision(), proceeds to return true.

In both cases the distributed immediate, allowed topologies not okay test case should've failed (expectedSelectedNode: "" and expectNoProvision: true)

verult · 2021-04-19T20:36:43Z

pkg/controller/controller_test.go

 	}

-	if expectSelectedNode != "" {
+	if tc.volOpts.PVC != nil {


Could you quickly explain why this check was added? (just for my understanding, not necessarily as a code comment)

Just for the sake of completeness. All other pointers were also checked for non-nil.

verult · 2021-04-19T20:40:24Z

pkg/controller/controller_test.go

+			expectErr:          true,
+			expectState:        controller.ProvisioningNoChange,
+			skipCreateVolume:   true,
+			expectNoProvision:  true, // not owner and will not change that either


Prior to the fix, the test will fail at expectNoProvision, while expectErr, expectState, and skipCreateVolume will pass, right?

It would have passed expectNoProvision, but for the wrong reason (not owner yet). Without the fix, it fails expectSelectedNode because the provisioner would set the current node to itself. The correct behavior is to not provision and not set the node.

Gotcha. The behavior of selected node is a little non-obvious to me, and I had trouble making sense of what behavior should be expected from the test case parameters. Given enough time I should be able to figure it out but I couldn't quite grasp it just by scanning the test case.

Just a nit - not sure what could improve it but maybe a comment explaining the scenario could help?

Maybe consider adding a comment that selecting a node == becoming owner

Added a comment.

verult · 2021-04-19T21:10:25Z

/lgtm
/hold
(in case you want to address any nits)

When determining who should provision a PVC with immediate binding, each instance must check the AllowedTopologies before trying to become the owner of the PVC, because if the allowed topology does not include the current node, then all provisioning attempts will fail with "error generating accessibility requirements". To test this, the provisioning tests get refactored (ShouldProvision gets tested separately) and extended (new test cases with AllowedTopologies). While at it, the redundant printing of the test case name gets removed. It's redundant because sub tests are now used, so each error message is automatically associated with its test.

ShouldProvision must not try to become the owner if capacity is insufficient. That was implemented already, just not tested.

pohly · 2021-04-19T21:27:06Z

/lgtm
/hold
(in case you want to address any nits)

I do, and did 😁

Please re-approve.

verult · 2021-04-19T21:27:20Z

/lgtm
/hold cancel

verult · 2021-04-19T21:27:44Z

/assign @msau42
for approval

msau42 · 2021-04-19T21:30:49Z

pkg/controller/controller.go

+				p.csiNodeLister,
+				p.nodeLister); err != nil {
+				if logger.Enabled() {
+					logger.Infof("%s: ignoring PVC %s/%s, allowed topologies is not compatible: %v", caller, claim.Namespace, claim.Name, err)


This call can error for other reasons besides incompatible allowedTopologies. Will this correctly recover in that case? Do we need a more specific error code to look for?

Which failures? When deployed on a node, fake CSINode and Node listers are used (

external-provisioner/cmd/csi-provisioner/csi-provisioner.go

Lines 304 to 340 in 213cd3d

// Avoid watching in favor of fake, static objects. This is particularly relevant for

// Node objects, which can generate significant traffic.

csiNode := &storagev1.CSINode{

ObjectMeta: metav1.ObjectMeta{

Name: nodeDeployment.NodeName,

},

Spec: storagev1.CSINodeSpec{

Drivers: []storagev1.CSINodeDriver{

{

Name: provisionerName,

NodeID: nodeDeployment.NodeInfo.NodeId,

},

},

},

}

node := &v1.Node{

ObjectMeta: metav1.ObjectMeta{

Name: nodeDeployment.NodeName,

},

}

if nodeDeployment.NodeInfo.AccessibleTopology != nil {

for key := range nodeDeployment.NodeInfo.AccessibleTopology.Segments {

csiNode.Spec.Drivers[0].TopologyKeys = append(csiNode.Spec.Drivers[0].TopologyKeys, key)

}

node.Labels = nodeDeployment.NodeInfo.AccessibleTopology.Segments

}

klog.Infof("using local topology with Node = %+v and CSINode = %+v", node, csiNode)

// We make those fake objects available to the topology code via informers which

// never change.

stoppedFactory := informers.NewSharedInformerFactory(clientset, 1000*time.Hour)

csiNodes := stoppedFactory.Storage().V1().CSINodes()

nodes := stoppedFactory.Core().V1().Nodes()

csiNodes.Informer().GetStore().Add(csiNode)

nodes.Informer().GetStore().Add(node)

csiNodeLister = csiNodes.Lister()

nodeLister = nodes.Lister()

), so all errors related to access to those can be ruled out. Errors related to incorrect or incomplete content of those objects also cannot occur.

I think all that remains is an unsuitable selected node, and therefore we don't need a more detailed error.

After having described it like this, the code could also be replaced with a direct comparison of the selected node. I'm undecided whether that is better: if GenerateAccessibilityRequirements then fails after the node became owner (against all expectations), then the volume is stuck. More code would be needed to handle that, like detecting this particular problem earlier and/or recovering earlier.

Given that immediate binding isn't recommended when using distributed provisioning, I think I prefer to merge this simpler solution even if it doesn't cover some edge cases.

Thanks for explaining. It wasn't obvious to me from a quick review of the code that this is the case. I think from a general readability and maintenance standpoint, it's better to code more defensively in case the function changes in the future and we can no longer make this assumption.

I can add a specific error and check for it here.

A separate error code, or a new condition, or perhaps a more targeted function like you mentioned would all work. Out of all those, I like the separate method approach the most because there's no confusion about what parts of the function are/are not relevant depending on the codepath.

Anyway, I think we can leave this to a future refactoring. For now, this works with the current assumptions.

/approve

k8s-ci-robot · 2021-04-20T17:25:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly

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

k8s-ci-robot requested a review from chrishenzie April 19, 2021 13:31

pohly force-pushed the allowed-topologies branch from 9183218 to cfabf52 Compare April 19, 2021 13:34

k8s-ci-robot assigned verult Apr 19, 2021

chrishenzie reviewed Apr 19, 2021

View reviewed changes

pohly force-pushed the allowed-topologies branch from cfabf52 to d3f761e Compare April 19, 2021 17:17

verult reviewed Apr 19, 2021

View reviewed changes

pohly force-pushed the allowed-topologies branch from d3f761e to e8dfeb3 Compare April 19, 2021 19:57

verult reviewed Apr 19, 2021

View reviewed changes

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

pohly added 2 commits April 19, 2021 23:25

controller: test behavior when capacity is exhausted

f262e0f

ShouldProvision must not try to become the owner if capacity is insufficient. That was implemented already, just not tested.

pohly force-pushed the allowed-topologies branch from e8dfeb3 to f262e0f Compare April 19, 2021 21:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2021

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 Apr 19, 2021

k8s-ci-robot assigned msau42 Apr 19, 2021

msau42 reviewed Apr 19, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2021

k8s-ci-robot merged commit 1b9d55d into kubernetes-csi:master Apr 20, 2021

pohly mentioned this pull request Apr 20, 2021

distributed provisioning: dedicated check for "incompatible topology" #613

Open

chrishenzie mentioned this pull request Apr 20, 2021

Update release notes for v2.2.0 #614

Merged

pohly mentioned this pull request Apr 30, 2021

distributed-on-master failing kubernetes-csi/csi-driver-host-path#274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distributed provisioning: allowed topologies + immediate binding #612

distributed provisioning: allowed topologies + immediate binding #612

pohly commented Apr 19, 2021 •

edited by msau42

Loading

pohly commented Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

pohly commented Apr 19, 2021

chrishenzie commented Apr 19, 2021

chrishenzie Apr 19, 2021

pohly Apr 19, 2021

verult Apr 19, 2021

pohly Apr 19, 2021

verult Apr 19, 2021

pohly Apr 19, 2021

verult Apr 19, 2021

verult Apr 19, 2021

verult Apr 19, 2021 •

edited

Loading

verult Apr 19, 2021

pohly Apr 19, 2021

verult Apr 19, 2021

pohly Apr 19, 2021

verult Apr 19, 2021

verult Apr 19, 2021

pohly Apr 19, 2021

verult commented Apr 19, 2021

pohly commented Apr 19, 2021

verult commented Apr 19, 2021

verult commented Apr 19, 2021

msau42 Apr 19, 2021

pohly Apr 20, 2021

msau42 Apr 20, 2021

pohly Apr 20, 2021

msau42 Apr 20, 2021

pohly Apr 20, 2021

k8s-ci-robot commented Apr 20, 2021

	// Avoid watching in favor of fake, static objects. This is particularly relevant for
	// Node objects, which can generate significant traffic.
	csiNode := &storagev1.CSINode{
	ObjectMeta: metav1.ObjectMeta{
	Name: nodeDeployment.NodeName,
	},
	Spec: storagev1.CSINodeSpec{
	Drivers: []storagev1.CSINodeDriver{
	{
	Name: provisionerName,
	NodeID: nodeDeployment.NodeInfo.NodeId,
	},
	},
	},
	}
	node := &v1.Node{
	ObjectMeta: metav1.ObjectMeta{
	Name: nodeDeployment.NodeName,
	},
	}
	if nodeDeployment.NodeInfo.AccessibleTopology != nil {
	for key := range nodeDeployment.NodeInfo.AccessibleTopology.Segments {
	csiNode.Spec.Drivers[0].TopologyKeys = append(csiNode.Spec.Drivers[0].TopologyKeys, key)
	}
	node.Labels = nodeDeployment.NodeInfo.AccessibleTopology.Segments
	}
	klog.Infof("using local topology with Node = %+v and CSINode = %+v", node, csiNode)

	// We make those fake objects available to the topology code via informers which
	// never change.
	stoppedFactory := informers.NewSharedInformerFactory(clientset, 1000*time.Hour)
	csiNodes := stoppedFactory.Storage().V1().CSINodes()
	nodes := stoppedFactory.Core().V1().Nodes()
	csiNodes.Informer().GetStore().Add(csiNode)
	nodes.Informer().GetStore().Add(node)
	csiNodeLister = csiNodes.Lister()
	nodeLister = nodes.Lister()

distributed provisioning: allowed topologies + immediate binding #612

distributed provisioning: allowed topologies + immediate binding #612

Conversation

pohly commented Apr 19, 2021 • edited by msau42 Loading

pohly commented Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

pohly commented Apr 19, 2021

chrishenzie commented Apr 19, 2021

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

verult Apr 19, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Apr 19, 2021

pohly commented Apr 19, 2021

verult commented Apr 19, 2021

verult commented Apr 19, 2021

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

k8s-ci-robot commented Apr 20, 2021

pohly commented Apr 19, 2021 •

edited by msau42

Loading

verult Apr 19, 2021 •

edited

Loading