Fix volume detach on hotplug attachment pod delete #10165
Conversation
kubevirt-bot
added
release-note
awels
force-pushed
the
dont_detach_deleted_attachment_pod
branch
from
ff2c1cb
to
203e56d
Compare
When the hotplug attachment pod is deleted, the VMI volumestatus goes back to bound, which triggers the manager to detach the volume from the running VM interrupting any IO on that volume. The pod is then re-created and the volume gets re-attached and operation can continue, but if the volume is mounted, it needs to be re-mounted in the VM. This commit modifies the logic so that if the volume is ready, it cannot go back to bound if the attachment pod disappears. This prevents the detachments and issues with IO on the running VM. Signed-off-by: Alexander Wels <awels@redhat.com>
awels
force-pushed
the
dont_detach_deleted_attachment_pod
branch
from
203e56d
to
bc0073f
Compare
|
/test pull-kubevirt-unit-test |
pkg/virt-controller/watch/vmi.go
Outdated
| return currentPhase == "" || currentPhase == virtv1.VolumeBound || currentPhase == virtv1.VolumePending || | ||
| currentPhase == virtv1.HotplugVolumeAttachedToNode | ||
| return (currentPhase == "" || currentPhase == virtv1.VolumeBound || currentPhase == virtv1.VolumePending || | ||
| currentPhase == virtv1.HotplugVolumeAttachedToNode) && currentPhase != virtv1.VolumeReady |
There was a problem hiding this comment.
This seems kind of weird, how does adding currentPhase != virtv1.VolumeReady change anything? currentPhase cannot be any of the explicit values checked AND virtv1.VolumeReady.
I think a switch statement with all the valid values enumerated may be the cleanest
There was a problem hiding this comment.
You make a good point, they are mutually exclusive.
pkg/virt-controller/watch/vmi.go
Outdated
| status.Phase = phase | ||
| status.Message = message | ||
| status.Reason = reason | ||
| if status.Phase != virtv1.VolumeReady { |
There was a problem hiding this comment.
sure that's the only value we care about? also would it make sense to create a helper function instead of explicit check?
There was a problem hiding this comment.
Yes I am sure, all the other phases the volume is not mounted in the VM and switching phase won't matter. But I can definitely make it a helper. Might explain the intent a little better.
Remove unneeded phase check, and move other check into its own function in case we need more elaborate checks Signed-off-by: Alexander Wels <awels@redhat.com>
|
/retest-required |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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 |
kubevirt-bot
added
the
approved
|
/cherrypick release-1.0 |
|
@awels: new pull request created: #10176 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
When the hotplug attachment pod is deleted, the VMI volumestatus goes back to bound, which triggers the manager to detach the volume from the running VM
interrupting any IO on that volume. The pod is then re-created and the volume gets re-attached and operation can continue, but if the volume is mounted, it needs to be re-mounted in the VM.
This commit modifies the logic so that if the volume is ready, it cannot go back to bound if the attachment pod disappears. This prevents the detachments and issues with IO on the running VM.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: