Add --network-id option#258
Conversation
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
@tofugarden Thanks! Let's try to simplify and minimize change in existing code.
Can you add more details on what does not work with ipv6? We can add a test mark as xfail to we know that does not work.
|
To simplify the change, I would start with adding --network-id in all layers without the options to set the host ip/ipv6/subnet. When this works locally and in the CI we can add the ipv4 options. Finally add the ipv6 options. |
Thank you so much, this is great! I should have time to have the first of these smaller patches in by Sunday. |
|
Comparing test results between macOS <26 & 26, and there seems to be inconsistent behavior that isn't documented as far as I can find. Local testing on macOS 15.6.1 shows:
On macOS 26, a /24 subnet is selected by There is nothing explicitly incorrect about having a bridge with no host address I think? It's just a shame that users on <26 will need to manually deconflict subnets if I think I'll write a test that starts two helpers without |
|
@tofugarden Don't worry about macOS < 26. macOS 27 will be released this year, and macOS 15 will become unsupported next year with macOS 28. We can add a warning about using --network-id requires without --host-ip-address on macOS < 26. I would not enforce this since letting the system assign the network is the best way - it ensures you do't conflict with other application creating networks. Let's focus now on adding --network-id without anything else. This can be nice little feature for next release. |
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
@tofugarden Thanks! Looks good in quick look, I'll do a more detailed review later.
Please squash all commits and force push, and update the PR message to reflect the current change - adding --network-id option.
|
@nirs couple small fixes, getting on it now! |
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Check the CI failures:
https://github.com/nirs/vmnet-helper/actions/runs/28299280398
If you enable the actions on you fork you can run the tests by creating a pull request to your fork. In this repo I need to approve the workflow manually.
Setting --network-id with --operation-mode=host only allows communication with other host mode interfaces with the same network ID. DHCP services are not provided on this network.
tofugarden
force-pushed
the
feat/network-id
branch
from
18f8b50 to
a212756
Compare
tofugarden
changed the title
--network-id and related host address options--network-id option
|
|
||
| - **--network-id**: Takes a UUID to identify the network. The guest can only | ||
| communicate with other host-mode interfaces with the same network ID. No DHCP | ||
| service is provided. |
There was a problem hiding this comment.
We can add a note about testing with the run script - it will not with --network-id since our vms are configured to use DHCP, and there is no DHCP server on this network.
If you know how to configure DHCP on the network it can be useful to add a section about this here or in docs/development since this is not related to integration with other tools.
Another option is to improve the the testing/ package to configure static ip for the vm when using --network-id. One option is to add --static-ip option to run script, and change cloud-init to configure a static ip instead o DHCP.
I'm also not sure how this will work with mDNS - the vms ip is discovered using mDNS.
For now we can add a note that this does not work with the run script and improve later if we lean how to make it work.
| if (!uuid_is_null(opts->network_id) && opts->operation_mode != VMNET_HOST_MODE) { | ||
| ERROR("--network-id requires --operation-mode set to 'host'"); | ||
| exit(EXIT_FAILURE); | ||
| } |
There was a problem hiding this comment.
this is not really related to the network options (--start-address, --end-address, --subnet-mask). Adding it here hides the check. We better add the check in the shared/bridged mode blocks - even if we have to repeat it, it makes the code more clear.
| if (options.subnet_mask != NULL) { | ||
| ERROR("[runner] conflicting arguments: --network cannot be used with --subnet-mask"); | ||
| exit(EXIT_FAILURE); | ||
| } |
There was a problem hiding this comment.
Looks like leftover from the previous version - not needed and not related to this change.
| ERROR("[runner] conflicting arguments: --network cannot be used with --subnet-mask"); | ||
| exit(EXIT_FAILURE); | ||
| } | ||
| if (options.subnet_mask != NULL) { |
| "--subnet-mask", help="The IPv4 subnet mask (string) to use on the interface." | ||
| ) | ||
| p.add_argument( | ||
| "--network-id", |
There was a problem hiding this comment.
We need to validate that the argument is a UUID:
type=validate_uuid,def validate_uuid(s):
parse and raise if s is not a uuid...
return parsed uuid...| if args.network_id: | ||
| p.error("--network cannot be used with --network-id") | ||
|
|
||
| if args.operation_mode == "bridged": |
There was a problem hiding this comment.
Missing validation that args.network_id is None
| if not args.shared_interface: | ||
| p.error("--shared-interface required for --operation-mode=bridged") | ||
| if args.enable_isolation: | ||
| p.error("--enable-isolation not compatible with --operation-mode=bridged") |
There was a problem hiding this comment.
Needs another block for shared interface rejecting non-None --network-id.
|
Tested locally with: I ran this after the full tests, so the host network (192.168.128.0/24) was still used by vmnet-broker. We got a new network (192.168.129.0/24) and ping works. |
|
The missing part to complete this work: learn how to use this with the run script to create 3 vms using host mode and 2 network ids:
vm1 and vm2 will be on same bridge (bridge100) and network (192.168.128.0/24) and can communicate. You may need to tweak the cloud-init configuration to assign static ip to the vms, or find how to add dhcpd to the network. Or maybe you have another way to test this mode? Testing this without --host-ip-address and --host-subnet-mask will be tricky, since you must provide static ip to the vm, but you cannot do without knowing which network you get from vmnet. |
|
@nirs do you want a CI workflow for this, or just a working |
|
Testing % shared_id=$(uuidgen)
% for vm in test0 test1; do ./run $vm --operation-mode=host --network-id=$shared_id & done
[ ... ] INFO VM is ready at 192.168.128.209
[ ... ] INFO VM is ready at 192.168.128.184
% ./run test2 --operation-mode=host --network-id=$(uuidgen) &
[ ... ] INFO VM is ready at 192.168.129.111
% ssh ubuntu@192.168.209 ping -c 5 -t 10 192.168.128.184
PING 192.168.128.184 (192.168.128.184) 56(84) bytes of data.
64 bytes from 192.168.128.184: icmp_seq=1 ttl=64 time=0.319 ms
64 bytes from 192.168.128.184: icmp_seq=2 ttl=64 time=0.469 ms
64 bytes from 192.168.128.184: icmp_seq=3 ttl=64 time=0.610 ms
64 bytes from 192.168.128.184: icmp_seq=4 ttl=64 time=1.02 ms
64 bytes from 192.168.128.184: icmp_seq=5 ttl=64 time=0.659 ms
% ssh ubuntu@192.168.128.209 ping -c 5 -t 10 192.168.129.111
ping: connect: Network is unreachableBeware of flakiness due to address conflicts, my IPAM strategy without I tried writing a workflow, but I think I kept running out of memory, many runs hung indefinitely or timed out despite the artifacts looking correct. I was able to get things to work once on |
| if subnet_mask == "0.0.0.0" and host_address == "0.0.0.0": | ||
| # This happens on macOS < 15, let's assign a random subnet | ||
| # This will make tests flaky | ||
| subnet_mask = "255.255.255.0" | ||
| host_address = "192.168.55.1" |
There was a problem hiding this comment.
Still need to test this, no need for CI just yet.
No need to update or add a workflow. The first step is testing this manually, maybe modding the run script to support static ip. This is good enough to prove that this feature works. This is enough to review this change. The next step is to document how to test this feature in docs/development.md. This should allow developers to test this feature manually. This is enough o merge this change. The next step is to modify the run script to support static ip. The vm should annoys the mDNS name and the ip address, allowing access to the vm using mDNS. If mDNS does not work, the user can use the static ip. It can work like this: This will not work before we implement --host-ip-address and --host-subnet-mask, and optional, since this is an esoteric feature. The last step is adding CI for this, but this will be very hard. We cannot create vms in host mode since we depend on installing packages. So this means creating vm in shared mode, stopping it and starting it in host mode. This is very slow and we are limited to 5 concurrent runners. I don't want to make the slow CI run 2-3 times slower for testing esoteric feature. If we will have faster runners that can create a vm in 15 seconds and start it in 5 seconds, it can become feasible to test this in the CI. |
Nice! But assigning random tips is not needed - this will be much simpler: It cannot be reliable before we support --host-ip-address and --host-subnet-mask, depending on getting networks 192.168.128.0/24 and 192.168.129.0/24.
If we set the static ip using cloud-init the ip address will remain when stating the vm.
Don't waste time on this, this will be very hard, very slow, and not reliable enough on the old GitHub intel runners.
macos-26-intel sounds interesting - if they are available we should switch to these runners. |
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Let's amend the first commit with the validation fixes.
| data.update(DISTROS[vm.distro]) | ||
| # If using static addresses, don't do mDNS setup | ||
| if not vm.ip_address: | ||
| data.update(DISTROS[vm.distro]) |
There was a problem hiding this comment.
mDNS is important - this is huge quality of life feature. If we add static ip support we need to try hard to make it work with mDNS.
Creating vm requires installing packages so it cannot work in host mode. We can create a vm in shared mode, stop it , and start it again in host mode.
| """ | ||
| if self.ip_address: | ||
| interface = ipaddress.IPv4Interface(self.ip_address) | ||
| return str(interface.ip) |
There was a problem hiding this comment.
I want to always have mDNS - if we need to access the vm by ip_address we can use:
vm.ip_address or vm.fqdn()
When user creates a vm with ip address, they can use the ip address to the connect to the vm. They don't need the logs showing the ip address.
| errno.EHOSTUNREACH, | ||
| errno.ECONNREFUSED, | ||
| ]: | ||
| time.sleep(5) |
There was a problem hiding this comment.
Sleeping will add unwanted delay during start and is not needed since when we access the vm via the mDNS address we block in the kernel up to 5 seconds per call.
If accessing with the ip address fail quickly and this becomes a busy loop, we can do this:
start = time.monotonic()
try:
connect ...
except OSError as e:
log ...
if time.monotonic() - start < 0.5:
time.sleep(1)This is good change regardless to protected from unexpected early return that we did not experience yet.
Minikube uses the same way:
https://github.com/kubernetes/minikube/blob/2af2c30ef6dfd5b349c77bc1018dff74c8d89309/pkg/drivers/common/ssh.go#L70
| "to the VM are defined using a random number generator seeded from the " | ||
| "name of the VM. See assign_ip_address for details. Changes to the static " | ||
| "IP address between runs are not propagated, and you should manually delete " | ||
| "the disk.img and cidata.iso files for the VM to be reachable." |
There was a problem hiding this comment.
This is too complicated and magical. Using static ip should be simple - the user specs the ip and responsible for avoiding clashes with other vms or devices on the network.
So the comment should mention that this requires the --ip-address option.
| if args.network_id and args.connection == "runner": | ||
| # Without an option to set the host address, --network-id cannot be used with | ||
| # --connection=runner | ||
| p.error("--network-id is not compatible with --connection=runner") |
There was a problem hiding this comment.
How we can run the vm using fd/socket but we cannot run using the runner?
| args, helper.interface[testing.VMNET_MAC_ADDRESS], fd=vm_sock.fileno() | ||
| args, | ||
| helper.interface[testing.VMNET_MAC_ADDRESS], | ||
| ip_address=assign_ip_address(helper, args), |
There was a problem hiding this comment.
Let's simplify - let the user set the ip:
ip_address=args.ip_address
| parsed = uuid.UUID(arg) | ||
| if parsed == uuid.NIL: | ||
| raise ValueError("Nil UUID will be ignored") | ||
| return parsed |
Ok I misunderstood, I thought we wanted a working
This was causing flakiness because
I think systemd-resolved ships with images for fedora & ubuntu, and has mDNS support. I'll try it out. Alpine doesn't, but that's a great workaround! |
One issue - when restarting a vm cloud-init will not reconfigure the vm unless we generate new cloud-init disk (with new metadata instance-id). So to reconfigure the ip address in host mode we must:
Example flow: We can document this flow in docs/developement.md I suggest you start with the docs changes before spending time on the implementation. We should agree on the direction we go first. |
Ok! What should the scope of this PR be? Do you plan to merge with --host-ip-address in |
I think we will have to release --network-id with --host-ip-address and --host-subnet-mask. There is no way to work with static ip if you don't control the network. You can simply add the next commit with the docs. When we agree on how it should work we can implement it. We can squash the docs commit and implementation later if needed. |
| ### Without `--network-id` | ||
|
|
||
| On most networks, the DHCP range can be constrained using `--start-address`, | ||
| `--end-address` and `--subnet-mask`. Addresses outside this range are still | ||
| routable as long as they belong to the same subnet. A static IP can be set with | ||
| `--guest-ip-address`: | ||
|
|
||
| ```console | ||
| ./run vm3 \ | ||
| --start-address 192.168.12.1 \ | ||
| --end-address 192.168.12.128 \ | ||
| --subnet-mask 255.255.255.0 \ | ||
| --guest-ip-address 192.168.12.129 | ||
| ``` |
There was a problem hiding this comment.
We would get this for free by implementing static addressing for --network-id. Happy to remove since it's out of scope for this PR.
| The following options are available for more control: | ||
| - `--host-subnet-mask` is the `vmnet-helper` option to set the subnet mask | ||
| when a network ID is set. Defaults to 255.255.255.0. | ||
| - `--host-ip-address` is the `vmnet-helper` option to set the host's IP | ||
| address when a network ID is set. When used with `./run`, defaults to the | ||
| first address in the `--guest-ip-address` subnet. |
There was a problem hiding this comment.
Not needed for the bare minimum, but these will be simple pass-through options to vmnet-helper once they are implemented there, so free
|
Also it might make sense for me to do two different PRs related to ./run first:
Neither of these are strictly dependent on each other, or --network-id, but we need them to test it. |
It should work now - can you share your plan on this change? You can open another issue to describe the change.
Interesting, can be useful feature to use shared network with dhcp but use static ip. Open another issue for this? Show example usage and explain the use case. |
2 participants
--network-id=can be set to isolate the interface to the network with the specified UUID. DHCP service is not provided on this network.Fixes #120