Introduce new flag - strict-topology #282

avalluri · 2019-05-21T12:56:50Z

With the current implementation, In delayed binding case, CSI driver is offered
with all nodes topology that are matched with 'selected node' topology keys in
CreateVolumeRequest.AccessibilityRequirements. So this allows the driver to
select any node from the passed preferred/requisite list to create volume. But this
results in scheduling failure when the volume created on a node other than
Kubernetes selected node.

To address this, introduced new flag "--strict-topology', when set, in case of
delayed binding, the driver is offered with only selected node topology, so that
the driver has to create the volume on this node.

This new flag can be used by drivers that support strict topology for volumes with delayed binding.

What type of PR is this?
/kind bug
/kind design

What this PR does / why we need it:
In case of delayed binding, creating volume on the different topology that is not accessed by the Kubernetes selected node for Pod scheduling, leads to unresolvable scheduling failures. So we should not allow the driver to create such volumes. We can avoid this by passing right/strict accessibility topologies instead of 'aggregated topology' to CreateVolume request.

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

Does this PR introduce a user-facing change?:
Yes

support strict topology for volumes with delayed binding

k8s-ci-robot · 2019-05-21T12:56:51Z

Welcome @avalluri!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-provisioner has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-05-21T12:57:05Z

Hi @avalluri. Thanks for your PR.

I'm waiting for a kubernetes-csi or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pohly

Can you add a README.md section which explains the topology support and add something for the new mode there?

pkg/controller/topology_test.go

pkg/controller/topology.go

avalluri · 2019-05-23T08:48:26Z

Can you add a README.md section which explains the topology support and add something for the new mode there?

@pohly I tried adding a section to ReadMe that explains how AccessibilityRequirements are prepared. Can you please have a look if it is good enough.

README.md

davidz627 · 2019-05-28T21:04:32Z

/cc @msau42 @verult

pohly · 2019-05-29T12:24:22Z

/retest

pohly

The description looks good to me now. For the code I'll defer to someone who is more familiar with it.

One more thing. Can you add a

support strict topology for volumes with delayed binding

to the PR description?

README.md

davidz627 · 2019-05-29T18:18:28Z

/ok-to-test

msau42

This generally lgtm! Thanks for working on this! Can you also add a release note to the initial comment describing the new option?

/approve

msau42 · 2019-06-04T00:24:39Z

cmd/csi-provisioner/csi-provisioner.go


 	enableLeaderElection = flag.Bool("enable-leader-election", false, "Enables leader election. If leader election is enabled, additional RBAC rules are required. Please refer to the Kubernetes CSI documentation for instructions on setting up these RBAC rules.")
 	leaderElectionType   = flag.String("leader-election-type", "endpoints", "the type of leader election, options are 'endpoints' (default) or 'leases' (strongly recommended). The 'endpoints' option is deprecated in favor of 'leases'.")
+	strictTopology       = flag.Bool("strict-topology", false, "Passes only selected node topology to CreateVolume Request, unlike default behavior of passing all nodes that match with topology keys of the selected node.")


To match the wording in the README:

"passing all nodes" => "passing aggregated cluster topologies"

msau42 · 2019-06-04T00:44:46Z

pkg/controller/topology.go

