Skip to content

Modify ControllerModifyVolume to support KEP-5381 #592

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iltyty
Copy link

@iltyty iltyty commented Jun 25, 2025

What type of PR is this?
Feature

What this PR does / why we need it:
This PR adds new fields to ControllerModifyVolumeRequest and ControllerModifyVolumeResponse to support KEP #5381.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This is an alternative for PR #593.

Does this PR introduce an API-breaking change?:

none

@iltyty iltyty changed the title Allow modifying volume topology Modify ControllerModifyVolume to support KEP-5381 Jun 25, 2025
@iltyty iltyty force-pushed the kep-5381-new-field branch 2 times, most recently from d84879a to 56e2f6e Compare July 1, 2025 14:48
@saad-ali
Copy link
Member

Can you specify the specific use cases that we are trying to solve here? And what are the use cases that are explicitly not supported?

@iltyty
Copy link
Author

iltyty commented Jul 21, 2025

Can you specify the specific use cases that we are trying to solve here? And what are the use cases that are explicitly not supported?

We've already mentioned this in KEP 5381, please refer to here: kubernetes/enhancements#5382.

@gnufied
Copy link
Contributor

gnufied commented Jul 22, 2025

I think adding more capability (or features) to existing ControllerModifyVolume should be avoided. ControllerModifyVolume could become swiss-army RPC (it already partially is) of all CSI rpc calls.

SP A uses it to apply, iops and throughput, SP B uses it to apply tags to volumes, SP C - uses it to apply iops, througput and now change topology. SP C - could use it for all the above and in stages.

The problem is, when a RPC call does too many things - it is very easy to get implementation wrong. It is very hard for CO to reason about, whether the operation has succeeded or failed. It also opens room for partial application of one or more properties which is undesirable, in case user wants CO to cancel previously issued operation (or at least stop trying).

While CSI RPC calls are not atomic, it is better if they do one thing and one thing only. It is better if CO knows what they are doing and has explicit state to track the operation (because drivers are supposed to be stateless).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants