virt-launcher, Fix Guest Agent updates causing an event handling deadlock #4253

oshoval · 2020-09-24T07:36:56Z

When guest-agent is installed, it will send update events,
which will send events to a channel of virt-launcher.
This channel has two purposes only:

Catch the first ADD event.
Catch the delete event.

Since this channel is sampled for checking of delete event
only after the qemu PID is gone, this channel can get full, in case wrong events are sent to it.

In such case the event handler (eventCallback) will hang forever
(because it tries to send to a full channel).
Guest agent wont be restarted and events wont be handled.

Since we just need ADD events and DELETE events
we dont need to send events to this channel upon
guest agent updates.

Moreover there was a double send to the virt-handler
which isnt needed as well.

Add mechanism to avoid sending unnecessary events to the channel.

Found as part of working on
https://bugzilla.redhat.com/show_bug.cgi?id=1874812

Signed-off-by: Or Shoval oshoval@redhat.com

None

pkg/virt-launcher/notify-client/client.go

…lock When guest-agent is installed, it will send update events, which will send events to a channel of virt-launcher. This channel has two meanings only: 1. Catch the first ADD event. 2. Catch the delete event. Since this channel is sampled for checking of delete event only after the qemu PID is gone, this channel can get full. In such case the event handler (eventCallback) will hang forever. Guest agent wont be restarted and events wont be handled. Since we just need ADD events and DELETE events we dont need to send events to this channel upon guest agent updates. Signed-off-by: Or Shoval <oshoval@redhat.com>

There is no need to send the event twice to virt-handler Signed-off-by: Or Shoval <oshoval@redhat.com>

The log also is not right, it should be logged only in case the osInfo indeed changed, and it doesnt add important info beside that we are in this section, and we have prints before and after it anyway. Signed-off-by: Or Shoval <oshoval@redhat.com>

davidvossel

good catch.

maybe we should make a wrapper function around the events <- event logic that ensures we only send the event to that channel if it is an ADD event or a MODIFY event with a domain with DeletionTimestamp != nil.

oshoval · 2020-09-24T16:07:11Z

good catch.

maybe we should make a wrapper function around the events <- event logic that ensures we only send the event to that channel if it is an ADD event or a MODIFY event with a domain with DeletionTimestamp != nil.

This is exactly what i did at first and removed
d22c7ba
i will return it, thanks

qinqon

Let's check if we need to make this thread safe.

pkg/virt-launcher/notify-client/client.go

qinqon

We can simplify the closure since the caller knows the kind of event.

pkg/virt-launcher/notify-client/client.go

Since the channel is meant only for the first add and delete events, its better to protected it, because only the first events are needed, and the delete event will be consumed only after the qemu PID is gone. So this channel should not receive unexpected events, because it will cause a deadlock. Signed-off-by: Or Shoval <oshoval@redhat.com>

oshoval · 2020-09-29T09:09:36Z

/test pull-kubevirt-e2e-k8s-cnao-1.17

openshift-ci-robot · 2020-09-29T09:09:39Z

@oshoval: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test images

Use /test all to run all jobs.

In response to this:

/test pull-kubevirt-e2e-k8s-cnao-1.17

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.

qinqon · 2020-09-29T09:21:32Z

/lgtm

Although the function updateEVents smell a little since it has mixed responsabilities, without proper naming.

oshoval · 2020-09-29T09:27:51Z

/lgtm

Although the function updateEVents smell a little since it has mixed responsabilities, without proper naming.

imho its in order to keep things simpler

alonSadan · 2020-09-29T09:49:57Z

looks good. Thanks.
One thing I didn't understand is why a closure is needed and not a regular func, can you please explain?

oshoval · 2020-09-29T10:01:34Z

looks good. Thanks.
One thing I didn't understand is why a closure is needed and not a regular func, can you please explain?

just in order to have the variables static (so they wont die between runs, because we need them to be persistent), and not use global for this purpose, so a closure gives this behavior

https://stackoverflow.com/questions/30558071/static-local-variable-in-go

alonSadan · 2020-09-29T10:27:23Z

looks good. Thanks.
One thing I didn't understand is why a closure is needed and not a regular func, can you please explain?

just in order to have the variables static (so they wont die between runs, because we need them to be persistent), and not use global for this purpose, so a closure gives this behavior

https://stackoverflow.com/questions/30558071/static-local-variable-in-go

Thanks for explaining:)
/lgtm

pkg/virt-launcher/notify-client/client.go

davidvossel

/approve

kubevirt-bot · 2020-09-29T13:48:50Z

