CSI: Expand Volume (Controller only) #18359
Conversation
gulducat
force-pushed
the
csi-resize-redux
branch
from
6044d21 to
1ecc3aa
Compare
| CSIControllerQuery | ||
| } | ||
|
|
||
| func (req *ClientCSIControllerExpandVolumeRequest) ToCSIRequest() *csi.ControllerExpandVolumeRequest { |
There was a problem hiding this comment.
Do we need to perform a nil check on the req here, or is that always non-nil?
There was a problem hiding this comment.
I do see some other nil checks in similar methods in this file, so maybe there's a reason for them? The only place this one is called (currently) already assumes non-nil to get req.PluginID, so I think possible-nil-ness would be bad there too, unless checked prior to that? I'm not sure how much our RPC boundaries increase the possibility of that kind of nil pointer...
| if vol == nil || plugin == nil || capacity == nil { | ||
| return errors.New("unexpected nil value") | ||
| } |
There was a problem hiding this comment.
Do we want to expand (hehe) on this error? I wonder how these nil values can manifest, and if they are user related, whether we can provide a more actionable message.
There was a problem hiding this comment.
this should never happen -- this is my fear of panics showing. I actually wonder if a panic would be preferable? because at least then it'd provide a full stack trace if there is an unexpected bug...
but server panics are especially scary to me, and extra-especially in a spot like this, which might not get tripped until some poor operator is trying to do a reasonable thing but ends up taking down a whole server, which might be concurrently doing other important things. 😬
| @@ -228,7 +231,7 @@ func (v *CSIVolume) Get(args *structs.CSIVolumeGetRequest, reply *structs.CSIVol | |||
| return v.srv.blockingRPC(&opts) | |||
| } | |||
|
|
|||
| func (v *CSIVolume) pluginValidateVolume(req *structs.CSIVolumeRegisterRequest, vol *structs.CSIVolume) (*structs.CSIPlugin, error) { | |||
| func (v *CSIVolume) pluginValidateVolume(vol *structs.CSIVolume) (*structs.CSIPlugin, error) { | |||
There was a problem hiding this comment.
Now that I'm looking at this signature, I'm realizing this is a very silly method name. I suspect at one time it did validate the volume somehow, but we never changed the name?
gulducat
force-pushed
the
csi-resize-redux
branch
from
1ecc3aa to
713b270
Compare
gulducat
force-pushed
the
csi-resize-redux
branch
from
8ca10be to
e7d689f
Compare
the first half of volume expansion,
this allows a user to update requested capacity
("capacity_min" and "capacity_max") in a volume
specification file, and re-issue either Register
or Create volume commands (or api calls).
the requested capacity will now be "reconciled"
with the current real capacity of the volume,
issuing a ControllerExpandVolume RPC call
to a running controller plugin, if requested
"capacity_min" is higher than the current
capacity on the volume in state.
csi spec:
https://github.com/container-storage-interface/spec/blob/c918b7f/spec.md#controllerexpandvolume
note: this does not yet cover NodeExpandVolume
gulducat
force-pushed
the
csi-resize-redux
branch
from
e7d689f to
f71a248
Compare
Addresses #10324 with declarative config (instead of a new imperative command/api route).
I still need to update docs, but to trigger a volume increase, modify the volume specification that was used initially to create/register the volume with a higher
capacity_min(and optionallycapacity_max), then issuecreateorregisteragain.This introduces a comparatively very slow process into the create/register flow (if expand is actually needed), and modifies the behavior of
createto be more idempotent than it currently is (presently it always issues create to the controller plugin).I hemmed and hawed a lot on where the validation logic should live, so I'd be happy to hear any suggestions to move any of it. I mainly want to avoid putting it right before writing to raft, because by that point reality (a real live disk in the world) may have already changed.
The commit history is messy right now; I intend to squash it before merging.
So far this covers
ControllerExpandVolumeto increase volumes outside of the instance (e.g. an AWS EBS volume), but does not yet complete the process (when required) withNodeExpandVolumeon the node where the volume is mounted. I think I'll make that a separate PR.