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

Load tls certificate and x509 cert pool once per file to reduce memory usage #5423

Conversation

lyonlai
Copy link
Collaborator

@lyonlai lyonlai commented Nov 11, 2019

At the moment when starting connections to mysql database it's creating multiple copies of tls certificates for the same pem file loaded, which is causing memory pressure when loading a big combo certificate (example the combo certificate offered by AWS RDS).

This PR address the issue by creating only one copy of loaded certificate per file and refer to it, which brings down the memory usage for tls connections. The image below shows the after(left) vs before(right) comparison via golang's pprof's web interface.

image

@lyonlai lyonlai requested a review from sougou as a code owner November 11, 2019 00:57
@lyonlai lyonlai force-pushed the ylai/2019-11-11/one-copy-of-tls-certificate-per-file branch from e2352a4 to dcfd276 Compare November 11, 2019 01:12
@sougou sougou requested a review from dkhenry November 11, 2019 02:02
Copy link
Collaborator

@systay systay left a comment

Choose a reason for hiding this comment

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

:nit: since you are touching most of the file, you might as well move to vterror.Error instead of fmt.Error

@lyonlai lyonlai force-pushed the ylai/2019-11-11/one-copy-of-tls-certificate-per-file branch from dcfd276 to 3b78aef Compare November 11, 2019 11:28
go/vt/vttls/vttls.go Outdated Show resolved Hide resolved
@lyonlai lyonlai force-pushed the ylai/2019-11-11/one-copy-of-tls-certificate-per-file branch from 3b78aef to ee81517 Compare November 12, 2019 01:29
@lyonlai lyonlai requested a review from dkhenry November 12, 2019 01:32
usage

compare to one copy per mysql connect previously

Signed-off-by: Yun Lai <ylai@squareup.com>
@lyonlai lyonlai force-pushed the ylai/2019-11-11/one-copy-of-tls-certificate-per-file branch from ee81517 to 6888606 Compare November 12, 2019 02:27
@lyonlai
Copy link
Collaborator Author

lyonlai commented Nov 19, 2019

@dkhenry Is there any other changes besides the sync.Map you mentioned? Can you please take a look at the change I've pushed up?

@dkhenry
Copy link
Contributor

dkhenry commented Nov 20, 2019

No changes, just delayed in getting back to this

@dkhenry dkhenry merged commit 83bfb04 into vitessio:master Nov 20, 2019
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.

3 participants