Fix `xtrabackup`/`builtin` context when doing `AddFiles` #16806

frouioui · 2024-09-18T23:42:13Z

Description

This PR fixes a long standing issue in the xtrabackup engine: the context used to AddFiles by the backup storage was canceled before AddFiles had the chance to complete. This problem can be experienced with the S3 backup storage, as it uploads the file through a goroutine, Ceph also does that and have been impacted in the past too. That goroutine wouldn't have time to complete before the XtrabackupEngine.backupFiles function returned and canceled the context through a defer statement. Leading to AddFiles failing since the context we pass got canceled.

To avoid this issue, we never really passed the context down to AddFiles. We attempted to pass the context in #12500 but reverted it in #14311 since #14188 was raised.

This PR fixes that by not canceling the context automatically, but only when the backupFiles return an error. If we have an error anywhere while executing backupFiles we should attempt to cancel the addition of the files, otherwise we can assume there is nothing to cancel. A different goroutine already make sure to cancel the context if we reach a certain timeout when closing the files.

cc @L3o-pold @vczyh as the original authors of the two issues. I have tested this on my end with manual tests (only on S3), but do you mind making sure it fixes your issues?

Related Issue(s)

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

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

vitess-bot · 2024-09-18T23:42:15Z

codecov · 2024-09-19T00:00:46Z

Codecov Report

Attention: Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 69.49%. Comparing base (56c39b2) to head (1102c59).
Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/mysqlctl/s3backupstorage/s3.go	12.50%	7 Missing ⚠️
go/vt/mysqlctl/xtrabackupengine.go	0.00%	3 Missing ⚠️
go/vt/mysqlctl/builtinbackupengine.go	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16806      +/-   ##
==========================================
- Coverage   69.53%   69.49%   -0.04%     
==========================================
  Files        1567     1569       +2     
  Lines      202388   202644     +256     
==========================================
+ Hits       140723   140826     +103     
- Misses      61665    61818     +153

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

L3o-pold · 2024-09-19T12:14:53Z

@frouioui with your PR I can no longer listing backup for example:

/vt/bin/vtctldclient --server=vtctld:15999 --logtostderr=true GetBackups unsharded/-
E0919 12:12:39.588828      27 main.go:56] rpc error: code = Unknown desc = operation error S3: HeadBucket, https response error StatusCode: 301, RequestID: 2ACET95HY572K3PX, HostID: X2nVlauIKbd8pM8PqINAZCTtrmJ7LmHkCHVrCZq9tTbR3cLb8s4AnS/SnrELiZzroPbO69weCZ4=, api error MovedPermanently: Moved Permanently

frouioui · 2024-09-19T13:12:39Z

@frouioui with your PR I can no longer listing backup for example:

/vt/bin/vtctldclient --server=vtctld:15999 --logtostderr=true GetBackups unsharded/-

E0919 12:12:39.588828      27 main.go:56] rpc error: code = Unknown desc = operation error S3: HeadBucket, https response error StatusCode: 301, RequestID: 2ACET95HY572K3PX, HostID: X2nVlauIKbd8pM8PqINAZCTtrmJ7LmHkCHVrCZq9tTbR3cLb8s4AnS/SnrELiZzroPbO69weCZ4=, api error MovedPermanently: Moved Permanently

Interesting, may I know what flags (redacted) you are using? We recently changed the SDK version, that might be the cause.

L3o-pold · 2024-09-19T13:16:41Z

@frouioui --backup_engine_implementation=builtin --s3_backup_aws_region=us-east-1 --s3_backup_storage_bucket=vitess --s3_backup_force_path_style=true --s3_backup_aws_endpoint=http://minio:9000

deepthi · 2024-09-19T22:49:39Z

@L3o-pold could you test GetBackups with #16664? That way we know whether the SDK upgrade is already breaking you without the changes in this PR.

frouioui · 2024-09-19T22:53:53Z

@L3o-pold could you test GetBackups with #16664? That way we know whether the SDK upgrade is already breaking you without the changes in this PR.

I tested and it is a problem introduced by #16664, it seems to be an issue with Minio and the new connector. I am still investigating

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

frouioui · 2024-09-20T21:51:20Z

@L3o-pold this should be fixed now, at least on my end with Minio I am able to do all kind of backup operations.

L3o-pold · 2024-09-23T09:00:06Z

@frouioui it works great congrats and thanks <3

deepthi

💯

deepthi · 2024-09-23T19:22:53Z

