From 339c8edda93feb3e7d81e96beb4ffe12ccd533b5 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 19 Aug 2026 03:03:33 -0400 Subject: [PATCH] Detect block uploader cancellation through wrapped errors (#10308) * Detect block uploader cancellation through wrapped errors Cancelling a block data mover backup was reported as a failure: the DataUpload ended Failed with an error message and the Backup went PartiallyFailed, for a user-requested cancel. The cause is a sentinel equality check. block.ErrCanceled is raised in the write loop and then wrapped twice before it reaches the provider -- once in block/uploader.go ("error backing up bdev %s") and again in block/snapshot.go ("Failed to run uploader backup for si %v") -- so `err == block.ErrCanceled` can never be true and the ErrorCanceled returns are unreachable. The filesystem provider avoids this by asking the uploader for its state (kpUploader.IsCanceled()) rather than inspecting the error. Use errors.Is at both the backup and restore sites. Adds TestBlockProviderCancelThroughWrappedError, which injects the doubly-wrapped sentinel exactly as production builds it. Note the assertion is require.ErrorIs, not ErrorContains: provider.ErrorCanceled and block.ErrCanceled carry identical message text, so a substring assertion passes whether or not the sentinel was recognised -- which is why the existing test, injecting the bare sentinel, did not catch this. Co-Authored-By: Claude Fable 5 Signed-off-by: Tiger Kaovilai (cherry picked from commit 9d6c5da7a893068d424b0c7896638787c636e213) Signed-off-by: Tiger Kaovilai * Add changelog for #10308 Signed-off-by: Tiger Kaovilai * lint: fix misspelling (recognised -> recognized) Signed-off-by: Tiger Kaovilai --------- Signed-off-by: Tiger Kaovilai Co-authored-by: Claude Fable 5 --- changelogs/unreleased/10308-kaovilai | 1 + pkg/uploader/provider/block.go | 9 ++++- pkg/uploader/provider/block_test.go | 57 ++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/10308-kaovilai diff --git a/changelogs/unreleased/10308-kaovilai b/changelogs/unreleased/10308-kaovilai new file mode 100644 index 000000000..ef832d521 --- /dev/null +++ b/changelogs/unreleased/10308-kaovilai @@ -0,0 +1 @@ +Fix block data mover cancellation being reported as a backup failure diff --git a/pkg/uploader/provider/block.go b/pkg/uploader/provider/block.go index 6a7ae2802..e37a0c16f 100644 --- a/pkg/uploader/provider/block.go +++ b/pkg/uploader/provider/block.go @@ -134,7 +134,11 @@ func (bp *blockProvider) RunBackup( snapshotInfo, _, err := blockBackupFunc(ctx, blkUploader, bp.bkRepo, path, realSource, cbtParam.Source, forceFull, parentSnapshot, cbtParam.Service, uploaderCfg, tags, log) - if err == block.ErrCanceled { + // errors.Is, not ==: the sentinel is wrapped twice on its way here, by + // block/uploader.go ("error backing up bdev %s") and again by + // block/snapshot.go ("Failed to run uploader backup for si %v"), so an + // equality check never matches and cancellation gets reported as a failure. + if errors.Is(err, block.ErrCanceled) { log.Warn("Block backup is canceled") return snapshotInfo.ID, false, snapshotInfo.Size, snapshotInfo.IncrementalSize, ErrorCanceled } @@ -176,7 +180,8 @@ func (bp *blockProvider) RunRestore( size, err := blockRestoreFunc(ctx, blkUploader, bp.bkRepo, snapshotID, volumePath, uploaderCfg, log) - if err == block.ErrCanceled { + // errors.Is, not ==: see the equivalent comment on the backup path above. + if errors.Is(err, block.ErrCanceled) { log.Warn("Block restore is canceled") return 0, ErrorCanceled } diff --git a/pkg/uploader/provider/block_test.go b/pkg/uploader/provider/block_test.go index ad8f68b52..42375be20 100644 --- a/pkg/uploader/provider/block_test.go +++ b/pkg/uploader/provider/block_test.go @@ -372,6 +372,63 @@ func TestBlockProviderRunBackup(t *testing.T) { } } +// TestBlockProviderCancelThroughWrappedError pins that cancellation is recognized +// after the sentinel has been wrapped, which is the only way it ever arrives in +// production: block/uploader.go wraps it with "error backing up bdev %s" and +// block/snapshot.go wraps that with "Failed to run uploader backup for si %v". +// +// Asserting on the message is useless here — provider.ErrorCanceled and +// block.ErrCanceled carry the *same* text ("uploader is canceled"), so a substring +// check passes whether or not the sentinel was actually recognized. The assertion +// has to be on identity. +func TestBlockProviderCancelThroughWrappedError(t *testing.T) { + t.Run("backup", func(t *testing.T) { + orig := blockBackupFunc + defer func() { blockBackupFunc = orig }() + blockBackupFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, _ string, _ string, _ cbtservice.SourceInfo, _ bool, _ string, _ cbtservice.Service, _ map[string]string, _ map[string]string, _ logrus.FieldLogger) (uploader.SnapshotInfo, bool, error) { + return uploader.SnapshotInfo{ID: "snap-cancel", Size: 2048, IncrementalSize: 1024}, false, + errors.Wrapf( + errors.Wrapf(block.ErrCanceled, "error backing up bdev %s", "ns/pvc"), + "Failed to run uploader backup for si %v", "si") + } + + bp := &blockProvider{ + requestorType: "test", + bkRepo: udmrepomocks.NewBackupRepo(t), + log: logrus.New(), + } + + _, _, _, _, err := bp.RunBackup( + t.Context(), "/dev/sda", "ns/pvc", map[string]string{}, false, "", + CBTParam{}, uploader.PersistentVolumeBlock, map[string]string{}, + &FakeBackupProgressUpdater{}, + ) + + require.ErrorIs(t, err, ErrorCanceled, + "a wrapped block.ErrCanceled must surface as provider.ErrorCanceled; otherwise the "+ + "DataUpload is marked Failed and the Backup PartiallyFailed for a user-requested cancel") + }) + + t.Run("restore", func(t *testing.T) { + orig := blockRestoreFunc + defer func() { blockRestoreFunc = orig }() + blockRestoreFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, _ string, _ string, _ map[string]string, _ logrus.FieldLogger) (int64, error) { + return 0, errors.Wrap(block.ErrCanceled, "error restoring bdev") + } + + bp := &blockProvider{ + requestorType: "test", + bkRepo: udmrepomocks.NewBackupRepo(t), + log: logrus.New(), + } + + _, err := bp.RunRestore(t.Context(), "snap-1", "/dev/sda", + uploader.PersistentVolumeBlock, map[string]string{}, &blockMockProgressUpdater{}) + + require.ErrorIs(t, err, ErrorCanceled) + }) +} + func TestBlockProviderRunRestore(t *testing.T) { testCases := []struct { name string