-		if err != nil {
-			return nil, err
+	if selectedCSINode != nil && strictTopology {
+		// Make sure that selected node topology is in allowed topologies list


We could probably be more efficient and just assume Kubernetes does the right thing.

I agree, but there is a test "topology from selected node is not in allowedTopologies" for this, so I added this check to satisfy the test.

msau42 · 2019-06-04T00:52:04Z

pkg/controller/topology.go

-		requisiteTerms, err = aggregateTopologies(kubeClient, driverName, selectedCSINode)
-		if err != nil {
-			return nil, err
+	if selectedCSINode != nil && strictTopology {


Can this be changed to a switch statement with the other 2 conditions, since all 3 are mutually exclusive from each other?

Those are not mutually exclusive. It's possible that both allowedTopologies and selectedNode set, and resulted topology depends on strictTopology value.

I could move this block to inside above if selectedNode != nil {..}

k8s-ci-robot · 2019-06-04T00:56:09Z

[APPROVALNOTIFIER] This PR is APPROVED

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

With the current implementation, In delayed binding case, CSI driver is offered with all nodes topology that are matched with 'selected node' topology keys in CreateVolumeRequest.AccessibilityRequirements. So this allows the driver to select any node from the passed preferred list to create volume. But this results in scheduling failure when the volume created on a node other than Kubernetes selected node. To address this, introduced new flag "--strict-topology', when set, in case of delayed binding, the driver is offered with only selected node topology, so that driver has to create the volume on this node. Modified tests so that now every test is run with and without 'strict topology'.

msau42 · 2019-06-04T21:35:23Z

/lgtm

Our recent change (kubernetes-csi/external-provisioner#282) got merged to master which fixes late binding case. Till it appears in next release(v1.2) we use canary builds which holds this change.

…strict-topology With the current implementation, In delayed binding case, CSI driver is offered with all nodes topology that are matched with 'selected node' topology keys in CreateVolumeRequest.AccessibilityRequirements. So this allows the driver to select any node from the passed preferred list to create volume. But this results in scheduling failure when the volume created on a node other than Kubernetes selected node. To address this, introduced new flag "--strict-topology', when set, in case of delayed binding, the driver is offered with only selected node topology, so that driver has to create the volume on this node. Modified tests so that now every test is run with and without 'strict topology'.

Our recent change (kubernetes-csi/external-provisioner#282) got merged to master which fixes late binding case. Till it appears in next release(v1.2) we use canary builds which holds this change.

Bump deps to v0.20.0

5f38a9075 Merge pull request kubernetes-csi#282 from rhrmo/update-go-1.24.6 579f62421 Update go to 1.24.6 74e066a82 Merge pull request kubernetes-csi#279 from Aishwarya-Hebbar/update-csi-prow-version 6f236be7d Update CSI prow driver version to v1.17.0 0ee55894b Merge pull request kubernetes-csi#280 from xing-yang/update_go_1.24.4 9af101534 update to go 1.24.4 f5fec3e36 Merge pull request kubernetes-csi#275 from chrishenzie/emeritus c5d285db8 Remove chrishenzie from kubernetes-csi-reviewers git-subtree-dir: release-tools git-subtree-split: 5f38a907597230563f5e2213aea116acdd9d86bc

74502e544 Merge pull request kubernetes-csi#278 from liangyuanpeng/migrate_k8s_testimages 533443055 Merge pull request kubernetes-csi#281 from kubernetes-csi/dependabot/github_actions/actions/checkout-5 458ce146f Bump actions/checkout from 4 to 5 5f38a9075 Merge pull request kubernetes-csi#282 from rhrmo/update-go-1.24.6 579f62421 Update go to 1.24.6 5ec1a52b8 use gcr.io/k8s-staging-test-infra instead of gcr.io/k8s-testimages 74e066a82 Merge pull request kubernetes-csi#279 from Aishwarya-Hebbar/update-csi-prow-version 6f236be7d Update CSI prow driver version to v1.17.0 0ee55894b Merge pull request kubernetes-csi#280 from xing-yang/update_go_1.24.4 9af101534 update to go 1.24.4 f5fec3e36 Merge pull request kubernetes-csi#275 from chrishenzie/emeritus c5d285db8 Remove chrishenzie from kubernetes-csi-reviewers git-subtree-dir: release-tools git-subtree-split: 74502e544bc6a17820892c0d490e8f0b59462998

k8s-ci-robot requested review from davidz627 and lpabon May 21, 2019 12:57

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2019

avalluri mentioned this pull request May 21, 2019

choosing node for volume(s) intel/pmem-csi#215

Closed

pohly requested changes May 21, 2019

View reviewed changes

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

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

avalluri force-pushed the fix-late-binding branch from 8dea010 to 050a6e5 Compare May 22, 2019 15:33

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 22, 2019

avalluri force-pushed the fix-late-binding branch from 050a6e5 to 5b784fb Compare May 23, 2019 08:43

avalluri force-pushed the fix-late-binding branch 4 times, most recently from b6b30b0 to b970817 Compare May 24, 2019 08:40

pohly requested changes May 24, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

avalluri force-pushed the fix-late-binding branch from b970817 to bfda61a Compare May 25, 2019 20:54

k8s-ci-robot requested review from msau42 and verult May 28, 2019 21:04

pohly requested changes May 29, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

avalluri force-pushed the fix-late-binding branch from bfda61a to 508be1a Compare May 29, 2019 12:41

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 29, 2019

msau42 reviewed Jun 4, 2019

View reviewed changes

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

avalluri changed the title ~~RFC: Introduce new flag - strict-topology~~ Introduce new flag - strict-topology Jun 4, 2019

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 Jun 4, 2019

avalluri force-pushed the fix-late-binding branch from 508be1a to 5bd554b Compare June 4, 2019 09:50

k8s-ci-robot assigned msau42 Jun 4, 2019

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

k8s-ci-robot merged commit 1730a1e into kubernetes-csi:master Jun 4, 2019

avalluri mentioned this pull request Jun 6, 2019

[release-1.2] Introduce new flag - strict-topology #288

Merged

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

Merge pull request kubernetes-csi#282 from jsafrane/bump-deps-to-v0-20-0

61c2e47

Bump deps to v0.20.0

darshansreenivas mentioned this pull request Oct 15, 2025

CSI Release tools update #1426

Merged

Introduce new flag - strict-topology #282

Introduce new flag - strict-topology #282

Uh oh!

Conversation

avalluri commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 21, 2019

Uh oh!

k8s-ci-robot commented May 21, 2019

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

avalluri commented May 23, 2019

Uh oh!

Uh oh!

Uh oh!

davidz627 commented May 28, 2019

Uh oh!

pohly commented May 29, 2019

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidz627 commented May 29, 2019

Uh oh!

msau42 left a comment

Choose a reason for hiding this comment

Uh oh!

msau42 Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

msau42 Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

avalluri Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

msau42 Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

avalluri Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 4, 2019

Uh oh!

msau42 commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

avalluri commented May 21, 2019 •

edited

Loading