Backup: safe compressor/decompressor closure

[shlomi-noach]

Description

As it happens, 3rd party compressor/decompressor implementations can be unreliable and hang while Close()ing. The current codebase assumes Close() always returns in a timely fashion, and therefore the entire backup/restore process can hang while the compressor/decompressor hangs.

There's another issue where if the compressor/decompressor errors, the backup process error flow loses information, as the Close() error overrides any prior (and more important) error.

In this PR we do the following:

• Set a timeout for the compressor/decompressor Close() function (essentially wrapping up with a context.WithTimeout())
• Ensure to keep track of, and aggregate, all errors up to the Close() call.

Related Issue(s)

#13677

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
• If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

• Is it really necessary to add this flag?
• Flag names should be clear and intuitive (as far as possible)
• Help text should be descriptive.
• Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow should be required, the maintainer team should be notified.

Bug fixes

• There should be at least one unit or end-to-end test.
• The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

• There should be some code comments as to why things are implemented the way they are.

New/Existing features

• Should be documented, either by modifying the existing documentation or creating new documentation.
• New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• vtctl command output order should be stable and awk-able.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from VTop, if used there.

[GuptaManan100]

You can return the error directly, you don't have to assign to finallErr, that happens implicitly.

[shlomi-noach]

OMG TIL

[shlomi-noach]

OK fixed, basically reverting much into the original assignment logic.

[GuptaManan100]

Some comment as above and following such assignments and returns. Although I don't mind keeping it this way for readability.

[shlomi-noach]

fixed

[GuptaManan100]

Could we add a test to verify the close method returns after 3 seconds, if the decompressor gets stuck?

[shlomi-noach]

Added unit test for closeWithTimeout() function.

[maxenglander]

A question and suggestion, but LGTM

[maxenglander]

Is the reason for moving this just to be more consistent with where this code is handled in restoreFile. I am all for this change just wanted to understand the reasoning.

[shlomi-noach]

The reason for moving it is to be more golang idiomatic: we create the compressor in the if backupStorageCompress block, and so this is where we should create the deferred Close() function, rather than two blocks down the road.

[maxenglander]

Just an idea, we have a package called ioutil where we have some utils for customizing reader/writer behavior https://github.com/vitessio/vitess/tree/main/go/ioutil.

You could wrap up this logic in an ioutil helper like this:

package ioutil

type timeoutCloser struct {
    io.Closer
    ctx context.Context
    timeout time.Duration
}

func (tc *timeoutCloser) Close() {
  // The logic you have below.
}

[shlomi-noach]

OK, I did a refactor and created a TimeoutCloser, please take a look. The TimeoutCloser I created does not take a context, because the timeout is only relevant for the Close() function, and not for the general lifetime of the closer.

[maxenglander]

Looks good! One reason that you might want to pass an external context to timeoutCloser is in case the overall backup fails for some other reason, or one file times out long before another, then the shared external context can force the Close operations to terminate early.

For example let's say you have one file that we are trying to Close, and then another file starts to Close 5 seconds later. The first file Close times out, and ideally what you would like to do is force the other file to Close. Another reason is that if files are in the middle of closing, and the backend needs to cancel for some other reason, you can force the Close calls to exit early through propagation of context expires. E.g.

file1 := &timeoutCloser{writer, parentCtx, 10*time.Second)
file2 := &timeoutCloser{writer, parentCtx, 10*time.Second)

go file1.Close()
go file2.Close()

select {
case parentCtx.Done():
case <-ohNoSomethingWentWrong:
  cancelParentCtx() // Will force all fileN.Close() to cancel.
}

[deepthi]

It turns out that in the past, a similar problem was fixed somewhat differently for xtrabackup.
It is worth reading the commentary in #5193 around the change. Once that is done, we'll need to agree on a way to solve it uniformly for both builtin and xtrabackup.
The main points are:

• how long should the timeout be? 3 seconds or 10 minutes?
• is it safe to abandon the Close? xtrabackup: Add a timeout on closing backup files. #5193 says no, this PR is doing that.

A minor nit regarding go/vt/mysqlctl/builtinbackupengine2_test.go. The filename should be descriptive of what it is intended for. Is there a good reason these tests could not go into the existing test file?

