-
Notifications
You must be signed in to change notification settings - Fork 611
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 bypass4netns integration #808
Conversation
It should be noted that Vagrant on macOS on GHA is extremely slow environment. |
SocketPath: socketPath, | ||
PidFilePath: pidFilePath, | ||
LogFilePath: logFilePath, | ||
IgnoreSubnets: []string{"127.0.0.0/8", "10.0.0.0/8"}, |
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.
"10.0.0.0/8" shouldn't be hardcoded.
But you may add a documentation about this and call it a day.
Eventually, this should be passed from (equivalent of ) nerdctl network list
and nerdctl network inspect
.
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.
The documentation can be added in docs/rootless.md
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 please explain that bypass4netns is experimental in https://github.com/containerd/nerdctl/blob/master/docs/experimental.md
Thank you for your comment! |
pkg/labels/labels.go
Outdated
@@ -69,4 +69,7 @@ const ( | |||
|
|||
// Platform is the normalized platform string like "linux/ppc64le". | |||
Platform = Prefix + "platform" | |||
|
|||
// Bypass4netns is the flag to enable acceleration with bypass4netns |
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.
Please explain the value format
cmd/nerdctl/run_linux.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
labelMaps := strutil.ConvertKVStringsToMap(strutil.DedupeStrSlice(lab)) |
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.
We need to support label-file
too, but it can be fixed in a separate PR if it is too complicated.
Please add "TODO" comment in that case.
Line 843 in b88e714
labelsFilePath, err := cmd.Flags().GetStringSlice("label-file") |
Yes. |
I improved the code and updated performance benchmark result |
docs/rootless.md
Outdated
| container -> host | 0.398 Gbps | **42.2 Gbps** | | ||
| host -> container | 20.6 Gbps | **47.4 Gbps** | | ||
|
||
This benchmark can be reproduced with [Vagrantfile(https://github.com/rootless-containers/bypass4netns/blob/master/Vagrantfile)](https://github.com/rootless-containers/bypass4netns/blob/master/Vagrantfile) |
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.
blob/master
Please specify the commit hash for reproducibility
docs/rootless.md
Outdated
Container networks which are not contained in the subnets can be broken or can cause problems. | ||
|
||
### TODO | ||
- Remove hard-coded subnets in pkg/bypass4netnsutil/bypass.go L.59 ~ L.60 |
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.
Remove "L.59 ~ L.60", as line numbers are not permanent
Could you squash commits? |
option '--label nerdctl/bypass4netns=true' in run command enables acceleration with bypass4netns Signed-off-by: Naoki MATSUMOTO <naoki@pibvt.net>
I improved and squashed commits into the first 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.
Thanks
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 is super cool 🎉
CI failure (rootful / v1.5.9) is unrelated
merging |
Thank you for reviewing and merging! |
option '--label nerdctl/bypass4netns=true' in run command enables acceleration with bypass4netns
Description
This patch is initial bypass4netns support in nerdctl.
bypass4netns(https://github.com/rootless-containers/bypass4netns) is an accelerator for rootless networking. This improves outgoing or incoming (with --publish option) networking performance.
The performance benchmark with iperf3 on Ubuntu 21.10 on Hyper-V VM is shown below.
Acceleration with bypass4netns is available with
--label nerdctl/bypass4netns=true
Example
$ nerdctl run -it --rm -p 8080:80 --label nerdctl/bypass4netns=true alpine
More detail is available at README.md(https://github.com/rootless-containers/bypass4netns/blob/master/README.md)
Signed-off-by: Naoki MATSUMOTO naoki@pibvt.net