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

Catch the deletion key before type assertion causes a panic #6419

Merged
merged 3 commits into from
Sep 20, 2021
Merged

Catch the deletion key before type assertion causes a panic #6419

merged 3 commits into from
Sep 20, 2021

Conversation

rthallisey
Copy link
Contributor

@rthallisey rthallisey commented Sep 15, 2021 •
edited

Handle cache.DeletedFinalStateUnknown keys in the virt-controller for VMIs.

fixes: #6418
Signed-off-by: Ryan Hallisey rhallisey@nvidia.com

Fix virt-controller panic caused by lots of deleted VMI events

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/S labels Sep 15, 2021
@rthallisey
Copy link
Contributor Author

/retest

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 15, 2021
rmohr
rmohr requested changes Sep 15, 2021
View reviewed changes
Copy link
Member

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

Nice catch. One change is needed so that we enqueue the right object to the workqueue.

pkg/virt-controller/watch/vmi.go Outdated Show resolved Hide resolved
pkg/virt-controller/watch/vmi.go Outdated Show resolved Hide resolved
pkg/virt-controller/watch/vmi.go Outdated Show resolved Hide resolved
@rmohr
Copy link
Member

rmohr commented Sep 15, 2021

@rthallisey could you add a release-note? I think this is worth a note so that other people running into panics can find this fix more easily.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 15, 2021
@rmohr
Copy link
Member

rmohr commented Sep 16, 2021

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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 Sep 16, 2021
@rthallisey
Catch the deletion key before type assertion causes a panic
d5ed1e3 
Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>

fixes: kubevirt#6418
Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>
@rthallisey
Re enqueue after the tombstone key has been seen
4594d3c 
Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2021
@rthallisey
Copy link
Contributor Author

rebased

@rthallisey
Move lower expectations back
7369518 
Signed-off-by: Ryan Hallisey <rhallisey@nvidia.com>
@rthallisey
Copy link
Contributor Author

/lgtm
re-adding lgtm after rebase

@kubevirt-bot
Copy link
Contributor

@rthallisey: you cannot LGTM your own PR.

In response to this:

/lgtm
re-adding lgtm after rebase

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.

@rthallisey
Copy link
Contributor Author

@rmohr @davidvossel PR is approved. Can you add lgtm?

davidvossel
davidvossel approved these changes Sep 20, 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

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2021
@kubevirt-bot kubevirt-bot merged commit f1830ba into kubevirt:main Sep 20, 2021
@rthallisey rthallisey deleted the controller-handle-deleted-keys branch September 20, 2021 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmohr rmohr rmohr requested changes

@davidvossel davidvossel davidvossel approved these changes

@EdDev EdDev Awaiting requested review from EdDev

@zcahana zcahana Awaiting requested review from zcahana

Assignees

@davidvossel davidvossel

@rmohr rmohr

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/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SIG-scale] At a large scale (1000+ VMIs), deleting a lot of VMIs can cause virt-controller to panic
4 participants
@rthallisey @rmohr @kubevirt-bot @davidvossel