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.

Sign up for GitHub

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

Jump to bottom

fix: Separate out source and destination remote options #389

Merged
merged 3 commits into from
Apr 24, 2023
Merged

fix: Separate out source and destination remote options #389

merged 3 commits into from
Apr 24, 2023

Conversation

dkoshkin
Copy link
Contributor

@dkoshkin dkoshkin commented Apr 20, 2023 •
edited
Loading

No need to Clone delegateTransport on every function call, we can reuse the same http.Transport for every call.
Close idle connections.

@dkoshkin
fix: open file limit with TLSRoundTripper
965f502 
No need to Clone delegateTransport on every function call, we can reuse the same http.Transport for every call.
@github-actions github-actions bot added fix and removed fix labels Apr 20, 2023
@dkoshkin dkoshkin force-pushed the dkoshkin/fix-http-trasport-leak branch from 3512070 to 66b0a31 Compare April 21, 2023 22:18
@dkoshkin dkoshkin force-pushed the dkoshkin/fix-http-trasport-leak branch from 66b0a31 to 445f70c Compare April 21, 2023 22:28
@jimmidyson
fix: Separate out source and destination remote options
5fad80b 
This removes the need to configure TLS certificates per request, which was
leading to opening too many sockets, and hitting too many open files on
systems with reasonable ulimits.
@jimmidyson jimmidyson force-pushed the dkoshkin/fix-http-trasport-leak branch from 2f36429 to 5fad80b Compare April 24, 2023 13:36
@jimmidyson
Copy link
Contributor

@dkoshkin I refactored the TLS configuration and separated out options for src and dest registries. I think is now more understandable and reuses connections better. Could you test it out please?

@dkoshkin
Copy link
Contributor Author

@dkoshkin I refactored the TLS configuration and separated out options for src and dest registries. I think is now more understandable and reuses connections better. Could you test it out please?

Yep testing.

@dkoshkin
Copy link
Contributor Author

Looks good, see below snippet without a bunch of open connections.

$ lsof -p 7766
COMMAND    PID   USER   FD      TYPE DEVICE SIZE/OFF     NODE NAME
mindthega 7766 centos  cwd       DIR  259,1     4096   123232 /home/centos
mindthega 7766 centos  rtd       DIR  259,1      224       64 /
mindthega 7766 centos  txt       REG  259,1 57511936     3927 /home/centos/mindthegap
mindthega 7766 centos    0u      CHR  136,0      0t0        3 /dev/pts/0
mindthega 7766 centos    1u      CHR  136,0      0t0        3 /dev/pts/0
mindthega 7766 centos    2u      CHR  136,0      0t0        3 /dev/pts/0
mindthega 7766 centos    3u     IPv4  39015      0t0      TCP ip-172-31-6-22.us-west-2.compute.internal:47450->204.79.197.219:https (ESTABLISHED)
mindthega 7766 centos    4u  a_inode   0,10        0     6451 [eventpoll]
mindthega 7766 centos    5r     FIFO    0,9      0t0    35935 pipe
mindthega 7766 centos    6w     FIFO    0,9      0t0    35935 pipe
mindthega 7766 centos    7u     IPv4  30485      0t0      TCP localhost:36149 (LISTEN)
mindthega 7766 centos    8u     IPv4  31300      0t0      TCP ip-172-31-6-22.us-west-2.compute.internal:47480->204.79.197.219:https (ESTABLISHED)
mindthega 7766 centos    9w      REG  259,1 36306944 33595437 /home/centos/.image-bundle-4281772975/docker/registry/v2/repositories/oss/kubernetes-csi/csi-attacher/_uploads/c322a0b9-9af0-4e29-a0a9-9978d83cc0b5/data
mindthega 7766 centos   10u     IPv4  33709      0t0      TCP localhost:46524->localhost:36149 (ESTABLISHED)
mindthega 7766 centos   11u     IPv4  37962      0t0      TCP localhost:36149->localhost:46524 (ESTABLISHED)
mindthega 7766 centos   14u     IPv4  39026      0t0      TCP localhost:46508->localhost:36149 (ESTABLISHED)
mindthega 7766 centos   17u     IPv4  39027      0t0      TCP localhost:36149->localhost:46508 (ESTABLISHED)

@jimmidyson jimmidyson changed the title fix: open file limit with TLSRoundTripper fix: Separate out source and destination remote options Apr 24, 2023
@jimmidyson jimmidyson merged commit 94087d3 into main Apr 24, 2023
@jimmidyson jimmidyson deleted the dkoshkin/fix-http-trasport-leak branch April 24, 2023 14:03
@github-actions github-actions bot added fix and removed fix labels Apr 24, 2023
@github-actions github-actions bot mentioned this pull request Apr 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmidyson jimmidyson jimmidyson approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dkoshkin @jimmidyson