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

dragonball: Use vhost-net device by default #8609

Merged
merged 2 commits into from Dec 14, 2023

Conversation

justxuewei
Copy link
Member

This patch set vhost-net as default backend of networking. It allows users
to set disable_vhost_net to true to reenable virtio-net backend.
Plus, which backend to use is a matter of hypervisor, runtime-rs will no
longer need to know that.

Fixes: #8608

@katacontainersbot katacontainersbot added the size/large Task of significant size label Dec 8, 2023
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 for your work!
But IMO you should split the PR into at least two commits, one for dragonball and one for runtime-rs, rather than having one PR which has only one commit with several changes at different levels.

@justxuewei
Copy link
Member Author

Thx @justxuewei for your work! But IMO you should split the PR into at least two commits, one for dragonball and one for runtime-rs, rather than having one PR which has only one commit with several changes at different levels.

@Apokleos Thanks for your kindly review. I've split this patch into two commits, one for runtime-rs, another one for dragonball. PTAL.

@justxuewei
Copy link
Member Author

/test

@justxuewei
Copy link
Member Author

/test

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 for your work! I have some comments.

fn add_net_device(&mut self, config: &NetworkConfig) -> Result<()> {
fn add_net_device(
&mut self,
hconfig: &HypervisorConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, there's no need introduce a hconfig here, you can just get the hypervisor config in add_net_device before you do dragonball_network_device

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

}
/// Generate Dragonball network config according to hypervisor config and
/// runtime network config.
pub(crate) fn dragonball_network_config(
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name dragonball_network_config is not clear. I think it'd better to change it to something that is more specific and easier to understand, such as "build_xxx", "setup_xxx", "init_xxx"...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@justxuewei
Copy link
Member Author

/test

@justxuewei justxuewei force-pushed the runtime-rs-vhost-net branch 2 times, most recently from 1804c7b to 9719dc4 Compare December 14, 2023 03:16
This patch set vhost-net as default backend of networking. It allows users
to set `disable_vhost_net` to `true` to reenable virtio-net backend.
Plus, which backend to use is a matter of hypervisor, runtime-rs will no
longer need to know that.

Fixes: kata-containers#8608

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
Dragonball sets a default queue config in the case of `None`. The
queue_size and num_queues of vhost-net are set to `Some(0)` by default.
Therefore, we might get an invalid queue config. This patch fixes this
issue.

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
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 Nice work!
LGTM !

@justxuewei
Copy link
Member Author

/test

@justxuewei justxuewei merged commit 7f611df into kata-containers:main Dec 14, 2023
223 of 243 checks passed
justxuewei added a commit to justxuewei/kata-containers that referenced this pull request Dec 22, 2023
The default network backend of runtime-rs with Dragonball is vhost-net
after kata-containers#8609 merged. The tests might be failed if vhost modules are not
loaded.

Fixes: kata-containers#8717

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
justxuewei added a commit to justxuewei/kata-containers that referenced this pull request Dec 22, 2023
The default network backend of runtime-rs with Dragonball is vhost-net
after kata-containers#8609 merged. The tests might be failed if vhost modules are not
loaded.

Fixes: kata-containers#8717

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dragonball: Use vhost-net device by default
4 participants