Skip to content
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

Jump to bottom

Add a WaitingForVolumeBinding printable status #6713

Merged
merged 4 commits into from
Nov 7, 2021
Merged

Add a WaitingForVolumeBinding printable status #6713

merged 4 commits into from
Nov 7, 2021

Conversation

zcahana
Copy link
Contributor

@zcahana zcahana commented Nov 2, 2021

What this PR does / why we need it:

This PR extends the VM status field (.status.printableStatus) with a new WaitingForVolumeBinding status, reported when a VM with a PVC/DV type of volume is started, and the backing PVC is not bound.

This PR replaces #6454 and #6605. In contrast to #6454, it avoids presenting the status as an error, as well as heuristics to detect when binding failures occur - and simply make it visible when the VM cannot start due to unbound volumes (be it a short intermediary state, or a long lasting state that requires intervention). Also, in line with the discussion in #6605, it ensures that stopped VMs don't report WaitingForVolumeBinding nor Provisioning.

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 https://bugzilla.redhat.com/show_bug.cgi?id=2013160

Special notes for your reviewer:

Release note:

Report WaitingForVolumeBinding VM status when PVC/DV-type volumes reference unbound PVCs

@zcahana
Add a WaitingForVolumeBinding printable status
9db7d72 
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
@zcahana
Use getDataVolumeFromCache() across VM controller
d62b722 
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Nov 2, 2021
@zcahana
Copy link
Contributor Author

zcahana commented Nov 2, 2021

/assign @davidvossel @awels

@zcahana zcahana force-pushed the volume_binding_status branch 2 times, most recently from 9fbbb84 to ffc0b87 Compare November 2, 2021 15:31
davidvossel
davidvossel reviewed Nov 3, 2021
View reviewed changes
pkg/virt-controller/watch/vm.go Outdated Show resolved Hide resolved
pkg/virt-controller/watch/vm.go
Comment on lines +1806 to +1809
if pvc.Status.Phase != k8score.ClaimBound {
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

does waiting for first consumer need to be accounted for here?

Copy link
Contributor Author

@zcahana zcahana Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually no, since the WaitingForVolumeBinding status is considered only when the VM is set to start:

func (c *VMController) isVirtualMachineStatusWaitingForVolumeBinding(vm *virtv1.VirtualMachine, vmi *virtv1.VirtualMachineInstance) bool {
    if !isSetToStart(vm, vmi) {
        return false
    }
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, there is some time period in which the VM is set to start, and the virt-launcher pod hasn't been scheduled yet. In this case, from the perspective of the storage subsystem, there's still no consumer for the PVC. But I think this is negligible enough and we can report that (typically very short) time period as WaitingForVolumeBinding too. If we ever add a Scheduling status, this can replace it then.

Copy link
Member

Choose a reason for hiding this comment

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

ah, i see. So the running status will match the printable status first in this case. I think this is all still accurate even with WFFC.

@zcahana
Avoid WaitingForVolumeBinding when VM is stopped
f515232 
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
@zcahana
Unit tests for WaitingForVolumeBinding
cc59019 
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
davidvossel
davidvossel approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2021
@davidvossel
Copy link
Member

/retest

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

8 similar comments
@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit 16d28c6 into kubevirt:main Nov 7, 2021
@zcahana zcahana deleted the volume_binding_status branch November 7, 2021 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidvossel davidvossel davidvossel approved these changes

@AlonaKaplan AlonaKaplan Awaiting requested review from AlonaKaplan

@jean-edouard jean-edouard Awaiting requested review from jean-edouard

Assignees

@davidvossel davidvossel

@awels awels

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zcahana @kubevirt-bot @davidvossel @kubevirt-commenter-bot @awels