go/vt/mysqlctl/s3backupstorage/s3.go

@@ -47,6 +47,8 @@ import (
 	"github.com/aws/smithy-go/middleware"
 	"github.com/spf13/pflag"

+	smithyendpoints "github.com/aws/smithy-go/endpoints"


nit: I don't see why this needs an override.

Fixed via 1102c59, we still need an override though using simply "endpoints", the IDE and compiler was not happy. Not exactly sure why, but in the migration docs, they also use override, and the package is named "transport" (even if the path is endpoint), so i ended up overriding it by "transport".

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

mattlord · 2024-09-25T01:32:51Z

go/vt/mysqlctl/s3backupstorage/s3.go

+func newEndpointResolver() *endpointResolver {
+	return &endpointResolver{
+		r:        s3.NewDefaultEndpointResolverV2(),
+		endpoint: &endpoint,


Would be nice IMO to have a comment about what this is here:

Suggested change

endpoint: &endpoint,

endpoint: &endpoint, // Use default endpoint of amazonaws.com

this is already the case in the variable definition:

// AWS endpoint, defaults to amazonaws.com but appliances may use a different location endpoint string

Use a context in S3's AddFiles

ff0f5c8

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

frouioui requested review from deepthi and mattlord as code owners September 18, 2024 23:42

frouioui requested a review from L3o-pold September 18, 2024 23:42

github-actions bot added this to the v21.0.0 milestone Sep 18, 2024

frouioui added Type: Bug Component: Backup and Restore labels Sep 18, 2024

frouioui changed the title ~~Use a context in S3's AddFiles~~ Fix xtrabackup context when doing AddFiles Sep 18, 2024

This was referenced Sep 18, 2024

Failed to backup to ceph(s3) using xtrabackup #8391

Closed

Bug Report: v18.0.0-rc1 S3 backup failed to upload #14188

Closed

Fix ctx in builtin and fix endpoint resolver

3f591d9

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

frouioui changed the title ~~Fix xtrabackup context when doing AddFiles~~ Fix xtrabackup/builtin context when doing AddFiles Sep 20, 2024

deepthi approved these changes Sep 23, 2024

View reviewed changes

deepthi mentioned this pull request Sep 23, 2024

Bug Report: Race condition on uploading backup and manifest #16825

Open

review suggestion

1102c59

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

mattlord approved these changes Sep 25, 2024

View reviewed changes

frouioui merged commit 766bc64 into vitessio:main Sep 27, 2024
97 checks passed

frouioui deleted the fix-backup-ctx branch September 27, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `xtrabackup`/`builtin` context when doing `AddFiles` #16806

Fix `xtrabackup`/`builtin` context when doing `AddFiles` #16806

frouioui commented Sep 18, 2024 •

edited

Loading

vitess-bot bot commented Sep 18, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

L3o-pold commented Sep 19, 2024

frouioui commented Sep 19, 2024

L3o-pold commented Sep 19, 2024

deepthi commented Sep 19, 2024

frouioui commented Sep 19, 2024

frouioui commented Sep 20, 2024

L3o-pold commented Sep 23, 2024 •

edited

Loading

deepthi left a comment

deepthi Sep 23, 2024

frouioui Sep 24, 2024

mattlord Sep 25, 2024

frouioui Sep 27, 2024

	endpoint: &endpoint,
	endpoint: &endpoint, // Use default endpoint of amazonaws.com

Fix xtrabackup/builtin context when doing AddFiles #16806

Fix xtrabackup/builtin context when doing AddFiles #16806

Conversation

frouioui commented Sep 18, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Sep 18, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

L3o-pold commented Sep 19, 2024

frouioui commented Sep 19, 2024

L3o-pold commented Sep 19, 2024

deepthi commented Sep 19, 2024

frouioui commented Sep 19, 2024

frouioui commented Sep 20, 2024

L3o-pold commented Sep 23, 2024 • edited Loading

deepthi left a comment

Choose a reason for hiding this comment

deepthi Sep 23, 2024

Choose a reason for hiding this comment

frouioui Sep 24, 2024

Choose a reason for hiding this comment

mattlord Sep 25, 2024

Choose a reason for hiding this comment

frouioui Sep 27, 2024

Choose a reason for hiding this comment

Fix `xtrabackup`/`builtin` context when doing `AddFiles` #16806

Fix `xtrabackup`/`builtin` context when doing `AddFiles` #16806

frouioui commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

L3o-pold commented Sep 23, 2024 •

edited

Loading