prevent vtctld from creating tons of S3 connections #15296

rvrangel · 2024-02-20T13:19:50Z

Description

This fixes the issue described in #15335. It seems the bug was introduced in #4200.

The default transport was changed in favour of one were we could skip validating S3 certificates, but instead of cloning the default transport and adjusting the settings, we are creating a new transport from scratch but not setting any defaults (see current Go defaults for DefaultTransport). By doing so, we don't set keepalives, timeouts, max idle connections, etc and other values in Transport and since some default to zero it basically means "no limit". To make matters worse, since we are creating a new transport for every client (in this case, for each request from what I understand), there is no sharing of connections between different requests, so the first call to GetBackups() will open some S3 connections and keep them on its private transport instead of sharing it across different requests.

After this fix, the number of established connections seems to be consistently below 5, and from initial testing it seems S3 connection errors seem to be gone.

Related Issue(s)

fixes Bug Report: vtctlds can create an excessive amount of connections when receiving concurrent GetBackups() calls #15335

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Renan Rangel <[email protected]>

vitess-bot · 2024-02-20T13:19:53Z

Signed-off-by: Renan Rangel <[email protected]>

codecov · 2024-02-20T13:39:43Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.44%. Comparing base (696fe0e) to head (13a3911).
Report is 68 commits behind head on main.

Files	Patch %	Lines
go/vt/mysqlctl/s3backupstorage/s3.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15296      +/-   ##
==========================================
- Coverage   67.41%   65.44%   -1.98%     
==========================================
  Files        1560     1562       +2     
  Lines      192752   193914    +1162     
==========================================
- Hits       129952   126906    -3046     
- Misses      62800    67008    +4208

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Renan Rangel <[email protected]>

dbussink · 2024-02-21T19:33:19Z

go/vt/mysqlctl/s3backupstorage/s3.go

+		defaultS3Transport.TLSClientConfig = tlsClientConf
+	}
+
+	return defaultS3Transport


Is this concurrency safe / does it need to be? Since it doesn't seem like it is.

I'm also not a big fan of having a global for this, those tend to bite us later on. Can we move it at least into S3BackupStorage instead then? That's still kinda global, but at least a tad better then.

We can then also initialize it already in the init() to avoid the whole concurrency question / potential problem?

yes, it should be, specially considering the default on the SDK already uses http.DefaultClient, which in turn uses http.DefaultTransport, this would have been a shared default in most projects that don't define their transport/client from scratch, see the Config docs.

I thought the S3BackupStorage would be created for each request being processed, but if that is not the case we could move it there. I will test it out and report back.

if the above is not possible and setting it up on init() is preferred I can do that too

I see that you added a unit test for this. What's the conclusion?
@dbussink time for a re-review?

@deepthi I’m back next week but I don’t see a refactor of this or a reason why that couldn’t be done. I strongly prefer not having a global like this if that can reasonably be avoided.

Also happy to push up that refactor once I’m back.

I have moved this to inside the S3BackupStorage and re-run our tests and seems the connections are getting re-used, so I have update the PR to reflect that, thanks for pointing it out!

deepthi

Good one.
Can you please take the description and split out the problem part of it into a new issue? How it is being fixed can be kept in the PR description.
We may want to backport this too.

rvrangel · 2024-02-22T15:26:54Z

thanks @deepthi , I created #15335 and mentioned it here in the PR description

mattlord

Thanks, @rvrangel ! I only had a few small nits/suggestions. Please let me know what you think. I can come back quickly and review again and help to get it merged.

mattlord · 2024-02-28T04:00:01Z

go/vt/mysqlctl/s3backupstorage/s3.go

