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.

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

runtime-rs: Support vhost-user-net device #8626

Merged
merged 6 commits into from Dec 26, 2023
Merged

runtime-rs: Support vhost-user-net device #8626

merged 6 commits into from Dec 26, 2023

Conversation

justxuewei
Copy link
Member

@justxuewei justxuewei commented Dec 11, 2023 •
edited

This commit introduces VhostUserEndpoint and supports relative to
vhost-user-net devices for device manager. For now, Dragonball is able to
attach vhost-user-net devices.

Fixes: #8625

@justxuewei justxuewei marked this pull request as draft December 11, 2023 08:54
@katacontainersbot katacontainersbot added the size/large Task of significant size label Dec 11, 2023
@justxuewei justxuewei changed the title runtime-rs: Support vhost-user-net devices runtime-rs: Support vhost-user-net device Dec 25, 2023
@justxuewei justxuewei added ok-to-test no-backport-needed Changed do not need to be applied to an older branch / repository no-forward-port-needed Changed do not need to be applied to a newer branch / repository labels Dec 25, 2023
@justxuewei justxuewei marked this pull request as ready for review December 25, 2023 06:43
@justxuewei justxuewei force-pushed the vhost-user-endpoint branch 4 times, most recently from 04e5851 to f4c0c24 Compare December 25, 2023 08:04
@justxuewei
runtime-rs: Simplify VhostUserType enum
374c2f0 
Remove unused string parameter from each item.

Fixes: kata-containers#8625

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
Apokleos
Apokleos reviewed Dec 25, 2023
View reviewed changes
Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

Thx @justxuewei great work！some comments

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs Outdated
Comment on lines 240 to 243
DeviceType::HybridVsock(_) => {
continue;
}
DeviceType::Vsock(_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, no need to do this, and the change does not related to the commit. I suggest keep it was.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with that is non-relevant with this patch. But it is necessary to do that from my perspective. The reason is that there is no warning when a developer somehow forgets to give a implementation of a device type. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you fix this because you got a warning? Or did you do it to make things easier for future developers?
If you insist on making the change, please put it in a separate commit. This will keep the changes clean and easy for reviewers to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I still think it is necessary for us. Plus, I improved the code style of it. It looks more concise now.

DeviceType::HybridVsock(_) | DeviceType::Vsock(_) => {
    continue;
}

The changes have been laid in a separate commit now. Please take a look, thanks!

src/runtime-rs/crates/resource/src/network/endpoint/vhost_user_endpoint.rs Outdated
#[derive(Debug)]
pub struct VhostUserEndpoint {
// Endpoint index
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

why dead code ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this field from VhostUserEndpoint.

src/runtime-rs/crates/hypervisor/src/dragonball/inner_device.rs Show resolved Hide resolved
@Apokleos
Copy link
Contributor

/test

Tim-0731-Hzt
Tim-0731-Hzt approved these changes Dec 26, 2023
View reviewed changes
Copy link
Member

@Tim-0731-Hzt Tim-0731-Hzt left a comment

Choose a reason for hiding this comment

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

Thanks @justxuewei LGTM

@justxuewei
runtime-rs: Support VhostUserEndpoint
54df832 
This commit introduces VhostUserEndpoint and supports relative to
vhost-user-net devices for device manager. For now, Dragonball is able to
attach vhost-user-net devices.

Fixes: kata-containers#8625

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
dragonball: Make vhost-user-net ready for VhostUserEndpoint
693a0cf 
The changes involve:

- Expose VhostUserConfig struct to runtime-rs.
- Set a default value while num_queues or queue_size are 0.

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
runtime-rs: DAN supports vhost-user-net device
60a4235 
DAN reads vhost-user-net device from JSON config. It only supports VMM
running as server right now.

Fixes: kata-containers#8625

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
runtime-rs: Remove unused index from Endpoints
f2d08bc 
The affected `Endpoint`s are `VhostUserEndpoint` and `TapEndpoint`.

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
runtime-rs: Expand all DeviceType in match arms
36a4cbc 
The compiler will give a warning if a developer forget to add an arm for
a new variants defined.

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
Copy link
Member Author

/test

Apokleos
Apokleos approved these changes Dec 26, 2023
View reviewed changes
Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

Thx @justxuewei LGTM!

@justxuewei
Copy link
Member Author

/test-s390x

@justxuewei justxuewei merged commit 1065ca6 into kata-containers:main Dec 26, 2023
168 of 176 checks passed
FuuuOverclocking
FuuuOverclocking reviewed Dec 28, 2023
View reviewed changes
src/dragonball/src/api/v1/virtio_net.rs
@@ -68,6 +68,7 @@ pub struct VirtioConfig {
pub allow_duplicate_mac: bool,
}

/// Config for vhost-user-net device
#[cfg(feature = "vhost-user-net")]
Copy link
Contributor

Choose a reason for hiding this comment

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

@justxuewei Hello~ I think src/dragonball/src/api/v1/virtio_net.rs: line 72 should be removed. VhostUserConfig is gated behind the vhost-user-net feature, but is exported when other features are enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for reporting this. I've raised a PR to fix this. Please refer to #8744.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FuuuOverclocking FuuuOverclocking FuuuOverclocking left review comments

@Apokleos Apokleos Apokleos approved these changes

@Tim-0731-Hzt Tim-0731-Hzt Tim-0731-Hzt approved these changes

Assignees
No one assigned
Labels
no-backport-needed Changed do not need to be applied to an older branch / repository no-forward-port-needed Changed do not need to be applied to a newer branch / repository ok-to-test size/large Task of significant size
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

runtime-rs: VhostUserEndpoint isn't supported yet
5 participants
@justxuewei @Apokleos @FuuuOverclocking @Tim-0731-Hzt @katacontainersbot