[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:

~~OWNERS~~ [davidvossel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

oshoval · 2020-09-29T13:50:01Z

Thanks
/retest

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 24, 2020

oshoval changed the title ~~virt-launcher, fix reboot loop causing event handling hang~~ virt-launcher, Fix reboot loop causing event handling hang Sep 24, 2020

kubevirt-bot added the size/M label Sep 24, 2020

kubevirt-bot requested review from ashleyschuett and vladikr September 24, 2020 07:38

oshoval changed the title ~~virt-launcher, Fix reboot loop causing event handling hang~~ WIP virt-launcher, Fix reboot loop causing event handling hang Sep 24, 2020

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2020

oshoval commented Sep 24, 2020

View reviewed changes

pkg/virt-launcher/notify-client/client.go Outdated Show resolved Hide resolved

oshoval added 3 commits September 24, 2020 15:25

virt-launcher, Remove double domain event sending

aea5bf1

There is no need to send the event twice to virt-handler Signed-off-by: Or Shoval <oshoval@redhat.com>

virt-launcher, Remove unneeded log

664bc40

The log also is not right, it should be logged only in case the osInfo indeed changed, and it doesnt add important info beside that we are in this section, and we have prints before and after it anyway. Signed-off-by: Or Shoval <oshoval@redhat.com>

oshoval force-pushed the fix_reboot branch from d22c7ba to 664bc40 Compare September 24, 2020 12:31

kubevirt-bot added size/XS and removed size/M labels Sep 24, 2020

oshoval changed the title ~~WIP virt-launcher, Fix reboot loop causing event handling hang~~ virt-launcher, Fix Guest Agent updates causing an event handling deadlock Sep 24, 2020

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2020

davidvossel reviewed Sep 24, 2020

View reviewed changes

oshoval requested a review from davidvossel September 29, 2020 06:37

kubevirt-bot added size/M and removed size/XS labels Sep 29, 2020

qinqon suggested changes Sep 29, 2020

View reviewed changes

pkg/virt-launcher/notify-client/client.go Outdated Show resolved Hide resolved

kubevirt-bot assigned qinqon Sep 29, 2020

qinqon suggested changes Sep 29, 2020

View reviewed changes

pkg/virt-launcher/notify-client/client.go Show resolved Hide resolved

oshoval force-pushed the fix_reboot branch from 0f7fa98 to 7cdcb60 Compare September 29, 2020 08:36

oshoval requested a review from qinqon September 29, 2020 08:38

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020

kubevirt-bot assigned alonSadan Sep 29, 2020

davidvossel reviewed Sep 29, 2020

View reviewed changes

pkg/virt-launcher/notify-client/client.go Show resolved Hide resolved

davidvossel approved these changes Sep 29, 2020

View reviewed changes

kubevirt-bot assigned davidvossel Sep 29, 2020

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2020

kubevirt-bot merged commit 507ce92 into kubevirt:master Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virt-launcher, Fix Guest Agent updates causing an event handling deadlock #4253

virt-launcher, Fix Guest Agent updates causing an event handling deadlock #4253

oshoval commented Sep 24, 2020 •

edited

davidvossel left a comment

oshoval commented Sep 24, 2020 •

edited

qinqon left a comment

qinqon left a comment

oshoval commented Sep 29, 2020

openshift-ci-robot commented Sep 29, 2020

qinqon commented Sep 29, 2020

oshoval commented Sep 29, 2020

alonSadan commented Sep 29, 2020

oshoval commented Sep 29, 2020

alonSadan commented Sep 29, 2020

davidvossel left a comment

kubevirt-bot commented Sep 29, 2020

oshoval commented Sep 29, 2020

virt-launcher, Fix Guest Agent updates causing an event handling deadlock #4253

virt-launcher, Fix Guest Agent updates causing an event handling deadlock #4253

Conversation

oshoval commented Sep 24, 2020 • edited

davidvossel left a comment

Choose a reason for hiding this comment

oshoval commented Sep 24, 2020 • edited

qinqon left a comment

Choose a reason for hiding this comment

qinqon left a comment

Choose a reason for hiding this comment

oshoval commented Sep 29, 2020

openshift-ci-robot commented Sep 29, 2020

qinqon commented Sep 29, 2020

oshoval commented Sep 29, 2020

alonSadan commented Sep 29, 2020

oshoval commented Sep 29, 2020

alonSadan commented Sep 29, 2020

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Sep 29, 2020

oshoval commented Sep 29, 2020

oshoval commented Sep 24, 2020 •

edited

oshoval commented Sep 24, 2020 •

edited