[shlomi-noach]

Is there a good reason these tests could not go into the existing test file?

Yes. For some reason the existing test file uses a different package name, and so it's not in the same namespace and we cannot test private functions.

[shlomi-noach]

I'm looking into @deepthi's comments.

[shlomi-noach]

I tend to agree that Close() should not be abandoned, and that an error on Close() should be considered a backup/restore failure.

[shlomi-noach]

No opinion on the timeout duration. I don't have a good sense of how long it makes sense for an external/3rd party closer to Close(). A minute? 5 minutes? 10 minutes?

[shlomi-noach]

Retroactively created issue: #13677

[maxenglander]

Hey @shlomi-noach

The way that it is made non-blocking is through context.Context propagation:

1. context.Context is passed to AddFile. That context is supposed to be attached to the storage handle returned from AddFile. For example look at how this is handled in the gcsbackupstorage:

   vitess/go/vt/mysqlctl/gcsbackupstorage/gcs.go

   Lines 96 to 103 in 70a9466

   	// AddFile implements BackupHandle.
   	func (bh *GCSBackupHandle) AddFile(ctx context.Context, filename string, filesize int64) (io.WriteCloser, error) {
   	if bh.readOnly {
   	return nil, fmt.Errorf("AddFile cannot be called on read-only backup")
   	}
   	object := objName(bh.dir, bh.name, filename)
   	return bh.client.Bucket(bucket).Object(object).NewWriter(ctx), nil
   	}

2. Under the hood, the gcsbackupstorage is using this API, which has this important note:

   It is the caller's responsibility to call Close when writing is done. To stop writing without saving the data, cancel the context.

3. In other words, the GCS object Close() method is responsive to cancellation of context that was passed to the object during creation.
4. If cancelAddFiles() is called, closeFile will end early with an error because the GCS objects Close() early with a context cancelled error.

Note that this entire mechanism is dependent on backupstorage implementation correctly handling context cancellations, which not all backup storage implementations actually do (:cough: s3backupstorage).

[deepthi]

I think #5193 really solved a different thing, not what we're trying to solve in this PR. We're trying to solve the scenario where Close() never returns. I don't see #5193 did that.

I agree with this assessment. The only other thing we need to do is to implement the changes in this PR for xtrabackup as well, because similar code exists there.

https://github.com/vitessio/vitess/blob/main/go/vt/mysqlctl/xtrabackupengine.go#L443-L447
https://github.com/vitessio/vitess/blob/main/go/vt/mysqlctl/xtrabackupengine.go#L673-L679

[deepthi]

For some reason the existing test file uses a different package name, and so it's not in the same namespace and we cannot test private functions.

Ok, now that you said it, I looked at the test and it is in mysqlctl_test package. Introduced in
a918ff3

The <file>_test.go name should not have been used for this. My recommendation is to rename builtinbackupengine2_test.go to builtinbackupengine_test.go and builtinbackupengine_test.go to backup_blackbox_test.go or something similar.

[deepthi]

No opinion on the timeout duration. I don't have a good sense of how long it makes sense for an external/3rd party closer to Close(). A minute? 5 minutes? 10 minutes?

I don't know either. I'd say it's worth waiting a minute. 3 seconds is definitely too short. Anything that involves a network call should not timeout in less than 5 seconds. That's a rule of thumb based on TCP timeouts.

[mdlayher]

You don't want to embed the closer here, just use the unexported field.

[shlomi-noach]

fixed

[mdlayher]

The channel sender goroutine should be the only one to defer close(done) or else you may return on context cancel, close this, and then the goroutine may try to send on a closed channel and panic.

[shlomi-noach]

fixed

[mdlayher]

More generally you could make this accept a context rather than synthesizing one with a timeout, but that's a nit.

[shlomi-noach]

fixed

[deepthi]

I thought what @mdlayher meant was that you create the context with timeout outside the call to NewTimeoutCloser and assume that the passed in context already has a timeout. So instead of NewTimeoutCloser(closer, timeout), you have NewTimeoutCloser(ctx, closer).

[shlomi-noach]

