Migrate the S3 SDK from v1 to v2 #16664

frouioui · 2024-08-27T22:29:31Z

Description

This PR updates the version of the AWS SDK from v1 to v2. The version 1 has been deprecated for some time now.

I have ran the following manual test:

Create a Vitess cluster (1 unsharded keyspace) from the main branch
Insert some data
Backup the cluster into AWS S3
Teardown the cluster
Create a Vitess cluster (same config) from the migrate-to-s3-v2 branch (with --restore_from_backup)
Restore from the previous backup
Observe that the inserted data shows up after restoring
Take another backup on AWS S3
List all available backup on AWS S3
Restore the cluster from the latest backup (using RestoreFromBackup)
Remove the first backup we did (the one from main) from AWS S3
Teardown the cluster
Create another Vitess cluster (same config) from the main branch (with --restore_from_backup)
Observe that the inserted data shows up after restoring

This test ensures that we can upgrade: old backups will still work with the new code, and that we can downgrade: new backups will work with the old code. While making sure that no config/flag change is needed between the two code version. It also checks that the GetBackups and RestoreFromBackup vtctldclient commands work.

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

vitess-bot · 2024-08-27T22:29:34Z

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

codecov · 2024-09-12T22:04:28Z

Codecov Report

Attention: Patch coverage is 40.50633% with 47 lines in your changes missing coverage. Please review.

Project coverage is 68.96%. Comparing base (538dd4c) to head (02598ab).
Report is 81 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/mysqlctl/s3backupstorage/s3.go	37.50%	40 Missing ⚠️
go/vt/mysqlctl/s3backupstorage/retryer.go	57.14%	6 Missing ⚠️
go/vt/mysqlctl/xtrabackupengine.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16664      +/-   ##
==========================================
- Coverage   68.98%   68.96%   -0.03%     
==========================================
  Files        1562     1565       +3     
  Lines      200690   201770    +1080     
==========================================
+ Hits       138449   139142     +693     
- Misses      62241    62628     +387

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

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

frouioui · 2024-09-12T23:14:24Z

go/vt/mysqlctl/s3backupstorage/s3.go

 			u.PartSize = partSizeBytes
 		})
 		object := objName(bh.dir, bh.name, filename)
 		sendStats := bh.bs.params.Stats.Scope(stats.Operation("AWS:Request:Send"))
 		// Using UploadWithContext breaks uploading to Minio and Ceph https://github.com/vitessio/vitess/issues/14188
