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

virtctl: improve usbredir as module #11643

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

virtctl: improve usbredir as module #11643

wants to merge 8 commits into from

Conversation

victortoso
Copy link
Member

What this PR does

Before this PR:
The virctl usbredir command was a big script that was hard to be re-used. The functional tests in usbredir_test.go were not testing virtctl's code at all, instead it was re-implementing the client logic without calling usbredirect binary.

After this PR:
The virtctl usbredir is broken down with a Client type with init, run and stop functions. The functional test makes usage of this.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

I'm interested in expanding a bit the virtctl/usbredir module to have a little bit more knowledge of usbredir protocol in a follow-up, in order to fetch details of devices being redirected by virtctl and expose that information to users. This PR is a somewhat 'cleaning the house' step before that.

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.

Release note

NONE

@kubevirt-bot 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. size/L area/virtctl sig/buildsystem Denotes an issue or PR that relates to changes in the build system. labels Apr 3, 2024
@alicefr
Copy link
Member

alicefr commented Apr 4, 2024

@victortoso you are changing the flag --to and this should at least go into the release notes

@alicefr
Copy link
Member

alicefr commented Apr 4, 2024

@victortoso you are changing the flag --to and this should at least go into the release notes

Ok, sorry, this is a local flag not for the command. Ignore my comment

@alicefr
Copy link
Member

alicefr commented Apr 4, 2024

Looks great!
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2024
xpivarc
xpivarc reviewed Apr 9, 2024
View reviewed changes
Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

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

note for me: I need to review the last 3 commits again.

pkg/virtctl/usbredir/usbredir.go Outdated Show resolved Hide resolved
pkg/virtctl/usbredir/usbredir.go Show resolved Hide resolved
pkg/virtctl/usbredir/usbredir.go Outdated Show resolved Hide resolved
@victortoso victortoso force-pushed the pull/virtctl/usbredir-as-module branch from a303ec8 to 91b7f86 Compare April 9, 2024 19:33
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2024
@kubevirt-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@victortoso victortoso force-pushed the pull/virtctl/usbredir-as-module branch from 91b7f86 to 4e65621 Compare April 9, 2024 19:33
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alicefr. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@victortoso
Copy link
Member Author

note for me: I need to review the last 3 commits again.

@xpivarc, i've updated with requested changes/suggestions.

@victortoso victortoso force-pushed the pull/virtctl/usbredir-as-module branch from 4e65621 to 409cbc3 Compare April 9, 2024 20:55
@victortoso
Copy link
Member Author

/test pull-kubevirt-unit-test-arm64
/test pull-kubevirt-e2e-k8s-1.29-sig-compute

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2024
@victortoso
virtctl: usbredir: Add bus-device option doc
20bb412 
Using bus-device is specially helpful when the user has multiple
identical devices but want to pick which one goes to which VMI.

Using "bus-device" format was always possible, just not properly
documented.

Also adds proper examples in the usage() method.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
virtctl: usbredir: move streamStop channel
5bf892c 
This should only be used in the read/write scope of that goroutine.
Use a select to properly handle local or remote completion.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
virtctl: usbredir: error when connection is closed
3cdb5bd 
The expected end of USB redirection is only when the user to stop
sharing the USB devices. If the connection is closed without expecting
it, we should treat it as error.

The follow up patch "virtctl: usbredir: Add Stop() to Client" will
improve this logic to make it clear when we expected it to stop.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
virtctl: usbredir: use full address directly
ec86b36 
We don't have to hardcode 'localhost' and do extra code here.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
virtctl: usbredir: remove extra channel and goroutine
cb2b255 
We need interrupt channel for signal.Notify() and we can use it to
replace sigStopChan. This also allows us to remove it the extra
goroutine.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
virtctl: usbredir: move run logic under Client type
05c51a3 
The main goal is to reuse this type for testing. Similar logic is
being duplicated here and tests/usbredir_test.go making it harder to
write bug reproducer as functional test.

Some variables were renamed too when moved as part of Client struct.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
virtctl: usbredir: Add Stop() to Client
f304e2b 
Add context when running usbredirect binary so we can stop it based on
request. This request is provided by a Stop() method of Client. This
is to be used by tests in the follow-up patch.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso
tests: usbredir: use virtctl/usbredir Client
b7dfa95 
This ensures that the same code used in virtctl to run and manage the
data from usbredirect binary are also used in the tests.

This also adds a test to expect failure when limit of
v1.UsbClientPassthroughMaxNumberOf devices is reached.

Signed-off-by: Victor Toso <victortoso@redhat.com>
@victortoso victortoso force-pushed the pull/virtctl/usbredir-as-module branch from 409cbc3 to b7dfa95 Compare April 22, 2024 14:04
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024
@victortoso
Copy link
Member Author

Rebased. Had conflict with c8d63db

@victortoso
Copy link
Member Author

/test pull-kubevirt-conformance-arm64
/test pull-kubevirt-unit-test-arm64

@xpivarc let me know if you have further comments on this one

@victortoso victortoso mentioned this pull request May 2, 2024
Draft
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpivarc xpivarc xpivarc left review comments

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@oshoval oshoval Awaiting requested review from oshoval

Assignees

@alicefr alicefr

Labels
area/virtctl dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note-none Denotes a PR that doesn't merit a release note. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/network sig/scale sig/storage size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@victortoso @alicefr @kubevirt-bot @xpivarc