Skip to content

Move ControllerModifyVolume to GA #588

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

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

Move ControllerModifyVolume to GA #588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions csi.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ service Controller {

rpc ControllerModifyVolume (ControllerModifyVolumeRequest)
returns (ControllerModifyVolumeResponse) {
option (alpha_method) = true;
}
}

Expand Down Expand Up @@ -413,7 +412,7 @@ message CreateVolumeRequest {
// as if they take precedence over the parameters field.
// This field SHALL NOT be specified unless the SP has the
// MODIFY_VOLUME plugin capability.
map<string, string> mutable_parameters = 8 [(alpha_field) = true];
map<string, string> mutable_parameters = 8;
}

// Specifies what source the volume will be created from. One of the
Expand Down Expand Up @@ -890,7 +889,7 @@ message ValidateVolumeCapabilitiesRequest {

// See CreateVolumeRequest.mutable_parameters.
// This field is OPTIONAL.
map<string, string> mutable_parameters = 6 [(alpha_field) = true];
map<string, string> mutable_parameters = 6;
}

message ValidateVolumeCapabilitiesResponse {
Expand All @@ -909,7 +908,7 @@ message ValidateVolumeCapabilitiesResponse {

// The volume creation mutable_parameters validated by the plugin.
// This field is OPTIONAL.
map<string, string> mutable_parameters = 4 [(alpha_field) = true];
map<string, string> mutable_parameters = 4;
}

// Confirmed indicates to the CO the set of capabilities that the
Expand Down Expand Up @@ -1021,8 +1020,6 @@ message ControllerGetVolumeResponse {
VolumeStatus status = 2;
}
message ControllerModifyVolumeRequest {
option (alpha_message) = true;

Copy link
Contributor

@huww98 huww98 Jul 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Before we promote this to GA, can we change mutable_parameters to OPTIONAL? And add words like:

When not specified, SPs MAY cancel previously requested in-progress modifications.
SPs SHOULD return success only when the volume is stable and no modifications on going.

This extends the use-case and can be a breaking change for SP. But the semantic should be clear and straightforward.

This can be useful for Kubernetes when reverting volumeAttributeClassName to empty, to ensure we've reached a stable state.

And if we add topology support in the future, passing empty mutable_parameters can be useful for CO to fetch the current topology of the volume without actually modify it, also useful in the reverting volumeAttributeClassName to empty case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we want to do that? We made it explicitly as something we need for Modify. And if you need a cancelling operation, we should have a CancelOperation API instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

And if you need a cancelling operation, we should have a CancelOperation API instead?

The key point is not canceling, but waiting for a stable state, to avoid left a mess behind. For use-case like kubernetes/kubernetes#132106 . In the PR description:

rollback from an infeasible pvc.spec.VacName to no VAC

However, there is no concept like infeasible error in the CSI spec. I think the only way we can sure that the volume returns to a stable state is retry the RPC until it returns success.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we want to support topology in the future (kubernetes/enhancements#5381), the current proposal is allowing setting new topology before modify success. So what if the topology has change and we want to rollback? We will need a way to fetch the current topology of the volume without actually modify it.

Of course we can validate the parameters before change topology. But there are still possibilities that the operation may fail half-way.

  • We may change multiple aspect of the volume in the same RPC call, e.g. disk tags, disk type, performance level. Some of them may success, but others may still fail.
  • The validation rules embedded in the CSI can be outdated.
  • Modification can be slow. There maybe other operator modifying the same volume concurrently, invalidating the previously valid request.

Or should we just say: SPs should only return OK or Abort if any part of the modification succeeded? But I'm afraid that SP may also lost track of previously failed modification tasks. SP may also not able to distinct the partial or complete failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, there is no concept like infeasible error in the CSI spec. I think the only way we can sure that the volume returns to a stable state is retry the RPC until it returns success.

For most errors we do not allow going back to older state. We have pretty well defined semantic of what we consider infeasible in k8s+csi - https://github.com/kubernetes-csi/external-resizer/blob/master/pkg/util/util.go#L274 .

We do not have an API to cancel any of in-progress CSI operations and that has worked well enough.

// Contains identity information for the existing volume.
// This field is REQUIRED.
string volume_id = 1;
Expand All @@ -1044,7 +1041,6 @@ message ControllerModifyVolumeRequest {
}

message ControllerModifyVolumeResponse {
option (alpha_message) = true;
}

message GetCapacityRequest {
Expand Down Expand Up @@ -1189,7 +1185,7 @@ message ControllerServiceCapability {

// Indicates the SP supports modifying volume with mutable
// parameters. See ControllerModifyVolume for details.
MODIFY_VOLUME = 14 [(alpha_enum_value) = true];
MODIFY_VOLUME = 14;

// Indicates the SP supports the GetSnapshot RPC.
// This enables COs to fetch an existing snapshot.
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ module github.com/container-storage-interface/spec
go 1.18

require (
github.com/golang/protobuf v1.5.4
google.golang.org/grpc v1.57.1
google.golang.org/protobuf v1.33.0
)

require (
github.com/golang/protobuf v1.5.3 // indirect
golang.org/x/net v0.23.0 // indirect
golang.org/x/sys v0.18.0 // indirect
golang.org/x/text v0.14.0 // indirect
Expand Down
10 changes: 2 additions & 8 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk=
github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY=
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek=
github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps=
github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
golang.org/x/net v0.23.0 h1:7EYJ93RZ9vYSZAIb2x3lnuvqO5zneoD6IvWjuhfxjTs=
golang.org/x/net v0.23.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
golang.org/x/sys v0.18.0 h1:DBdB3niSjOA/O0blCZBqDefyWNYveAYMNF1Wum0DYQ4=
golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ=
golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
google.golang.org/genproto/googleapis/rpc v0.0.0-20230803162519-f966b187b2e5 h1:eSaPbMR4T7WfH9FvABk36NBMacoTUKdWCvV0dx+KfOg=
google.golang.org/genproto/googleapis/rpc v0.0.0-20230803162519-f966b187b2e5/go.mod h1:zBEcrKX2ZOcEkHWxBPAIvYUWOKKMIhYcmNiUIu2ji3I=
google.golang.org/grpc v1.57.1 h1:upNTNqv0ES+2ZOOqACwVtS3Il8M12/+Hz41RCPzAjQg=
google.golang.org/grpc v1.57.1/go.mod h1:Sd+9RMTACXwmub0zcNY2c4arhtrbBYD1AUHI/dt16Mo=
google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI=
google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=

Loading