-		_, err := uploader.Upload(&s3manager.UploadInput{
+		_, err := uploader.Upload(context.Background(), &s3.PutObjectInput{


Using context.Background() is a side-effect of the issue described in #14188. Upgrading to the SDK V2 does not fix it, the issue is rooted in how we handle the context and the goroutines. I started investigating the issue a bit deeper and will continue in a subsequent PR.

deepthi · 2024-09-12T23:29:33Z

go/vt/mysqlctl/s3backupstorage/retryer_test.go

-			r: &request.Request{
-				Retryable: aws.Bool(false),
-			},
+			name:           "no error",


Nice refactor.

deepthi · 2024-09-12T23:31:32Z

go/vt/mysqlctl/xtrabackupengine.go

@@ -277,7 +277,7 @@ func (be *XtrabackupEngine) executeFullBackup(ctx context.Context, params Backup
 	if err != nil {
 		return BackupUnusable, vterrors.Wrapf(err, "cannot JSON encode %v", backupManifestFileName)
 	}
-	if _, err := mwc.Write([]byte(data)); err != nil {
+	if _, err := mwc.Write(data); err != nil {


This code is going to be called regardless of s3 or other storage backend. Why this change?

data is a already a slice of bytes, I saw it while debugging something and thought I'd remove the useless conversion, I can revert if you want

marf · 2024-11-30T14:35:23Z

Hi, I have recently upgraded to Vitess v21 from v20. Now the backup fails with the following error:

+ /vt/bin/vtctldclient --action_timeout=2h0m0s --server esound-vtctld-d7707a8c.default.svc.cluster.local:15999 BackupShard esound/-
E1130 13:16:50.577523       7 main.go:56] rpc error: code = Unknown desc = TabletManager.Backup on uk-0828527281: StartBackup failed: operation error S3: HeadBucket, resolve auth scheme: resolve endpoint: endpoint rule error, Custom endpoint `` was not a valid URI

I haven't changed how the backup configuration works and the s3 credentials and configuration are correct, I copy it here:

spec:
  backup:
    locations:
      - s3:
          authSecret:
            key: credentials
            name: vitess-s3-credentials
          bucket: esound-vitess-backup
          region: eu-west-2

Any idea what this could be? Seems more an issue with this PR.

Thank you!

marf · 2024-11-30T14:59:14Z

I think I have found the issue.

Seems a bug with the fact that it does not check if the endpoint config is not specified and if not specified it uses an empty value instead of completely ignoring it and constructing the URL from the other parameters.

I managed to get it working by specifying this in the spec:

spec:
  backup:
    locations:
      - s3:
          authSecret:
            key: credentials
            name: vitess-s3-credentials
          bucket: esound-vitess-backup
          region: eu-west-2
          endpoint: https://s3.eu-west-2.amazonaws.com/ # fix for Vitess v21.0.0

frouioui · 2024-12-02T14:22:10Z

Hi @marf, thanks for raising this. I opened a direct Pull Request to release-21.0 to fix the issue, we should be releasing a new v21 patch tonight. #17307

marf · 2024-12-02T14:25:58Z

Thank you @frouioui.

As soon as released I will test it.

I want to raise also the fact that even in the case I specified a custom endpoint to make the backup working after a certain point during the finalization of the backup for a large table that we have called usertrack we get this error:

esound/- (uk-0828527281): time:{seconds:1733007057 nanoseconds:802234680} file:"builtinbackupengine.go" line:803 value:"backing up  \"vt_esound/userstracks.ibd\": 98.91% (417421408.00/422031360.00kb)"
esound/- (uk-0828527281): time:{seconds:1733007062 nanoseconds:802172059} file:"builtinbackupengine.go" line:803 value:"backing up  \"vt_esound/userstracks.ibd\": 99.38% (419396064.00/422031360.00kb)"
esound/- (uk-0828527281): time:{seconds:1733007067 nanoseconds:802024811} file:"builtinbackupengine.go" line:803 value:"backing up  \"vt_esound/userstracks.ibd\": 99.96% (421879616.00/422031360.00kb)"
esound/- (uk-0828527281): time:{seconds:1733007068 nanoseconds:95660900} file:"builtinbackupengine.go" line:902 value:"closing compressor"
esound/- (uk-0828527281): time:{seconds:1733007068 nanoseconds:97128328} file:"builtinbackupengine.go" line:795 value:"Completed backing up  \"vt_esound/userstracks.ibd\""
esound/- (uk-0828527281): time:{seconds:1733007081 nanoseconds:110832416} file:"builtinbackupengine.go" line:509 value:"resetting mysqld super_read_only to true"
esound/- (uk-0828527281): time:{seconds:1733007081 nanoseconds:111730683} file:"builtinbackupengine.go" line:526 value:"restarting mysql replication"
E1130 22:51:23.243289       7 main.go:56] rpc error: code = Unknown desc = TabletManager.Backup on uk-0828527281: upload multipart failed, upload id: PbdDGKgIlKtHUj0OYGkB.gI.g.TINRpUZtUZwHqOHkhthC9Uu04ScDsqiPDN5XMnvxkQW8T4d0Z01gpM2P26ebuHN73TVLYfa4pUI7I7Pg7bILDB2g13j0371Ga9KFKi, cause: operation error S3: UploadPart, https response error StatusCode: 0, RequestID: , HostID: , canceled, context canceled

I then downgraded to vitess v20.0.3 and this issue seems not to happen with this version. Not sure if it is something related with the upgrade of the S3 SDK.

frouioui · 2024-12-02T14:28:29Z

This is fixed by #17285 and is already included in the upcoming 21.0.1 release.

marf · 2024-12-02T14:35:54Z

Awesome, thank you @frouioui.

frouioui · 2024-12-02T14:46:30Z

As soon as released I will test it.

Thanks for that! If you see any issue, let me know :)

github-actions bot added this to the v21.0.0 milestone Aug 27, 2024

frouioui force-pushed the migrate-to-s3-v2 branch 2 times, most recently from bd75773 to 9edbe34 Compare September 5, 2024 16:04

frouioui added 2 commits September 12, 2024 15:45

Migrate from AWS SDK v1 to v2

5b6ba75

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

Merge remote-tracking branch 'origin/main' into migrate-to-s3-v2

dc21803

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

frouioui force-pushed the migrate-to-s3-v2 branch from 9edbe34 to dc21803 Compare September 12, 2024 21:46

Small fix

02598ab

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

frouioui commented Sep 12, 2024

View reviewed changes

frouioui marked this pull request as ready for review September 12, 2024 23:15

frouioui requested review from deepthi, mattlord, ajm188, harshit-gangal, rohit-nayak-ps and systay as code owners September 12, 2024 23:15

deepthi removed request for systay, ajm188 and harshit-gangal September 12, 2024 23:27

deepthi removed the request for review from rohit-nayak-ps September 12, 2024 23:27

deepthi reviewed Sep 12, 2024

View reviewed changes

deepthi approved these changes Sep 17, 2024

View reviewed changes

mattlord approved these changes Sep 17, 2024

View reviewed changes

frouioui merged commit 3182049 into main Sep 17, 2024
189 checks passed

frouioui deleted the migrate-to-s3-v2 branch September 17, 2024 23:28

deepthi mentioned this pull request Sep 19, 2024

Fix xtrabackup/builtin context when doing AddFiles #16806

Merged

5 tasks

frouioui mentioned this pull request Oct 17, 2024

Switch S3 backups to aws-sdk-go-v2 #14712

Closed

frouioui mentioned this pull request Dec 2, 2024

[release-21.0] S3: optional endpoint resolver and correct retrier #17307

Merged

frouioui mentioned this pull request Dec 2, 2024

Implement a per-file retry mechanism in the BuiltinBackupEngine #17271

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the S3 SDK from v1 to v2 #16664

Migrate the S3 SDK from v1 to v2 #16664

frouioui commented Aug 27, 2024 •

edited

Loading

vitess-bot bot commented Aug 27, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

frouioui Sep 12, 2024

deepthi Sep 12, 2024

deepthi Sep 12, 2024

frouioui Sep 12, 2024 •

edited

Loading

marf commented Nov 30, 2024

marf commented Nov 30, 2024

frouioui commented Dec 2, 2024

marf commented Dec 2, 2024

frouioui commented Dec 2, 2024

marf commented Dec 2, 2024

frouioui commented Dec 2, 2024

Migrate the S3 SDK from v1 to v2 #16664

Migrate the S3 SDK from v1 to v2 #16664

Conversation

frouioui commented Aug 27, 2024 • edited Loading

Description

Checklist

vitess-bot bot commented Aug 27, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

frouioui Sep 12, 2024

Choose a reason for hiding this comment

deepthi Sep 12, 2024

Choose a reason for hiding this comment

deepthi Sep 12, 2024

Choose a reason for hiding this comment

frouioui Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

marf commented Nov 30, 2024

marf commented Nov 30, 2024

frouioui commented Dec 2, 2024

marf commented Dec 2, 2024

frouioui commented Dec 2, 2024

marf commented Dec 2, 2024

frouioui commented Dec 2, 2024

frouioui commented Aug 27, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

frouioui Sep 12, 2024 •

edited

Loading