search

containers / gvisor-tap-vsock

A new network stack based on gVisor
Apache License 2.0
269 stars 50 forks source link

Revert MacOS regression introduced in 600910caefc729efaaae16219be51d081284a104 #385

Closed maciej-szlosarczyk closed 3 months ago

maciej-szlosarczyk commented 3 months ago

On MacOS, gvproxy 0.7.4 does not close connections properly, leading to resource exhaustion over time.

Original issue reported in podman here:

https://github.com/containers/podman/issues/23616

openshift-ci[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maciej-szlosarczyk Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Kubernetes 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](https://github.com/containers/gvisor-tap-vsock/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
cfergeau commented 3 months ago

We cannot patch code in vendor like this, as this code is only a copy of code from other repositories, in this case, it's a copy of github.com/inetaf/tcpproxy. If we modify it, the modifications will be overwritten next time someone runs go mod vendor.

The code that you want to change comes from this commit https://github.com/inetaf/tcpproxy/commit/2862066fc2a9405880f212f71230425bdfe9950e which is the commit just before the latest commit in inetaf/tcpproxy, and the latest commit is a rename of the go module.

What we can try to do is to change the inetaf/tcpproxy version we use in our go.mod file, and instead of using the latest commit, we could use the commit just before the one which seems problematic. The latest commit (https://github.com/inetaf/tcpproxy/commit/3ce58045626c8bc343a593c90354975e61b1817a) makes this harder than it should, but what I've done in https://github.com/cfergeau/gvisor-tap-vsock/tree/fix-macos-regression should work.

Can you build a gvproxy binary from this branch, and see if it helps with the regression you are seeing?

maciej-szlosarczyk commented 3 months ago

Hi @cfergeau! Yes, this definitely works, I just checked against your branch and there aren't any hangers-on:

% lsof -nP -iTCP | grep gvproxy
gvproxy   26399 maciej   10u  IPv4 0xe8e0d84786cf3972      0t0  TCP 127.0.0.1:57485 (LISTEN)
gvproxy   26399 maciej   33u  IPv6 0x9a475d1349cde53d      0t0  TCP *:5432 (LISTEN)
cfergeau commented 3 months ago

Hi @cfergeau! Yes, this definitely works, I just checked against your branch and there aren't any hangers-on:

% lsof -nP -iTCP | grep gvproxy
gvproxy   26399 maciej   10u  IPv4 0xe8e0d84786cf3972      0t0  TCP 127.0.0.1:57485 (LISTEN)
gvproxy   26399 maciej   33u  IPv6 0x9a475d1349cde53d      0t0  TCP *:5432 (LISTEN)

Great thanks for testing! I'll create a new PR from my branch then, this should be good enough while we take a closer look as to why this 'half close' commit is causing a regression.

cfergeau commented 3 months ago

Closing this, this is replaced by https://github.com/containers/gvisor-tap-vsock/pull/386