The problem I see with that approach is that the timeout should really only apply to the Close() function, not to the overall lifespan of the closer object, which is created way before it's being Close()d.

[maxenglander]

I think the fact that this helper is wrapping the io.Closer interface makes it sort of clear that this only applies to the Close function. We could rename NewTimeoutCloser to something more descriptive, like ClosesWithTimeout.

[shlomi-noach]

I'm open to any change. To be fair, I don't think this functionality justifies a new type. The entire logic is in one function; maybe we should refactor this back into some ioutil.CloseWithTimeout() function. I don't mind either way and I don't have any opinions on naming.

[mdlayher]

Ah I see now. What I had intended was:

func NewTimeoutCloser(ctx context.Context, closer io.Closer, timeout time.Duration) *TimeoutCloser {

Shoudl be:

func NewContextCloser(ctx context.Context, closer io.Closer) *ContextCloser {

Which means you don't particularly care if there was a timeout or the user canceled it, the end result is the same. A context cancelation will unblock the operation, and your API can be entirely agnostic of the timeout duration value.

[shlomi-noach]

@mdlayher right -- so again the problem I see here, is that the obejct is created at some point in time, let's say 12:00. Then, some files are compressed/decompressed, saved/loaded. It's now 12:17. Now we want to Close() them. Now say we want a 1min timeout for the Close() function. But 17min have already passed. If we were to supply context into NewContextCloser(), then by the time we call Close() the context will already have expired 16min ago, giving us zero time to Close().

So the issue is we really have to start the counter/timer as we begin the call to Close(). Which is why the TimeoutCloser has to be aware of the timeout.

To top that, I again think we're better off with just a CloseWithTimeout() function, because this entire object grabs so much information upon construction, that it only uses in the Close() function.

[mdlayher]

SGTM, I'm good with your plan.

[shlomi-noach]

I'm thinking to now just keep it as it is. If anyone feels strongly about how to re-implement this, please feel absolutely free to modify, I don't really mind either way (other than we must ensure the timout only applies to the runtime of Close() as described above).

[mdlayher]

assert.ErrorIs(t, err, context.DeadlineExceeded)

[shlomi-noach]

fixed

[shlomi-noach]

My recommendation is to rename builtinbackupengine2_test.go to builtinbackupengine_test.go and builtinbackupengine_test.go to backup_blackbox_test.go or something similar.

Done.

[shlomi-noach]

@maxenglander thank you - and you are right.

[shlomi-noach]

The only other thing we need to do is to implement the changes in this PR for xtrabackup as well, because similar code exists there.

Done

[shlomi-noach]

Reduced closeTimeout to 1min. This applies both to XtrabackupEngine and BuiltinEngine.

[shlomi-noach]

This was renamed from go/vt/mysqlctl/builtinbackupengine_test.go

[shlomi-noach]

File is renamed to go/vt/mysqlctl/builtinbackupengine_test.go

[shlomi-noach]

I seem to have broken backup related tests. Looking into.

[deepthi]

This all looks mostly fine to me. The one thing that isn't very clear/ideal/idiomatic is covered by #13668 (comment)

Of course tests are telling us something, so we have to make them happy before merging.

[shlomi-noach]

Found the reason for the test failures: by deferring the Close() function, we inadvertently also changes the logic ordering: the code that was previously non-deferred ran first, now it runs last.

Specifically, this piece of code has to run after the decompressor closes, or else the decompressor receives an unexpected EOF:

	// Close the backupPipe to finish writing on destination.
	if err = bw.Close(); err != nil {
		return vterrors.Wrapf(err, "cannot flush destination: %v", name)
	}

	if err := br.Close(); err != nil {
		return vterrors.Wrap(err, "failed to close the source reader")
	}

To fix that I introduced an inner function createAndCopy inside backupFile. The deferred Close() now runs inside the inner function. Not the prettiest but it seems to to the job.

[shlomi-noach]

Going to merge this in the next couple hours. I think the core behavior changes are all agreed on, and what remaining discussions around the Closer implementation have converged to "no strong opinions" and we can always iterate on those in a follow up PR. If there's otherwise any outstanding concerns please comment.