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

Fix how we cancel the context in the builtin backup engine #17285

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Fix how we cancel the context in the builtin backup engine #17285

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d53b99
Fix how we cancel the context when taking a builtin backup
frouioui Nov 26, 2024
4cc10e5
Fix how we cancel the context when restoring
frouioui Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions go/vt/mysqlctl/builtinbackupengine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,15 @@ func (be *BuiltinBackupEngine) backupFiles(
wg := sync.WaitGroup{}

ctxCancel, cancel := context.WithCancel(ctx)
defer cancel()
defer func() {
// We may still have operations in flight that require a valid context, such as adding files to S3.
// Unless we encountered an error, we should not cancel the context, this is taken care of later
// in the process. If we encountered an error however, we can safely cancel the context as we should
// no longer work on anything and exit fast.
if finalErr != nil {
cancel()
}
}()
Copy link
Contributor

@shlomi-noach shlomi-noach Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I follow:

  • It looks as if the function can exit and still leave the context active, in which case, what is cancelling it?
  • If we still have operations in flight, why would we exit the function? Should we not wait until everything is complete?

Something feels off here, as an anti-pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

The S3 and Ceph uploads may be incomplete by the time we return from this function. Writing to the buffers used by these storage implementation will be complete (which is how we're able to return from be.backupFile and be.backupFiles), but the actual reading from the buffer and uploading to the remote storage will/may not be complete. If we cancel the context too early, unfinished uploads will be canceled and marked as failed.

It is only at a later stage where we wait for all the uploads to be finished with the EndBackup method on the backup handle:

vitess/go/vt/mysqlctl/backup.go

Lines 190 to 191 in 16fa7a3

case BackupUsable:
finishErr = bh.EndBackup(ctx)

At this stage we will observe the failures created by the S3 or Ceph storage implementation and will decide to fail - even though we have already uploaded the backup (including the MANIFEST) and restarted MySQL.

There is definitely something off with this code. We must wait for the full backup (writing to the backend storage included) to complete before going forward with writing the MANIFEST and assuming the backup is useable. This is something that I am implementing along with a retry mechanism on: #17271.

Copy link
Member Author

Choose a reason for hiding this comment

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

what is cancelling it?

The caller of ExecuteBackup will eventually cancel it, whether it is through a gRPC call or through vtbackup, the context always gets canceled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarity! ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

@frouioui How can it ever cancel it? ctx might be cancelled, but ctxCancel is not? That leads to a memory leak.

So even if the outer context is cancelled, we still need to cancel this inner one to avoid the memory leak.


for i := range fes {
wg.Add(1)
Expand Down Expand Up @@ -1037,7 +1045,15 @@ func (be *BuiltinBackupEngine) restoreFiles(ctx context.Context, params RestoreP
wg := sync.WaitGroup{}

ctxCancel, cancel := context.WithCancel(ctx)
defer cancel()
defer func() {
// We may still have operations in flight that require a valid context, such as adding files to S3.
// Unless we encountered an error, we should not cancel the context. This is taken care of later
// in the process. If we encountered an error however, we can safely cancel the context as we should
// no longer work on anything and exit fast.
if err != nil {
cancel()
}
}()

for i := range fes {
wg.Add(1)
Expand Down
Loading