-
Notifications
You must be signed in to change notification settings - Fork 176
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
Add the tun mounting with vhost is requested #394
Add the tun mounting with vhost is requested #394
Conversation
@adrianchiris @martinkennelly can you please have a look on this PR please, I will like to get your comments about it. |
f456f39
to
f951e87
Compare
/cc @zshi-redhat |
/cc @NicolasDichtel |
return GetVhostNetDeviceSpec() | ||
deviceSpec[0] = GetVhostNetDeviceSpec()[0] | ||
|
||
if !TunDeviceExist() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we now require /dev/net/tun to exist when needVhostNet
is set to true
in resource config.
is there no use-case for vhost-net without tun device ? (there must be because that how it was working before :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment!
That was my question I am not aware from our side of any use case that we use vhost until now.
Now we have a use-case (dpdk slow path/ vpp) there is a need to create a tap device and for that, the tun device should be available inside the pod.
The other solution is to have a new flag for the tun device but I will like to know if there is a use-case for only vhost-net mount if not I don't see a reason to create another flag to will be always equal to the vhost one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With our use case, we also need /dev/net/tun, it is exported to via:
volumes:
- hostPath:
path: /dev/net/tun
type: ""
name: net
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not with upstream DPDK. The vhost-net (virtio-user) PMD creates a tap device and configures it as the backend of the vhost-net device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, thanks for the input. if all use-cases sift down to needing tun as well, to me it makes sense to provide it as a mount when needVhostNet is specified.
lets see if we get additional inputs, maybe raise this in one of the NPWG network resource mgmt meeting ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it at the next meeting thanks everyone for the inputs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update documentation as well to mention this new mount ?
pkg/netdevice/netInfoProviders.go
Outdated
if !VhostNetDeviceExist() { | ||
glog.Errorf("GetDeviceSpecs(): /dev/vhost-net doesn't exist") | ||
return nil | ||
} | ||
return GetVhostNetDeviceSpec() | ||
deviceSpec[0] = GetVhostNetDeviceSpec()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just
deviceSpec := GetVhostNetDeviceSpec()
...
...
deviceSpec := append(deviceSpec, GetTunDeviceSpec()...)
return deviceSpec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just didn't want to append as in the underling it will create a new slice and copy everything but again here is only a small slice so if you want I can change the code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, if later on, if either GetVhostNetDeviceSpec() or GetTunDeviceSpec() end up returning more than one element, you will not need to change here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me :)
8c009a6
to
f83cd6c
Compare
document updated. @adrianchiris can you have another round of review please |
This commit add also the tun device into the container. This is needed to create tap devices inside the container network namespace Vhost-net is used to accelerate the dpdk connected to kernel networking using tap devices. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
f83cd6c
to
0427aef
Compare
pkg/netdevice/netInfoProviders.go
Outdated
@@ -72,11 +72,20 @@ func NewVhostNetInfoProvider() types.DeviceInfoProvider { | |||
/* DeviceInfoProvider Interface */ | |||
|
|||
func (rip *vhostNetInfoProvider) GetDeviceSpecs() []*pluginapi.DeviceSpec { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove newline, un-needed change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done thanks, the linter was complaining about it :)
merge with two approvals. |
This commit add also the tun device into the container.
This is needed to create tap devices inside the container network namespace
Vhost-net is used to accelerate the dpdk connected to kernel networking using tap devices.
Signed-off-by: Sebastian Sch sebassch@gmail.com