gvforwarder as a systemd service #1003

vyasgun · 2025-01-21T07:02:06Z

Create a tap device using nmcli with a hardcoded mac address
Start gvforwarder systemd service which will use this device

Based on the following code:
#673
cfergeau@03a4054

openshift-ci · 2025-01-21T07:02:10Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-01-21T07:02:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gbraad 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:

OWNERS

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

vyasgun · 2025-01-21T07:02:17Z

/test all

cfergeau · 2025-01-21T08:57:33Z

I'd recommend also picking up the changes from https://github.com/cfergeau/snc/commits/gvisor_service/ which update the unit files used in the PR to use a unit file close to https://github.com/containers/gvisor-tap-vsock/tree/main/contrib/systemd

With the current code, I still have this question/concern #673 (comment)

vyasgun · 2025-01-22T03:17:07Z

/retest

vyasgun · 2025-01-23T04:39:35Z

/retest

vyasgun · 2025-01-23T06:21:46Z

/retest

vyasgun · 2025-01-24T06:59:33Z

/retest

vyasgun · 2025-01-24T08:26:57Z

/retest

vyasgun · 2025-01-27T11:23:29Z

/retest

vyasgun · 2025-01-28T05:43:25Z

/retest

vyasgun · 2025-02-03T10:37:47Z

/retest

vyasgun · 2025-02-06T07:43:49Z

/retest

vyasgun · 2025-02-06T08:36:17Z

/retest

vyasgun · 2025-02-06T09:31:13Z

/retest

vyasgun · 2025-02-07T08:47:16Z

/retest

vyasgun · 2025-02-07T13:15:37Z

/retest

vyasgun · 2025-02-10T10:27:29Z

/retest

createdisk.sh

cfergeau

Looks good to me, thanks a lot for putting this into shape/testing it.

[email protected]

vyasgun · 2025-02-11T03:35:29Z

/test e2e-snc

- Create a tap device using nmcli with a hardcoded mac address - Start gvforwarder systemd service which will use this device Signed-off-by: vyasgun <[email protected]>

cfergeau · 2025-02-11T08:53:46Z

createdisk.sh

+# when tap device is in use.
+${SSH} core@${VM_IP} 'sudo bash -x -s' <<EOF
+  nmcli connection add type tun ifname tap0 con-name tap0 mode tap autoconnect yes 802-3-ethernet.cloned-mac-address 5A:94:EF:E4:0C:EE
+EOF


Isn't this equivalent to ${SSH} core@${VM_IP} 'sudo nmcli connection add type tun ifname tap0 con-name tap0 mode tap autoconnect yes 802-3-ethernet.cloned-mac-address 5A:94:EF:E4:0C:EE'?

In short; you are questioning why this needs to be wrapped in a sudo bash -x -s.
Would otherwise an error occur? I do not see characters that would be wrongly interpreted by the host shell (like zsh could do).

@anjannath How was this solved for the self-sufficient bundle?

this is not being changed for the self-sufficient bundle, its been tested with the existing situation which is that there is a container image which runs the gvforwarder and that container also has a dhcp client script which configures the interface using the dhcp service from gvisor-tap-vsock

i think we can also scp the NetworkManger config file to /etc/NetworkManager/system-connections instead of running nmcli commands, there's a config file in: https://github.com/containers/gvisor-tap-vsock/blob/main/contrib/networkmanager/vsock0.nmconnection

[connection] id=tap0 type=tun autoconnect=true interface-name=tap0 [tun] mode=2 [802-3-ethernet] cloned-mac-address=5A:94:EF:E4:0C:EE [ipv4] method=auto [proxy]

cfergeau · 2025-02-11T08:54:38Z

@praveenkumar can you take a look at this PR? you also looked into this in the past.

gbraad

I would approve, but I question the use of the command... first why invoked like this... though, even ... why is the creation of the tap not part of a systemd unit by itself? As in that case you can depend on it...

