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.

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

Shared network for vfkit driver using vmnet-helper #20501

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

nirs
Copy link
Contributor

@nirs nirs commented Mar 7, 2025

Add new network option for vfkit "vment-shared", connecting vfkit to the
vmnet shared network. Clusters using this network can access other
clusters in the same network, similar to socket_vment with QEMU driver.

If network is not specified, we default to the "nat" network, keeping
the previous behavior. If network is "vment-shared", the vfkit driver
manages 2 processes: vfkit and vmnet-helper.

Like vfkit, vmnet-helper is started in the background, in a new process
group, so it not terminated if the minikube process group is terminate.

Since vment-helper requires root to start the vmnet interface, we start
it with sudo, creating 2 child processes. vment-helper drops privileges
immediately after starting the vment interface, and run as the user and
group running minikube.

Stopping the cluster will stop sudo, which will stop the vmnet-helper
process. Deleting the cluster kill both sudo and vment-helper by killing
the process group.

This change is not complete, but it is good enough to play with the new
shared network.

Example usage:

  1. Install vmnet-helper:
    https://github.com/nirs/vmnet-helper?tab=readme-ov-file#installation

  2. Setup vment-helper sudoers rule:
    https://github.com/nirs/vmnet-helper?tab=readme-ov-file#granting-permission-to-run-vmnet-helper

  3. Start 2 clusters with vmnet-shared network:

% minikube start -p c1 --driver vfkit --network vment-shared
...

% minikube start -p c2 --driver vfkit --network vmnet-shared
...

% minikube ip -p c1
192.168.105.18

% minikube ip -p c2
192.168.105.19
  1. Both cluster can access the other cluster:
% minikube -p c1 ssh -- ping -c 3 192.168.105.19
PING 192.168.105.19 (192.168.105.19): 56 data bytes
64 bytes from 192.168.105.19: seq=0 ttl=64 time=0.621 ms
64 bytes from 192.168.105.19: seq=1 ttl=64 time=0.989 ms
64 bytes from 192.168.105.19: seq=2 ttl=64 time=0.490 ms

--- 192.168.105.19 ping statistics ---
3 packets transmitted, 3 packets received, 0% packet loss
round-trip min/avg/max = 0.490/0.700/0.989 ms

% minikube -p c2 ssh -- ping -c 3 192.168.105.18
PING 192.168.105.18 (192.168.105.18): 56 data bytes
64 bytes from 192.168.105.18: seq=0 ttl=64 time=0.289 ms
64 bytes from 192.168.105.18: seq=1 ttl=64 time=0.798 ms
64 bytes from 192.168.105.18: seq=2 ttl=64 time=0.993 ms

--- 192.168.105.18 ping statistics ---
3 packets transmitted, 3 packets received, 0% packet loss
round-trip min/avg/max = 0.289/0.693/0.993 ms

To complete this work we need:

  • Update start flags to consider network options for vfkit
  • Validate that vmnet is installed when using --network vment-shared
  • Document vfkit driver network options
  • Install vmnet-helper on the CI hosts
  • Add test using --network vmnet-shared

Based on #20506

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 7, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nirs
Once this PR has been reviewed and has the lgtm label, please assign spowelljr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Hi @nirs. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 7, 2025
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@nirs
Copy link
Contributor Author

nirs commented Mar 7, 2025

@afbjorklund can you review this?

@nirs nirs force-pushed the vmnet-helper branch 3 times, most recently from 86b449f to 6bf1d56 Compare March 9, 2025 22:38
@nirs
Copy link
Contributor Author

nirs commented Mar 9, 2025

Example machine config when vmnet-shared is used:

