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

dragonball: add vhost-user connection management logic #8450

Merged
merged 2 commits into from Nov 21, 2023
Merged

dragonball: add vhost-user connection management logic #8450

merged 2 commits into from Nov 21, 2023

Conversation

adamqqqplay
Copy link
Contributor

The vhost-user connection management logic will be used by
the upcoming features: vhost-user-net, vhost-user-blk and
vhost-user-fs.

Fixes: #8448

@katacontainersbot
Copy link
Contributor

Can one of the admins verify this patch?

@katacontainersbot katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Nov 15, 2023
@adamqqqplay
Copy link
Contributor Author

/test

1 similar comment
@studychao
Copy link
Member

/test

justxuewei
justxuewei reviewed Nov 15, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Thanks @adamqqqplay, a few comments here.

src/dragonball/src/dbs_utils/src/lib.rs Outdated
Comment on lines 5 to 11
// Reexport timerfd public interfaces.
pub mod timerfd {
pub use timerfd::*;
}

// Reexport vmm_sys_util public interfaces.
pub use vmm_sys_util::{aio, errno, eventfd, fallocate, ioctl, tempdir, tempfile, terminal};
Copy link
Member

Choose a reason for hiding this comment

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

Are these re-export interfaces necessary? Shall we remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed.

src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated
@@ -125,6 +125,32 @@ pub enum ActivateError {
InvalidQueueConfig,
#[error("IO: {0}.")]
IOError(#[from] IOError),
#[error("VirtIo error")]
VirtIoError(Error),
#[error("VirtIo error")]
Copy link
Member

Choose a reason for hiding this comment

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

Is error message correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated
#[error("VirtIo error")]
EpollMgr(dbs_utils::epoll_manager::Error),
#[cfg(feature = "vhost")]
#[error("VirtIo error")]
Copy link
Member

Choose a reason for hiding this comment

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

Is error message correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated
@@ -155,6 +181,9 @@ pub enum Error {
/// Guest gave us a descriptor that was too big to use.
#[error("descriptor length too big.")]
DescriptorLengthTooBig,
/// Error from the epoll event manager
#[error("dbs_utils error: {0}.")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[error("dbs_utils error: {0}.")]
#[error("dbs_utils error: {0:?}.")]

Use it to print error stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated

#[cfg(feature = "vhost")]
/// Error from the vhost subsystem
#[error("Vhost error: {0}")]
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated
VhostError(vhost_rs::Error),
#[cfg(feature = "vhost")]
/// Error from the vhost user subsystem
#[error("Vhost-user error: {0}")]
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/connection.rs Outdated Show resolved Hide resolved
@studychao
Copy link
Member

/test

1 similar comment
@justxuewei
Copy link
Member

/test

justxuewei
justxuewei approved these changes Nov 19, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks @adamqqqplay!

justxuewei
justxuewei requested changes Nov 20, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment •
edited

Choose a reason for hiding this comment

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

Please replace VirtIoXxx with VirtioXxx, see: #8465.

adamqqqplay and others added 2 commits November 21, 2023 09:51
@adamqqqplay
dragonball: add vhost-user connection management logic
a6a399d 
The vhost-user connection management logic will be used by
the upcoming features: vhost-user-net, vhost-user-blk and
vhost-user-fs.

Fixes: kata-containers#8448

Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>
Signed-off-by: Qinqi Qu <quqinqi@linux.alibaba.com>
Signed-off-by: Huang Jianan <jnhuang@linux.alibaba.com>
@mofishzz @adamqqqplay
dragonball: add test utils for vhost-user
a957139 
The test utils will be used by the upcoming feature tests: vhost-user-net,
vhost-user-blk and vhost-user-fs.

Signed-off-by: Beiyue <beiyue@linux.alibaba.com>
Signed-off-by: Huang Jianan <jnhuang@linux.alibaba.com>
@adamqqqplay
Copy link
Contributor Author

Please replace VirtIoXxx with VirtioXxx, see: #8465.

@justxuewei Fixed, PTAL.

justxuewei
justxuewei approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@justxuewei
Copy link
Member

/test

@studychao
Copy link
Member

/test-s390x

@studychao studychao merged commit 6a6c3c5 into kata-containers:main Nov 21, 2023
150 of 162 checks passed
@adamqqqplay adamqqqplay mentioned this pull request Dec 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justxuewei justxuewei justxuewei approved these changes

@studychao studychao studychao approved these changes

Assignees
No one assigned
Labels
ok-to-test size/huge Largest and most complex task (probably needs breaking into small pieces)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dragonball: add general vhost-user support
5 participants
@adamqqqplay @katacontainersbot @studychao @justxuewei @mofishzz