gbraad · 2025-02-17T05:00:33Z

createdisk.sh

  systemctl daemon-reload
-  systemctl enable gvisor-tap-vsock.service
+  systemctl enable [email protected]


... I like how this can be targeted with %i, but this 'depends' on actions performed previously by creating this device.

For this increment, this would work. But most likely would change with the self-sufficient bundle.

The nmcli command creates a Network Manager configuration file in the bundle, the self-sufficient bundle should be similar from that perspective?

gbraad · 2025-02-17T05:03:00Z

createdisk.sh

+# when tap device is in use.
+${SSH} core@${VM_IP} 'sudo bash -x -s' <<EOF
+  nmcli connection add type tun ifname tap0 con-name tap0 mode tap autoconnect yes 802-3-ethernet.cloned-mac-address 5A:94:EF:E4:0C:EE
+EOF


In short; you are questioning why this needs to be wrapped in a sudo bash -x -s.
Would otherwise an error occur? I do not see characters that would be wrongly interpreted by the host shell (like zsh could do).

@anjannath How was this solved for the self-sufficient bundle?

cfergeau · 2025-02-17T10:31:13Z

why is the creation of the tap not part of a systemd unit by itself? As in that case you can depend on it...

In a way the creation of the tap is part of a systemd unit, it's added to NetworkManager configuration files, which is started through systemd.
The gv-user-network unit has dependencies on the network device:

After=NetworkManager.service
BindsTo=sys-devices-virtual-net-%i.device
After=sys-devices-virtual-net-%i.device

praveenkumar · 2025-02-25T06:47:57Z

createdisk.sh

+  tee /etc/systemd/system/[email protected] <<TEE
+[Unit]
+Description=gvisor-tap-vsock Network Traffic Forwarder
+After=NetworkManager.service


Should we run this after NetworkManager-wait-online.service beacuse as per man NetworkManager-wait-online.service this make sure that delays reaching the network-online target until NetworkManager reports that the startup is completed on the D-Bus

Not sure NetworkManager-wait-online.service can complete before gv-user-network service is started, so I'm not sure it would work to order them the opposite way.

The way I see this gv-user-network service is running the forwarder command for an existing interface and NetworkManager-wait-online service make sure that interface exist because it waits until all the network profile is enabled. Or my understanding is wrong here?

The gv-user-network unit file has:

BindsTo=sys-devices-virtual-net-%i.device After=sys-devices-virtual-net-%i.device

This makes sure the interface exists before the gv-user-network unit tries to start.

man NetworkManager-wait-online.service says:

Startup is not complete as long as NetworkManager profiles are in an activating state.

When a device reaches the activate state depends on its configuration. For example, with a profile that has both IPv4 and IPv6 enabled, by default, NetworkManager considers the device as fully activated already when only one of the address families is ready.
[...]

From this, it is not clear when a tun interface reaches the activate state. If NM waits until it gets an IP for example, this means gvforwarder must be running before the interface "activates", in which case it's problematic to order it after NetworkManager-wait-online.service. Maybe a tun interface is activated before getting an IP, in which case it would be less problematic, but I don't know from just reading the man page.

However, ordering the unit after NetworkManager and after the tun device is available seems enough to me, I don't think ordering it after NetworkManager-wait-online.service brings us anything useful?

it is not clear when a tun interface reaches the activate state.

Yes and looking at the CI failure I think it hit the network failure so let's stick to NetworkManager.service only instead NetworkManager-wait-online.service

I tried this and we should keep the After value as it is

openshift-ci · 2025-02-26T14:28:01Z

@vyasgun: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-snc	`cf5affc`	link	true	`/test e2e-snc`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Jan 21, 2025

vyasgun force-pushed the spike/gvforwarder-service branch from fb9c40e to 6278601 Compare January 21, 2025 09:54

vyasgun force-pushed the spike/gvforwarder-service branch from 04595ac to 24924c0 Compare January 23, 2025 06:21