{
    "ConfigVersion": 3,
    "Driver": {
        "IPAddress": "192.168.105.21",
        "MachineName": "minikube",
        "SSHUser": "docker",
        "SSHPort": 22,
        "SSHKeyPath": "",
        "StorePath": "/Users/nir/.minikube",
        "SwarmMaster": false,
        "SwarmHost": "",
        "SwarmDiscovery": "",
        "Boot2DockerURL": "file:///Users/nir/.minikube/cache/iso/arm64/minikube-v1.35.0-arm64.iso",
        "DiskSize": 20000,
        "CPU": 2,
        "Memory": 6000,
        "Cmdline": "",
        "ExtraDisks": 0,
        "Network": "vmnet-shared",
        "MACAddress": "de:a2:9c:71:3c:f7",
        "VmnetHelper": {
            "MachineDir": "/Users/nir/.minikube/machines/minikube",
            "InterfaceID": "a0c43efb-2dcc-4abb-a310-568782c5dc7a"
        }
    },
    "DriverName": "vfkit",
    "HostOptions": {
        "Driver": "",
        "Memory": 0,
        "Disk": 0,
        "EngineOptions": {
            "ArbitraryFlags": null,
            "Dns": null,
            "GraphDir": "",
            "Env": null,
            "Ipv6": false,
            "InsecureRegistry": [
                "10.96.0.0/12"
            ],
            "Labels": null,
            "LogLevel": "",
            "StorageDriver": "",
            "SelinuxEnabled": false,
            "TlsVerify": false,
            "RegistryMirror": [],
            "InstallURL": "https://get.docker.com"
        },
        "SwarmOptions": {
            "IsSwarm": false,
            "Address": "",
            "Discovery": "",
            "Agent": false,
            "Master": false,
            "Host": "",
            "Image": "",
            "Strategy": "",
            "Heartbeat": 0,
            "Overcommit": 0,
            "ArbitraryFlags": null,
            "ArbitraryJoinFlags": null,
            "Env": null,
            "IsExperimental": false
        },
        "AuthOptions": {
            "CertDir": "/Users/nir/.minikube",
            "CaCertPath": "/Users/nir/.minikube/certs/ca.pem",
            "CaPrivateKeyPath": "/Users/nir/.minikube/certs/ca-key.pem",
            "CaCertRemotePath": "",
            "ServerCertPath": "/Users/nir/.minikube/machines/server.pem",
            "ServerKeyPath": "/Users/nir/.minikube/machines/server-key.pem",
            "ClientKeyPath": "/Users/nir/.minikube/certs/key.pem",
            "ServerCertRemotePath": "",
            "ServerKeyRemotePath": "",
            "ClientCertPath": "/Users/nir/.minikube/certs/cert.pem",
            "ServerCertSANs": null,
            "StorePath": "/Users/nir/.minikube"
        }
    },
    "Name": "minikube"
}

@nirs
Copy link
Contributor Author

nirs commented Mar 9, 2025

Example multi-node cluster

% minikube start --network vmnet-shared --nodes 2 --cni auto
😄  minikube v1.35.0 on Darwin 15.3.1 (arm64)
✨  Using the vfkit (experimental) driver based on user configuration
❗  --network flag is only valid with the docker/podman, KVM and Qemu drivers, it will be ignored
👍  Starting "minikube" primary control-plane node in "minikube" cluster
🔥  Creating vfkit VM (CPUs=2, Memory=4050MB, Disk=20000MB) ...
📦  Preparing Kubernetes v1.32.2 on containerd 1.7.23 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔗  Configuring CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: default-storageclass, storage-provisioner

👍  Starting "minikube-m02" worker node in "minikube" cluster
🔥  Creating vfkit VM (CPUs=2, Memory=4050MB, Disk=20000MB) ...
🌐  Found network options:
    ▪ NO_PROXY=192.168.105.22
📦  Preparing Kubernetes v1.32.2 on containerd 1.7.23 ...
    ▪ env NO_PROXY=192.168.105.22
    > kubelet.sha256:  64 B / 64 B [-------------------------] 100.00% ? p/s 0s
    > kubectl.sha256:  64 B / 64 B [-------------------------] 100.00% ? p/s 0s
    > kubeadm.sha256:  64 B / 64 B [-------------------------] 100.00% ? p/s 0s
    > kubectl:  53.25 MiB / 53.25 MiB [------------] 100.00% 37.13 MiB p/s 1.6s
    > kubelet:  71.75 MiB / 71.75 MiB [------------] 100.00% 12.43 MiB p/s 6.0s
    > kubeadm:  66.81 MiB / 66.81 MiB [------------] 100.00% 10.11 MiB p/s 6.8s
🔎  Verifying Kubernetes components...
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

% kubectl get node                                                           
NAME           STATUS   ROLES           AGE     VERSION
minikube       Ready    control-plane   5m1s    v1.32.2
minikube-m02   Ready    <none>          4m40s   v1.32.2

% kubectl get node -o jsonpath='{.items[*].status.addresses[0].address}{"\n"}'
192.168.105.22 192.168.105.23

Each node get its own vment-helper process:

