-
Notifications
You must be signed in to change notification settings - Fork 618
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
Support for lima usernet network #1383
Conversation
156fcce
to
46ac22d
Compare
pkg/limayaml/limayaml.go
Outdated
@@ -31,6 +31,7 @@ type LimaYAML struct { | |||
Env map[string]string `yaml:"env,omitempty" json:"env,omitempty"` | |||
DNS []net.IP `yaml:"dns,omitempty" json:"dns,omitempty"` | |||
HostResolver HostResolver `yaml:"hostResolver,omitempty" json:"hostResolver,omitempty"` | |||
UserNet UserNet `yaml:"userNet,omitempty" json:"userNet,omitempty"` |
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.
Seems confusing to have two network options both called "user"...
https://wiki.qemu.org/Documentation/Networking#User_Networking_(SLIRP)
Originally posted by @afbjorklund in #1222 (comment)
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.
Bikeshedding:
gvisor
(confusing because it does not use gvisor's process sandbox)gvisor-tap-vsock
(confusing because it is not gvisor, nor tap, nor vsock)netstack
(confusing because any networking can be called "net(work) stack"user-v2
(confusing because there was nouser-v1
)modern
(confusing because it can be eventually legacy)
Edit
builtin
multivm
multi-vm
vm-to-vm
vm2vm
v2v
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.
Actually it would be OK to name both of them "user", and then toggle the slirp vs gvisor later ?
Just was afraid that we would have one called "user" and a different one called "usernet"...
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.
Am bit confused on this "two network options both called user"
As of now we don't have any configuration for slirp right?
And the one we provide now will be more a toggle, if enabled will use the latest or revert to the old modal ?
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.
It is called user in qemu, is what I meant. If there is an option called usernet in lima too, it is confusing.
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.
Or "multivm", "multi-vm", "vm-to-vm", "vm2vm", "v2v"
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.
To me the following suggestions looks good,
- user-v2 as this give support for giving further versions as well like user-v3 etc.
- builtin since usernet command is within limactl it kind of makes sense as well
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.
@afbjorklund @jandubois
WDYT?
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 don't know what happened with "user-v1", is that the normal one ? But "user-v2" seems complicated to me, as a yaml default. But maybe it needs versioning (as per above, "user-v3", "user-v4") otherwise "builtin" is simpler.
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.
What about calling it "builtin"
My vote is for builtin
46ac22d
to
f85f804
Compare
16ad9b5
to
ebda445
Compare
Marking PR as ready for review as the code looks lot better compared to the initial version. There are some todo items but i think it can be done incrementally as well. Should not affect the current code much. |
Testing for performance are done as well. I tested in M1 with some long running instance (for more than a day with sleep). Haven't faced any issues (There was slight degrade when did iperf3, but on restarting iperf3 -s on host the performance was back to more meaningful value) Also performance of QEMU is improved with this PR containers/gvisor-tap-vsock#188 Performance report
QEMU <-> Host
VZ <-> QEMU
|
Seems mixed up with a different PR |
examples/experimental/usernet.yaml
Outdated
@@ -0,0 +1,15 @@ | |||
# Example to run vz instance with lima usernet enabled |
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.
usernet.yaml
usernet-v2.yaml
, or maybe net-user-v2.yaml
(If we are going to call the new network as user-v2
)
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.
Will rename this once the name is decided in above comment
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
Here is what I got:
And eventually:
|
Missed support for linux with networks.lima property. |
400129a
to
b0ae1bc
Compare
@afbjorklund |
To be honest, I kind of like the name "usernet". I'm also not confusing it with QEMU networking, as I always think of that one as SLIRP. "User mode networking" is just the general category, and SLIRP is a specific instance of it. Now, you could argue that we should not use the category name ("user mode networking") and usurp it for the specific Lima implementation, but again personally I'm not bothered by it because it is disambiguated by context (it is the Lima usernet). I think descriptive names are good. And if we ever need another name, and still can't come up with anything better, then we can still use |
"slirp" has become a general category too. |
I have no strong feelings about the name. I prefer Just to throw something else out: How about Not even sure if I like it myself though; might delete later. 🤣 |
b0ae1bc
to
53a6924
Compare
48c408d
to
00a3f9d
Compare
for ipAddr, leaseAddr := range leases { | ||
if vmMacAddr == leaseAddr { | ||
err = c.delegate.Expose(&types.ExposeRequest{ | ||
Local: fmt.Sprintf(":%d", sshPort), |
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.
Local: fmt.Sprintf(":%d", sshPort), | |
Local: fmt.Sprintf("127.0.0.1:%d", sshPort), |
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
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.
s/-/_/ in the filename
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
9a5535a
to
0a7451f
Compare
- location: "~" | ||
- location: "/tmp/lima" | ||
writable: true | ||
mountType: "virtiofs" |
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.
Shouldn't be needed
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
@@ -0,0 +1,15 @@ | |||
# Example to run vz instance with experimental user-v2 network enabled | |||
vmType: "vz" |
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.
Shouldn't be needed
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
15953b8
to
20b34b5
Compare
Sorry needs another rebase |
Signed-off-by: Balaji Vijayakumar <[email protected]>
20b34b5
to
800e450
Compare
@AkihiroSuda - Done :) |
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
@jandubois @afbjorklund May I merge this, or do you want to take the final look before merging? |
I did want to review it, but I continue to be out of time. Feel free to merge if you are ok with it. At least it gets it into more people's hands for testing. |
Any chance of releasing this? It seems it would fix some issues I'm having. |
Yes, probably next week |
fixes #1222
This PR adds support for the following,
Todo items