Skip to content

[cinder-csi-plugin] Don't report topology capability when --with-topology=False #2862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 4, 2025
Merged

[cinder-csi-plugin] Don't report topology capability when --with-topology=False #2862

merged 3 commits into from
Apr 4, 2025

Conversation

stephenfin
Copy link
Member

@stephenfin stephenfin commented Mar 24, 2025 •
edited
Loading

What this PR does / why we need it:

The PluginCapability_Service_VOLUME_ACCESSIBILITY_CONSTRAINTS capability flag determines whether the provisioner attempts to look up topology information from the node or not 1 2. If we report it but don't return topology information from the node, we end up with failures to provision 3. Fix the issue by optionally reporting the capability, like Manila already does.

In addition to the main fix, we also duplicate some helpful logs from Cinder to Manila, plus we fix a minor issue in the startup logging for both. More information on all these changes is provided in the commits.

Which issue this PR fixes(if applicable):

fixes #2861

Special notes for reviewers:

Release note:

NONE

@stephenfin
cinder-csi-plugin, manila-csi-plugin: Correct formatting character
0c4782d 
%T prints the type. %t prints the word true/false, which is what we
want. [1]

[1] https://pkg.go.dev/fmt

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
cinder-csi-plugin: Don't report topology capability when --with-topol…
71f521f 
…ogy=False

The 'PluginCapability_Service_VOLUME_ACCESSIBILITY_CONSTRAINTS'
capability flag determines whether the provisioner attempts to look up
topology information from the node or not [1][2]. If we report it but
don't return topology information from the node, we end up with
failures to provision [3]. Fix the issue by optionally reporting the
capability, like Manila already does.

[1] https://github.com/kubernetes-csi/external-provisioner/blob/17e2429e9f/pkg/controller/controller.go#L685-L700
[2] https://github.com/kubernetes-csi/external-provisioner/blob/17e2429e9f/pkg/controller/controller.go#L994-L996
[3] https://github.com/kubernetes-csi/external-provisioner/blob/17e2429e9f/pkg/controller/topology.go#L177

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
manila-csi-plugin: Unify some logging between cinder, manila
7aa34cd 
Signed-off-by: Stephen Finucane <[email protected]>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 24, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 24, 2025
@stephenfin
Copy link
Member Author

/cc @kayrus

@k8s-ci-robot k8s-ci-robot requested a review from kayrus March 24, 2025 17:45
@stephenfin stephenfin mentioned this pull request Mar 24, 2025
Merged
mdbooth
mdbooth reviewed Apr 1, 2025
View reviewed changes
@mdbooth
Copy link
Contributor

mdbooth commented Apr 1, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2025
mandre
mandre reviewed Apr 3, 2025
View reviewed changes
Copy link
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@dulek
Copy link
Contributor

dulek commented Apr 4, 2025

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2025
@k8s-ci-robot k8s-ci-robot merged commit f78589a into kubernetes:master Apr 4, 2025
8 checks passed
@stephenfin stephenfin deleted the fix-cinder-with-topology-flag branch April 4, 2025 09:52
@stephenfin
Copy link
Member Author

/cherry-pick release-1.31

@stephenfin
Copy link
Member Author

/cherry-pick release-1.32

@k8s-infra-cherrypick-robot
Copy link

@stephenfin: #2862 failed to apply on top of branch "release-1.31":

Applying: cinder-csi-plugin, manila-csi-plugin: Correct formatting character
Using index info to reconstruct a base tree...
M	pkg/csi/cinder/driver.go
M	pkg/csi/manila/driver.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/csi/manila/driver.go
CONFLICT (content): Merge conflict in pkg/csi/manila/driver.go
Auto-merging pkg/csi/cinder/driver.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 cinder-csi-plugin, manila-csi-plugin: Correct formatting character

In response to this:

/cherry-pick release-1.31

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-sigs/prow repository.

@k8s-infra-cherrypick-robot
Copy link

@stephenfin: new pull request created: #2871

In response to this:

/cherry-pick release-1.32

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-sigs/prow repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Apr 4, 2025
Merged
@stephenfin stephenfin mentioned this pull request Apr 4, 2025
Merged
This was referenced May 8, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulek dulek Awaiting requested review from dulek

@gouthampacha gouthampacha Awaiting requested review from gouthampacha

@kayrus kayrus Awaiting requested review from kayrus

2 more reviewers

@mandre mandre mandre left review comments

@mdbooth mdbooth mdbooth left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@mandre mandre

@mdbooth mdbooth

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[cinder-csi-plugin] Setting --with-topology=false on controller causes provisioner errors
6 participants
@stephenfin @mdbooth @dulek @k8s-ci-robot @k8s-infra-cherrypick-robot @mandre