vyasgun force-pushed the spike/gvforwarder-service branch from 24924c0 to 327901a Compare January 24, 2025 06:40

vyasgun force-pushed the spike/gvforwarder-service branch from 327901a to 5264a19 Compare January 27, 2025 11:23

cfergeau assigned vyasgun Feb 5, 2025

vyasgun force-pushed the spike/gvforwarder-service branch from ccc602e to 4e0a92e Compare February 7, 2025 08:47

vyasgun force-pushed the spike/gvforwarder-service branch from 4e0a92e to a999631 Compare February 7, 2025 13:15

vyasgun force-pushed the spike/gvforwarder-service branch from a999631 to d1501b4 Compare February 10, 2025 10:26

vyasgun force-pushed the spike/gvforwarder-service branch 2 times, most recently from b83b47e to 53cf03b Compare February 10, 2025 13:10

vyasgun changed the title ~~[WIP] [Spike] gvforwarder as a systemd service~~ gvforwarder as a systemd service Feb 10, 2025

vyasgun marked this pull request as ready for review February 10, 2025 13:17

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 10, 2025

openshift-ci bot requested review from gbraad and praveenkumar February 10, 2025 13:17

vyasgun force-pushed the spike/gvforwarder-service branch from 53cf03b to 6cf746e Compare February 10, 2025 13:41

cfergeau reviewed Feb 10, 2025

View reviewed changes

createdisk.sh Outdated Show resolved Hide resolved

cfergeau reviewed Feb 10, 2025

View reviewed changes

[email protected] Outdated Show resolved Hide resolved

vyasgun force-pushed the spike/gvforwarder-service branch from 6cf746e to abece15 Compare February 11, 2025 03:37

gvforwarder as a systemd service

cf5affc

- Create a tap device using nmcli with a hardcoded mac address - Start gvforwarder systemd service which will use this device Signed-off-by: vyasgun <[email protected]>

vyasgun force-pushed the spike/gvforwarder-service branch from abece15 to cf5affc Compare February 11, 2025 03:38

cfergeau reviewed Feb 11, 2025

View reviewed changes

gbraad reviewed Feb 17, 2025

View reviewed changes

praveenkumar reviewed Feb 25, 2025

View reviewed changes

vyasgun force-pushed the spike/gvforwarder-service branch from f4bab4e to cf5affc Compare February 26, 2025 10:17

gvforwarder as a systemd service #1003

Are you sure you want to change the base?

gvforwarder as a systemd service #1003

Conversation

vyasgun commented Jan 21, 2025 • edited Loading

openshift-ci bot commented Jan 21, 2025

openshift-ci bot commented Jan 21, 2025

vyasgun commented Jan 21, 2025

cfergeau commented Jan 21, 2025

vyasgun commented Jan 22, 2025

vyasgun commented Jan 23, 2025

vyasgun commented Jan 23, 2025

vyasgun commented Jan 24, 2025

vyasgun commented Jan 24, 2025

vyasgun commented Jan 27, 2025

vyasgun commented Jan 28, 2025

vyasgun commented Feb 3, 2025

vyasgun commented Feb 6, 2025

vyasgun commented Feb 6, 2025

vyasgun commented Feb 6, 2025

vyasgun commented Feb 7, 2025

vyasgun commented Feb 7, 2025

vyasgun commented Feb 10, 2025

cfergeau left a comment

Choose a reason for hiding this comment

vyasgun commented Feb 11, 2025

Choose a reason for hiding this comment

gbraad Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

anjannath Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau commented Feb 11, 2025

gbraad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

cfergeau commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 26, 2025

vyasgun commented Jan 21, 2025 •

edited

Loading

gbraad Feb 17, 2025 •

edited

Loading

anjannath Feb 17, 2025 •

edited

Loading

gbraad Feb 17, 2025 •

edited

Loading

cfergeau Feb 26, 2025 •

edited

Loading