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

fix leaking TCP connections which leads to consistent conformance test failures #2358

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

gauravkghildiyal
Copy link
Member

@gauravkghildiyal gauravkghildiyal commented Aug 28, 2023

What type of PR is this?

/kind bug
/kind test

What this PR does / why we need it:

The problem:

  • At present, each HTTP request creates a new TCP connection within our tests.
  • We make keep-alive HTTP requests which means that after the HTTP request completes, we don't close the TCP connection.
  • While we are waiting for convergence, we continuously make HTTP requests which keep on leaking TCP connections.
# Example of active TCP connections to a Gateway with IP 42.54.109.216
❯ ss -tan | grep 42.54.109.216 | head -n 3
ESTAB      0      0               192.168.24.68:50606           42.54.109.216:80
ESTAB      0      0               192.168.24.68:51233           42.54.109.216:80
ESTAB      0      0               192.168.24.68:41312           42.54.109.216:80

# Count of active TCP connections keeps on increasing
❯ ss -tan | grep 42.54.109.216 | wc -l
130
  • If the implementation takes longer to program the Gateway, the leaks end up exhausting the maximum number of active connections to a single host
    • The limit of maximum active connections to a single destination may either come from the client machine, or from any NAT system which sits in front of the client machine where the tests are run.
  • On reaching the limit, we fail to make new HTTP requests and end up failing the tests.

Solution:

Solution is pretty straightforward: we disable HTTP keep-alives.

  • Even right now, although we use HTTP keep-alive, we are actually not making use of this feature since we always create a new TCP connection for each HTTP request. This means the change does not have any adverse effect as we just closes the old TCP connections which we are no longer using.
  • Given that we are just working with tests here, we may not want to over-optimize by reusing TCP connections and just use the simplest approach to fix this right now.

Which issue(s) this PR fixes:

Fixes #2357

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. kind/test labels Aug 28, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2023
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2023
Copy link
Contributor

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

DisableKeepAlives part lgtm

conformance/utils/roundtripper/roundtripper.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 28, 2023
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2023
@shaneutt shaneutt added this to the v0.8.0 milestone Aug 28, 2023
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, gauravkghildiyal, robscott

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

The pull request process is described 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

@robscott
Copy link
Member

Thanks @gauravkghildiyal!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2023
@k8s-ci-robot k8s-ci-robot merged commit 2ed9486 into kubernetes-sigs:main Aug 28, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
No open projects
6 participants