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

Revert MacOS regression introduced in 600910caefc729efaaae16219be51d081284a104 #385

Conversation

maciej-szlosarczyk
Copy link

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

Original issue reported in podman here:

containers/podman#23616

Copy link
Contributor

openshift-ci bot commented Aug 20, 2024

[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:

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

Original issue reported in podman here:

containers/podman#23616

Signed-off-by: Maciej Szlosarczyk <[email protected]>
@cfergeau
Copy link
Collaborator

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 inetaf/tcpproxy@2862066 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 (inetaf/tcpproxy@3ce5804) 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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

Closing this, this is replaced by #386

@cfergeau cfergeau closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants