csi: fix volume updating behavior #18588
Conversation
fix for part of: c6dbba7 which allowed updating volumes while in use, because CSI expand may occur while in use. but it mistakenly stopped copying other important values that may be updated whether in use or not. this moves some of the in-use validation to happen during Merge(), before writing to state, leaving UpsertCSIVolume with only minimal final sanity-checking.
|
|
||
| // Update fields that are safe to change while volume is being used. | ||
| if err := old.UpdateSafeFields(v); err != nil { | ||
| return fmt.Errorf("unable to update in-use volume: %w", err) | ||
| } | ||
| v = old | ||
| v.ModifyIndex = index | ||
|
|
There was a problem hiding this comment.
It looks like we've moved the logic we wanted here into Merge, but then don't we need to add the call to Merge here?
There was a problem hiding this comment.
Nah, we validate and build up new CSIVolume(s) in the register/create flows via reconcileVolume -> Merge prior to UpsertCSIVolume here. Validation should always happen before getting this far, so "store this in state please" (this upsert method) only handles writing state (and a couple remaining validations above that I opted not to touch).
There was a problem hiding this comment.
It's that v = old line on 2436 that made any updating necessary here (via UpdateSafeFields), but that was the source of other intended updates not getting saved. Without v = old, old is used only for some last-minute sanity-checking, then the already-merged-earlier volume provided by the caller is what gets committed to state.
fix for part of: c6dbba7 (specifically around here)
which allowed updating volumes while in use,
because CSI expand may occur while in use.
but it mistakenly stopped copying other
important values that may be updated
whether in use or not.
this moves some of the in-use validation to
happen during Merge(), before writing to state,
leaving UpsertCSIVolume with only minimal final
sanity-checking.