+	if defaultS3Transport == nil {
+		tlsClientConf := &tls.Config{InsecureSkipVerify: tlsSkipVerifyCert}
+
+		defaultS3Transport = http.DefaultTransport.(*http.Transport).Clone()


This seems correct after reading through: golang/go#26013

I think that we should get rid of this getter and instead initialize the variable in init() though. This is a more clear signal of our intent and a more standard pattern. Does that make sense?

yes, that makes sense. I will move it to the init() function. I will updated the PR tomorrow

as mentioned in the other comment, I have moved this to the S3BackupStorage struct, so we don't need to initialise a global anymore 🙌

mattlord · 2024-02-28T04:08:03Z

go/vt/mysqlctl/s3backupstorage/s3_test.go

@@ -276,3 +276,16 @@ func TestSSECustomerFileBase64Key(t *testing.T) {
 	assert.Nil(t, sseData.customerKey, "customerKey expected to be nil")
 	assert.Nil(t, sseData.customerMd5, "customerMd5 expected to be nil")
 }
+
+func Test_getS3Transport(t *testing.T) {


Nit, but TestGetS3Transport would be the typical name in the code base.

I updated it to TestGetTransport to avoid the redundancy

mattlord · 2024-02-28T04:34:21Z

go/vt/mysqlctl/s3backupstorage/s3.go

@@ -83,6 +83,9 @@ var (

 	// path component delimiter
 	delimiter = "/"
+
+	// use a shared transport for all connections
+	defaultS3Transport *http.Transport


Nit, but s3 is a bit redundant here. We can just call it defaultTransport.

got rid of the global variable 😌

Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>

dbussink · 2024-02-29T16:38:45Z

go/vt/mysqlctl/s3backupstorage/s3.go

+// This creates a new transport based off http.DefaultTransport the first time and returns the same
+// transport on subsequent calls so connections can be reused as part of the same transport.
+func (bs *S3BackupStorage) getTransport() *http.Transport {
+	if bs.transport == nil {


Same question as before though, does this get run concurrently?

I think we can create the transport whenever we create the struct instead to avoid that?

it is possible, considering that we will have multiple checks on different backups can happen at the same time. perhaps adding a mutex here would be better so it is always safe, regardless if it is set when we create the struct or when we first call this function, what do you think?

actually, this is only called on S3BackupStorage.client() which acquires a mutex when it is running, so we shouldn't see this running concurrently

vitess/go/vt/mysqlctl/s3backupstorage/s3.go

Lines 457 to 463 in eff2a92

func (bs *S3BackupStorage) client() (*s3.S3, error) {

bs.mu.Lock()

defer bs.mu.Unlock()

if bs._client == nil {

logLevel := getLogLevel()

httpClient := &http.Client{Transport: bs.getTransport()}

@rvrangel i think we can then also move the initialization there too? No need to wrap it in another lazy loading function?

Sorry, that’s wrong. I mean where bs is initialized.

yeah, we can do that if you prefer - originally I didn't to make it easier to have a unit test around this (and the existing client() function doesn't have a test, looks like it makes a call to S3 to validate it) and since we are not using a contructor function like New() to create this struct - everything gets done in this function - it is harder to mock the S3 client that makes an actual HeadBucket() call.

so my question is, would you be okay with moving this into client() without a unit test, or do you want to make this into a bigger PR where we can mock that call and change how S3BackupStorage gets created (or all structs that implement BackupStorage in this case)?

there is also the consideration that we might have a single S3BackupStorage but we might call S3BackupStorage.client() for each request - which then will use a different transport for each client we initialise, while the current approach uses the same transport for all clients generated by the same S3BackupStorage

I think a small refactor with a (private) constructor and then testing transport is fine?

I don’t see how we create different transports though? We can then set directly bs.transport?

taking a better look at the code, we do that initialisation under init() so it should be easy enough to update it with a new() method instead of the current line, so I will update it tomorrow:

vitess/go/vt/mysqlctl/s3backupstorage/s3.go

Lines 512 to 513 in eff2a92

func init() {

backupstorage.BackupStorageMap["s3"] = &S3BackupStorage{params: backupstorage.NoParams()}

but if we set bs.transport directly, the problem is the functions in this struct were designed in a way to call bs.client() every time, like here:

vitess/go/vt/mysqlctl/s3backupstorage/s3.go

Lines 292 to 294 in eff2a92

func (bs *S3BackupStorage) ListBackups(ctx context.Context, dir string) ([]backupstorage.BackupHandle, error) {

log.Infof("ListBackups: [s3] dir: %v, bucket: %v", dir, bucket)

c, err := bs.client()

as an example take S3BackupStorage.ListBackups(), every time we call it, it will in turn call S3BackupStorage.client() which will set bs.transport = again, making it a different transport each time. or unless you meant to lazy load it inside the client() function as we are doing in the current chage?

@dbussink I have moved this to a private constructor as we talked, and instead of setting bs.transport inside client() it is now only set on the constructor - and I have removed the lazy loading, but that will cause issues if there is any place where we use S3StorageBackup{} without initialising the transport, but it doesn't seem like we do?

let me know if this looks good to you or if you want to add extra safety on the client() function

Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>

dbussink

I like how this is now much simpler imho.

mattlord

Nice work, @rvrangel ! Thank you!

Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>

deepthi · 2024-03-04T19:33:05Z

This is a really great contribution @rvrangel! Thank you.

…15296) (#15401) Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

prevent vtctld from creating tons of S3 connections

dcdb7cf

Signed-off-by: Renan Rangel <[email protected]>

github-actions bot added this to the v20.0.0 milestone Feb 20, 2024

add unit test

f4976ed

Signed-off-by: Renan Rangel <[email protected]>

rvrangel added the Component: General Changes throughout the code base label Feb 21, 2024

goimports

d1a031e

Signed-off-by: Renan Rangel <[email protected]>

rvrangel added the Type: Performance label Feb 21, 2024

rvrangel marked this pull request as ready for review February 21, 2024 13:08

rvrangel requested review from deepthi and mattlord as code owners February 21, 2024 13:08

dbussink reviewed Feb 21, 2024

View reviewed changes

deepthi reviewed Feb 22, 2024

View reviewed changes

deepthi added Backport to: release-19.0 Needs to be back ported to release-19.0 and removed NeedsWebsiteDocsUpdate What it says NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Feb 22, 2024

systay mentioned this pull request Feb 26, 2024

Release of v19.0.0 #15357

Closed

38 tasks

frouioui removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request labels Feb 26, 2024

mattlord reviewed Feb 28, 2024

View reviewed changes

move function to struct

eff2a92

Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>

dbussink reviewed Feb 29, 2024

View reviewed changes

move to private contructor

13a3911

Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>

dbussink approved these changes Mar 1, 2024

View reviewed changes

mattlord self-requested a review March 4, 2024 19:25

mattlord approved these changes Mar 4, 2024

View reviewed changes

mattlord merged commit 259d963 into vitessio:main Mar 4, 2024
102 checks passed

vitess-bot pushed a commit that referenced this pull request Mar 4, 2024

prevent vtctld from creating tons of S3 connections (#15296)

2c59593

Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>

vitess-bot bot mentioned this pull request Mar 4, 2024

[release-19.0] prevent vtctld from creating tons of S3 connections (#15296) #15401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent vtctld from creating tons of S3 connections #15296

prevent vtctld from creating tons of S3 connections #15296

rvrangel commented Feb 20, 2024 •

edited by mattlord

Loading

vitess-bot bot commented Feb 20, 2024 •

edited by mattlord

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

dbussink Feb 21, 2024

dbussink Feb 21, 2024

rvrangel Feb 22, 2024

deepthi Feb 27, 2024

dbussink Feb 28, 2024

rvrangel Feb 29, 2024

deepthi left a comment

rvrangel commented Feb 22, 2024

mattlord left a comment

mattlord Feb 28, 2024

mattlord Feb 28, 2024

rvrangel Feb 28, 2024

rvrangel Feb 29, 2024

mattlord Feb 28, 2024

rvrangel Feb 29, 2024

mattlord Feb 28, 2024 •

edited

Loading

rvrangel Feb 29, 2024

dbussink Feb 29, 2024

rvrangel Feb 29, 2024

rvrangel Feb 29, 2024

dbussink Feb 29, 2024

dbussink Feb 29, 2024

rvrangel Feb 29, 2024 •

edited

Loading

rvrangel Feb 29, 2024

dbussink Feb 29, 2024

rvrangel Feb 29, 2024

rvrangel Mar 1, 2024

dbussink left a comment

mattlord left a comment

deepthi commented Mar 4, 2024

	func (bs S3BackupStorage) client() (s3.S3, error) {
	bs.mu.Lock()
	defer bs.mu.Unlock()
	if bs._client == nil {
	logLevel := getLogLevel()

	httpClient := &http.Client{Transport: bs.getTransport()}

	func init() {
	backupstorage.BackupStorageMap["s3"] = &S3BackupStorage{params: backupstorage.NoParams()}

	func (bs *S3BackupStorage) ListBackups(ctx context.Context, dir string) ([]backupstorage.BackupHandle, error) {
	log.Infof("ListBackups: [s3] dir: %v, bucket: %v", dir, bucket)
	c, err := bs.client()

prevent vtctld from creating tons of S3 connections #15296

prevent vtctld from creating tons of S3 connections #15296

Conversation

rvrangel commented Feb 20, 2024 • edited by mattlord Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Feb 20, 2024 • edited by mattlord Loading

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

rvrangel commented Feb 22, 2024

mattlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlord Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvrangel Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbussink left a comment

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

deepthi commented Mar 4, 2024

rvrangel commented Feb 20, 2024 •

edited by mattlord

Loading

vitess-bot bot commented Feb 20, 2024 •

edited by mattlord

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

mattlord Feb 28, 2024 •

edited

Loading

rvrangel Feb 29, 2024 •

edited

Loading