virt-handler: Fix panic when listing devices on host without USB devices #11047

jschintag · 2024-01-19T09:41:54Z

What this PR does / why we need it:
Fix a panic when trying to list USB Devices on host without USB devices

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
None

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

Fix potential crash when trying to list USB devices on host without any

kubevirt-bot · 2024-01-19T09:42:04Z

Hi @jschintag. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

0xFelix · 2024-01-19T09:48:11Z

Hi, thanks for the fix! Can you add a test case for it?

jschintag · 2024-01-19T10:10:18Z

Hi, thanks for the fix! Can you add a test case for it?

Sure, will do

jschintag · 2024-01-19T10:46:02Z

I have added a test for this case.

pkg/virt-handler/device-manager/usb_device_test.go

alicefr · 2024-01-19T12:08:31Z

pkg/virt-handler/device-manager/usb_device_test.go

+			oldPathToUSBDevices := PathToUSBDevices
+			PathToUSBDevices = "/this/path/does/not/exist"
+			DeferCleanup(func() {
+				PathToUSBDevices = oldPathToUSBDevices


Please initialize this variable in a BeforeEach section for the tests

I have moved the code to a BeforeEach Section.

I mean without the DeferCleanup. You can add another BeforeEach in the other context with the valid path.

In that case i would rather just drop the DeferCleanup code entirely, since the other context does not call the discoverPluggedUSBDevices function at all, instead overriding it with their own implementation to ensure consistent test results.
I mainly added DeferCleanup to not leave the unit in a weird state afterwards, so setting the variable to it's default value should not need to happen in a BeforeEach

Sounds good

I have dropped the DeferCleanup

victortoso · 2024-01-19T12:26:50Z

Hi Jan! My minor nit would only be to break it in two commits, moving the existing tests under their own Context() would make the diff of the fix much nicer to check. Other than that, many thanks for the fix!

alicefr · 2024-01-19T12:31:35Z

/test pull-kubevirt-build

jschintag · 2024-01-19T12:35:34Z

Hi Jan! My minor nit would only be to break it in two commits, moving the existing tests under their own Context() would make the diff of the fix much nicer to check. Other than that, many thanks for the fix!

Done

alicefr · 2024-01-19T13:10:50Z

/ok-to-test

alicefr · 2024-01-19T13:11:06Z

/lgtm

0xFelix

/hold

0xFelix · 2024-01-19T13:50:34Z

pkg/virt-handler/device-manager/usb_device.go

@@ -44,7 +44,7 @@ import (
 	pluginapi "kubevirt.io/kubevirt/pkg/virt-handler/device-manager/deviceplugin/v1beta1"
 )

-const (
+var (


I don't think its good to make this exported const a writable var. Can you change discoverPluggedUSBDevices to take the path as a function parameter instead?

An alternative is to un-export it.

I went with unexporting it, since as far as i can tell at least inside kubevirt this export is never used anyway.
Additionally, since for all intents and purposes this is a constant outside of tests, i don't think something like

var discoverLocalUSBDevicesFunc = func(){ discoverPluggedUSBDevices("/sys/bus/usb/devices") }

would be the right choice.

Just to confirm, It was a mistake on my side to have it exported, probably a left over from different interactions in the code...

0xFelix · 2024-01-19T13:53:15Z

pkg/virt-handler/device-manager/usb_device_test.go

-			DevicePath:   "/dev/bus/usb/002/010",
-		},
-	}
+	Context("Host has USB Devices", func() {


While it's good to distinguish between host has USB devices or not this makes the diff pretty large. Can you instead just add the test case for an invalid path to USB devices? That's also more generic, because having an invalid path or an error during accessing this path doesn't necessarily mean a host has no USB devices.

I have changed it to a more generic Should return empty when encountering an error and reverted the commit that added the context.

0xFelix · 2024-01-19T13:53:53Z

pkg/virt-handler/device-manager/usb_device_test.go

+	})
+	Context("Host has no USB Devices", func() {
+		BeforeEach(func() {
+			PathToUSBDevices = "/this/path/does/not/exist"


When making the path a function parameter the BeforeEach can be dropped.

0xFelix · 2024-01-22T09:50:33Z

pkg/virt-handler/device-manager/usb_device_test.go

@@ -216,6 +216,13 @@ var _ = Describe("USB Device", func() {
 			},
 		),
 	)
+	BeforeEach(func() {


This will break or impact the other tests, won't it? Can you set the unexported var in the test case It instead?

It won't since the other test does not call discoverPluggedUSBDevices(). As for having it this way, please see this thread #11047 (comment)

I don't think we should taint other test cases, even though they might not use this variable right now.

I don't think we should taint other test cases, even though they might not use this variable right now.

Then it is back to the question, use a DeferCleanup or drop the BeforeEach and do it inside the test?
What do you think @0xFelix @alicefr

IMO inside the test is fine. Will that permanently alter the variable for subsequent runs of tests? (With BeforeEach it does too, but at least it's obvious.)

With normal go test it should, since the test is running in the same process. For ginkgo i don't know, but i think it uses go test somehow so likely it is the same here.

I have dropped the framework specific BeforeEach, because it is intended for repeating setup operations, whereas in this instance we only really want the variable to be changed for a single test. It is not shared code and should therefore not happen outside of the actual test function.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>

jschintag · 2024-01-23T09:10:04Z

/retest

victortoso · 2024-01-23T11:21:36Z

Thanks again,
/lgtm

alicefr · 2024-01-23T12:30:11Z

The code looks fine to me
/approve
/hold
Waiting @0xFelix to have a second look

kubevirt-bot · 2024-01-23T12:30:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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~~ [alicefr]

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

kubevirt-commenter-bot · 2024-01-23T12:30:22Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-k8s-1.27-ipv6-sig-network

0xFelix

Thanks!

/lgtm
/hold cancel

victortoso · 2024-01-23T13:45:08Z

I think this could be cherry picked to v1.1.0 ? Not sure how that is done.

victortoso · 2024-01-23T13:45:35Z

/retest-required

0xFelix · 2024-01-23T14:03:42Z

/cherry-pick release-1.1

kubevirt-bot · 2024-01-23T14:03:44Z

@0xFelix: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.1

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.

kubevirt-commenter-bot · 2024-01-24T06:54:03Z

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

jschintag · 2024-01-24T15:21:12Z

/retest

kubevirt-bot · 2024-01-24T23:28:47Z

@0xFelix: new pull request created: #11079

In response to this:

/cherry-pick release-1.1

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.

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. labels Jan 19, 2024

kubevirt-bot requested review from 0xFelix and vasiliy-ul January 19, 2024 09:42

kubevirt-bot added size/XS needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 19, 2024

jschintag mentioned this pull request Jan 19, 2024

Enable building and running kubevirt on s390x #10490

Open

19 tasks

jschintag force-pushed the fix-usb-error branch from d979fc9 to 1f87c2c Compare January 19, 2024 10:43

kubevirt-bot added size/L and removed size/XS labels Jan 19, 2024

alicefr reviewed Jan 19, 2024

View reviewed changes

pkg/virt-handler/device-manager/usb_device_test.go Outdated Show resolved Hide resolved

alicefr reviewed Jan 19, 2024

View reviewed changes

jschintag force-pushed the fix-usb-error branch from 1f87c2c to d3ab2c5 Compare January 19, 2024 12:24

jschintag force-pushed the fix-usb-error branch from d3ab2c5 to 343da72 Compare January 19, 2024 12:35

jschintag force-pushed the fix-usb-error branch 2 times, most recently from 269a291 to 884d6c4 Compare January 19, 2024 13:03

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 19, 2024

kubevirt-bot assigned alicefr Jan 19, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2024

0xFelix reviewed Jan 19, 2024

View reviewed changes

kubevirt-bot added size/S and removed size/L labels Jan 22, 2024

0xFelix reviewed Jan 22, 2024

View reviewed changes

virt-handler: Fix panic when listing devices on host without USB devices

3a414ea

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>

jschintag force-pushed the fix-usb-error branch from 00562aa to 3a414ea Compare January 22, 2024 14:23

kubevirt-bot assigned victortoso Jan 23, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2024

xpivarc approved these changes Jan 23, 2024

View reviewed changes

kubevirt-bot assigned xpivarc Jan 23, 2024

0xFelix approved these changes Jan 23, 2024

View reviewed changes

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2024

kubevirt-bot assigned 0xFelix Jan 23, 2024

kubevirt-bot merged commit 9b39fcf into kubevirt:main Jan 24, 2024
39 checks passed

kubevirt-bot mentioned this pull request Jan 24, 2024

[release-1.1] virt-handler: Fix panic when listing devices on host without USB devices #11079

Merged

jschintag deleted the fix-usb-error branch January 25, 2024 07:33

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

jschintag mentioned this pull request Mar 27, 2024

Add jschintag to kubevirt members kubevirt/project-infra#3328

Merged

7 tasks

virt-handler: Fix panic when listing devices on host without USB devices #11047

virt-handler: Fix panic when listing devices on host without USB devices #11047

Conversation

jschintag commented Jan 19, 2024 • edited

kubevirt-bot commented Jan 19, 2024

0xFelix commented Jan 19, 2024

jschintag commented Jan 19, 2024

jschintag commented Jan 19, 2024

alicefr Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victortoso commented Jan 19, 2024

alicefr commented Jan 19, 2024

jschintag commented Jan 19, 2024

alicefr commented Jan 19, 2024

alicefr commented Jan 19, 2024

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschintag Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschintag commented Jan 23, 2024

victortoso commented Jan 23, 2024

alicefr commented Jan 23, 2024

kubevirt-bot commented Jan 23, 2024

kubevirt-commenter-bot commented Jan 23, 2024

0xFelix left a comment

Choose a reason for hiding this comment

victortoso commented Jan 23, 2024

victortoso commented Jan 23, 2024

0xFelix commented Jan 23, 2024

kubevirt-bot commented Jan 23, 2024

kubevirt-commenter-bot commented Jan 24, 2024

jschintag commented Jan 24, 2024

kubevirt-bot commented Jan 24, 2024

jschintag commented Jan 19, 2024 •

edited

alicefr Jan 19, 2024 •

edited

jschintag Jan 22, 2024 •

edited