% ps au | grep vmnet-helper | grep -v grep 
nir  60260   0.0  0.0 410743984   3792 s020  S     1:07AM   0:00.71 /opt/vmnet-helper/bin/vmnet-helper --fd 21 --interface-id c9ae713b-5937-42de-9018-bbb95a425506
root 60259   0.0  0.0 410737168   6448 s020  S     1:07AM   0:00.01 sudo --non-interactive --close-from 22 /opt/vmnet-helper/bin/vmnet-helper --fd 21 --interface-id c9ae713b-5937-42de-9018-bbb95a425506
nir  60254   0.0  0.0 410735792   3728 s020  S     1:07AM   0:00.73 /opt/vmnet-helper/bin/vmnet-helper --fd 13 --interface-id cf62fb28-2533-4c25-8d35-1eef1736f707
root 60253   0.0  0.0 410754576   6992 s020  S     1:07AM   0:00.01 sudo --non-interactive --close-from 14 /opt/vmnet-helper/bin/vmnet-helper --fd 13 --interface-id cf62fb28-2533-4c25-8d35-1eef1736f707

@afbjorklund
Copy link
Collaborator

@afbjorklund can you review this?

I could take a look at it later perhaps, but one of the minikube maintainers will still need to "take over" the vfkit driver.

Probably should have an issue, as well.

Copy link

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

I also noticed several vment typos in commit logs

@@ -0,0 +1,79 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

maybe check it is not installed and Suggest ppl to install it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to use NotFoundVmnetHelper in the same way we handle NotFoundSocketVmnet.

If you ask for --network vmnet-shared and vment-helper is not installed or not configured properly, you will get link to the docs on how to install and configure it.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2025
@nirs nirs mentioned this pull request Mar 14, 2025
nirs added 10 commits March 16, 2025 00:36
Current code contains multiple implementations for managing a process
using pids, with various issues:

- Some are unsafe, terminating a process by pid without validating that
  the pid belongs to the right process. Some use unclear
- Using unclear terms like checkPid() (what does it mean?)
- Some are missing tests

Let's clean up the mess by introducing a process package. The package
provides:

- process.WritePidfile(): write a pid to file
- process.ReadPidfile(): read pid from file
- process.Exists(): tells if process matching pid and name exists
- process.Terminate() terminates a process matching pid and name
- process.Kil() kill a process matching pid and name

The library is tested on linux, darwin, and windows. On windows we don't
have a standard way to terminate a process gracefully, so
process.Terminate() is the same as process.Kill().

I want to use this package in vfkit and the new vment package, and later
we can use it for qemu, hyperkit, and other code using managing
processes with pids.
- Simplify GetState() using process.ReadPidfile()
- Simplify Start() using process.WritePidfile()
GetState() had several issues:

- When accessing vfkit HTTP API, we handled only "running",
  "VirtualMachineStateRunning", "stopped", and
  "VirtualMachineStateStopped", but there are other 10 possible states,
  which we handled as state.None, when vfkit is running and need to be
  stopped. This can lead to wrong handling in the caller.

- When handling "stopped" and "VirtualMachineStateStopped" we returned
  state.Stopped, but did not remove the pidfile. This can cause
  termination of unrelated process or reporting wrong status when the
  pid is reused.

- Accessing the HTTP API will fail after we stop or kill it. This
  cause GetState() to fail when the process is actually stopped, which
  can lead to unnecessary retries and long delays (kubernetes#20503).

- When retuning state.None during Remove(), we use tried to do graceful
  shutdown which does not make sense in minikube delete flow, and is not
  consistent with state.Running handling.

Accessing vfkit API to check for state does not add much value for our
used case, checking if the vfkit process is running, and it is not
reliable.

Fix all the issues by not using the HTTP API in GetState(), and use only
the process state. We still use the API for stopping and killing vfkit
to do graceful shutdown. This also simplifies Remove(), since we need to
handle only the state.Running state.

With this change we consider vfkit as stopped only when the process does
not exist, which takes about 3 seconds after the state is reported as
"stopped".

Example stop flow:

    I0309 18:15:40.260249   18857 main.go:141] libmachine: Stopping "minikube"...
    I0309 18:15:40.263225   18857 main.go:141] libmachine: set state: {State:Stop}
    I0309 18:15:46.266902   18857 main.go:141] libmachine: Machine "minikube" was stopped.
    I0309 18:15:46.267122   18857 stop.go:75] duration metric: took 6.127761459s to stop

Example delete flow:

    I0309 17:00:49.483078   18127 out.go:177] * Deleting "minikube" in vfkit ...
    I0309 17:00:49.499252   18127 main.go:141] libmachine: set state: {State:HardStop}
    I0309 17:00:49.569938   18127 lock.go:35] WriteFile acquiring /Users/nir/.kube/config: ...
    I0309 17:00:49.573977   18127 out.go:177] * Removed all traces of the "minikube" cluster.
