gvforwarder as a systemd service #1052
Conversation
Reviewer's Guide by SourceryThis pull request refactors the setup for the gvisor-tap-vsock forwarder. It adds the creation of a tap device using No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @praveenkumar - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider making the hardcoded MAC address
5A:94:EF:E4:0C:EEconfigurable instead of embedding it directly in the script. - Extracting the
gvforwarderbinary from a container might be fragile; consider alternative methods like packaging or building from source. - Creating the systemd unit file via a
teeheredoc could be replaced by copying a dedicated unit file for better readability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # create the tap device interface with specified mac address | ||
| # this mac address is used to allocate a specific IP to the VM | ||
| # 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 |
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling potential errors when adding the tap device.
Capture and check the nmcli command’s exit status (e.g., check $?) so any failures in creating the tap interface are detected and handled.
| # create the tap device interface with specified mac address | |
| # this mac address is used to allocate a specific IP to the VM | |
| # 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 | |
| # create the tap device interface with specified mac address | |
| # this mac address is used to allocate a specific IP to the VM | |
| # 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 | |
| status=\$? | |
| if [ \$status -ne 0 ]; then | |
| echo "Error: Failed to add tap device interface. nmcli exit status: \$status" | |
| exit \$status | |
| fi | |
| EOF |
|
/retest |
praveenkumar
force-pushed
the
gvforwarder-service
branch
from
0c68848 to
19d1eff
Compare
|
Is it the same as #1003 but against a different branch? or are there some differences? |
It is same but against |
|
/retest |
| Description=gvisor-tap-vsock Network Traffic Forwarder | ||
| After=NetworkManager.service | ||
| BindsTo=sys-devices-virtual-net-%i.device | ||
| After=sys-devices-virtual-net-%i.device |
There was a problem hiding this comment.
Do we need to add Before=nodeip-configuration.service as in #1054 ?
There was a problem hiding this comment.
I don't know but I can try to do that also.
| tee /etc/systemd/system/gv-user-network@.service <<TEE | ||
| [Unit] | ||
| Description=gvisor-tap-vsock Network Traffic Forwarder | ||
| After=NetworkManager.service |
There was a problem hiding this comment.
Should this be Before=NetworkManager.service ? otherwise it starts very late after boot
There was a problem hiding this comment.
@anjannath networkManager is responsible to activate tun/tap device so it that is not even active how would this work.
There was a problem hiding this comment.
Should this be
Before=NetworkManager.service? otherwise it starts very late after boot
Is it NetworkManager.service which would start late? or network-online?
Since we have After=sys-devices-virtual-net-%i.device , do we even need the reference to NetworkManager.service?
There was a problem hiding this comment.
I spoke with Anjan, and the TAP creation should be independent of the network which configures. But the current, After is too undetermined, as it can start at any point after; anything with priority or dependency goes first.
There was a problem hiding this comment.
In short, rather use Before than After statements, as that ensures the order.
praveenkumar
force-pushed
the
gvforwarder-service
branch
from
19d1eff to
6c6f66f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
praveenkumar
force-pushed
the
gvforwarder-service
branch
from
6c6f66f to
7669205
Compare
- Create a tap device using nmcli with a hardcoded mac address - Start gvforwarder systemd service which will use this device Signed-off-by: vyasgun <vyasgun20@gmail.com>
Signed-off-by: Praveen Kumar <kumarpraveen.nitdgp@gmail.com>
praveenkumar
force-pushed
the
gvforwarder-service
branch
from
7669205 to
f04603b
Compare
|
/retest |
| @@ -16,7 +16,7 @@ INSTALL_DIR=${1:-crc-tmp-install-data} | |||
| OPENSHIFT_VERSION=$(${JQ} -r .clusterInfo.openshiftVersion $INSTALL_DIR/crc-bundle-info.json) | |||
| BASE_DOMAIN=$(${JQ} -r .clusterInfo.baseDomain $INSTALL_DIR/crc-bundle-info.json) | |||
| BUNDLE_TYPE=$(${JQ} -r .type $INSTALL_DIR/crc-bundle-info.json) | |||
| ADDITIONAL_PACKAGES="cloud-init" | |||
| ADDITIONAL_PACKAGES="cloud-init gvisor-tap-vsock-gvforwarder" | |||
There was a problem hiding this comment.
This does mean we need to keep this package up to date along with Podman's timeline/schedule.
There was a problem hiding this comment.
@gbraad This is subpackage and I think as of now it is not used by podman machine but build everytime there is release of gvisor-tap-vsock so we can just use it without maintaining (like building ourself).
praveenkumar
changed the title
|
/cherry-pick release-4.19 |
|
@praveenkumar: once the present PR merges, I will cherry-pick it on top of In response to this:
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. |
|
/cherry-pick master |
|
@praveenkumar: once the present PR merges, I will cherry-pick it on top of In response to this:
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. |
github-project-automation
bot
moved this from Work In Progress
to Done
in Project planning: crc
|
@praveenkumar: new pull request created: #1060 In response to this:
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. |
|
@praveenkumar: new pull request created: #1061 In response to this:
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. |
Summary by Sourcery
Configure
gvforwarder(gvisor-tap-vsock) to run as a native systemd service instead of a container.Build:
tap0network interface usingnmcliwith a fixed MAC address during VM setup.gvforwarderbinary directly onto the host system.gv-user-network@.service) to manage thegvforwarderprocess.gvisor-tap-vsock.