Prevent calling controller-expand if volume in-use #86
Conversation
k8s-ci-robot
added
the
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied 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 |
k8s-ci-robot
added
approved
pkg/controller/controller.go
Outdated
|
|
||
| func (ctrl *resizeController) deletePod(obj interface{}) { | ||
| pod := parsePod(obj) | ||
| if pod != nil { |
| @@ -98,17 +112,38 @@ func NewResizeController( | |||
| DeleteFunc: ctrl.deletePVC, | |||
| }, resyncPeriod) | |||
|
|
|||
| podInformer.Informer().AddEventHandlerWithResyncPeriod(cache.ResourceEventHandlerFuncs{ | |||
There was a problem hiding this comment.
I see that the in-tree PVC protection controller also checks the pod. What is the advantage of checking thru the pod vs checking the VolumeAttachment object?
There was a problem hiding this comment.
Because not all volume types support attach/detach and hence there may not be an VolumeAttachment object.
pkg/controller/controller.go
Outdated
| if ctrl.usedPVCs.hasInUseErrors(pvc) && ctrl.usedPVCs.checkForUse(pvc) { | ||
| // Record an event to indicate that resizer is not expanding the pvc | ||
| ctrl.eventRecorder.Event(pvc, v1.EventTypeWarning, util.VolumeResizeFailed, | ||
| fmt.Sprintf("CSI resizer is not expanding %s because it is in-use", pv.Name)) |
There was a problem hiding this comment.
Can you add a message to indicate that the CSI driver only supports offline volume expansion.
pkg/controller/controller.go
Outdated
| } | ||
| pod, ok := obj.(*v1.Pod) | ||
| if !ok { | ||
| tombstone, ok := obj.(cache.DeletedFinalStateUnknown) |
There was a problem hiding this comment.
Can you pick a different name than "tombstone"?
| return false | ||
| } | ||
| // if this is a failed precondition error then that means driver does not support expansion | ||
| // of in-use volumes |
There was a problem hiding this comment.
Can you add a comment linking to the CSI spec description about this?
k8s-ci-robot
added
size/XL
gnufied
force-pushed
the
add-volume-in-use-error
branch
from
1bc70d8 to
38e5ebf
Compare
|
@xing-yang PTAL. addressed last comments. |
|
/retest |
| pvcInUse: true, | ||
| }, | ||
| { | ||
| Name: "Resize PVC, no FS resize, pvc-inuse but no failedprecondition error", |
There was a problem hiding this comment.
This one should be "with FS resize" as there is no failedprecondition?
There was a problem hiding this comment.
it should not matter. The "FS resize" only decides whether node expansion is pending or not. But since in this case, all we are trying to test is - if a pvc was in-use but has no failedprecondition from before then expansion should proceed as usual.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
If volume can not be expanded when in-use we must not call ControllerExpandVolume until we know volume is not in-use.
Fixes #81