Previously we did not check the process name when checking a pid from a
pidfile. If the pidfile became state we would assume that vfkit is
running and try to stop it via the HTTP API, which would never succeed.
Now we detect stale pidfile and remove it.
If setting vfkit state to "Stop" fails, we used to return an error.
Retrying the operation may never succeed.

Fix by falling back to terminating vfkit using a signal. This terminates
vfkit immediately similar to HardStop[1].

We can still fail if the pidfile is corrupted but this is unlikely and
requires manual cleanup.

In the case when we are sure the vfkit process does not exist, we remove
the pidfile immediately, avoiding leftover pidfile if the caller does
not call GetState() after Stop().

[1] crc-org/vfkit#284
We know that setting the state to `HardStop` typically fails:

    I0309 19:19:42.378591   21795 out.go:177] 🔥  Deleting "minikube" in vfkit ...
    W0309 19:19:42.397472   21795 delete.go:106] remove failed, will retry: kill: Post "http://_/vm/state": EOF

This may lead to unnecessary retries and delays. Fix by falling back to
sending a SIGKILL signal.

Example delete flow when setting vfkit state fails:

    I0309 20:07:41.688259   25540 out.go:177] 🔥  Deleting "minikube" in vfkit ...
    I0309 20:07:41.712017   25540 main.go:141] libmachine: Failed to set vfkit state to 'HardStop': Post "http://_/vm/state": EOF
Remove temporary and unneeded mac variable. It is easier to follow the
code when we use d.MACAddress.
System state changes should be more visible to make debugging easier.
The package manages the vmnet-helper[1] child process, providing
connection to the vmnet network without running the guest as root.

We will use vmnet-helper for the vfkit driver, which does not have a way
to use shared network, when guests can access other guest in the
network.  We can use it later with the qemu driver as alternative to
socket_vmnet.

[1] https://github.com/nirs/vmnet-helper
Add new network option for vfkit "vmnet-shared", connecting vfkit to the
vmnet shared network. Clusters using this network can access other
clusters in the same network, similar to socket_vmnet with QEMU driver.

If network is not specified, we default to the "nat" network, keeping
the previous behavior. If network is "vmnet-shared", the vfkit driver
manages 2 processes: vfkit and vmnet-helper.

Like vfkit, vmnet-helper is started in the background, in a new process
group, so it not terminated if the minikube process group is terminate.

Since vmnet-helper requires root to start the vmnet interface, we start
it with sudo, creating 2 child processes. vmnet-helper drops privileges
immediately after starting the vmnet interface, and run as the user and
group running minikube.

Stopping the cluster will stop sudo, which will stop the vmnet-helper
process. Deleting the cluster kill both sudo and vmnet-helper by killing
the process group.

This change is not complete, but it is good enough to play with the new
shared network.

Example usage:

1. Install vmnet-helper:
   https://github.com/nirs/vmnet-helper?tab=readme-ov-file#installation

2. Setup vmnet-helper sudoers rule:
   https://github.com/nirs/vmnet-helper?tab=readme-ov-file#granting-permission-to-run-vmnet-helper

3. Start 2 clusters with vmnet-shared network:

    % minikube start -p c1 --driver vfkit --network vmnet-shared
    ...

    % minikube start -p c2 --driver vfkit --network vmnet-shared
    ...

    % minikube ip -p c1
    192.168.105.18

    % minikube ip -p c2
    192.168.105.19

4. Both cluster can access the other cluster:

    % minikube -p c1 ssh -- ping -c 3 192.168.105.19
    PING 192.168.105.19 (192.168.105.19): 56 data bytes
    64 bytes from 192.168.105.19: seq=0 ttl=64 time=0.621 ms
    64 bytes from 192.168.105.19: seq=1 ttl=64 time=0.989 ms
    64 bytes from 192.168.105.19: seq=2 ttl=64 time=0.490 ms

    --- 192.168.105.19 ping statistics ---
    3 packets transmitted, 3 packets received, 0% packet loss
    round-trip min/avg/max = 0.490/0.700/0.989 ms

    % minikube -p c2 ssh -- ping -c 3 192.168.105.18
    PING 192.168.105.18 (192.168.105.18): 56 data bytes
    64 bytes from 192.168.105.18: seq=0 ttl=64 time=0.289 ms
    64 bytes from 192.168.105.18: seq=1 ttl=64 time=0.798 ms
    64 bytes from 192.168.105.18: seq=2 ttl=64 time=0.993 ms

    --- 192.168.105.18 ping statistics ---
    3 packets transmitted, 3 packets received, 0% packet loss
    round-trip min/avg/max = 0.289/0.693/0.993 ms
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants