From 1462c5caa54207c84a058a9026521cf103dbb500 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Fri, 14 Feb 2020 21:34:22 +0100 Subject: [PATCH] zfs: fix batch destroy panic if all snaps are undestroyable See https://github.com/zrepl/zrepl/pull/259#issuecomment-585334023 panic: runtime error: index out of range [0] with length 0 goroutine 14 [running]: github.com/zrepl/zrepl/zfs.tryBatch(0xd6aa20, 0xc0000b8018, 0xc00025e0c0, 0x0, 0x6, 0xd61d80, 0x1280df8, 0xd58920, 0xc000132000) zrepl/zfs/versions_destroy.go:129 +0x302 github.com/zrepl/zrepl/zfs.doDestroyBatchedRec(0xd6aa20, 0xc0000b8018, 0xc000578a80, 0x6, 0x6, 0xd61d80, 0x1280df8) zrepl/zfs/versions_destroy.go:184 +0x4a5 github.com/zrepl/zrepl/zfs.doDestroyBatched(0xd6aa20, 0xc0000b8018, 0xc000222780, 0x6, 0x8, 0xd61d80, 0x1280df8) zrepl/zfs/versions_destroy.go:95 +0xc7 github.com/zrepl/zrepl/zfs.doDestroy(0xd6aa20, 0xc0000b8018, 0xc0005788d0, 0x6, 0x6, 0xd61d80, 0x1280df8) zrepl/zfs/versions_destroy.go:82 +0x362 github.com/zrepl/zrepl/zfs.ZFSDestroyFilesystemVersions(...) zrepl/zfs/versions_destroy.go:41 github.com/zrepl/zrepl/endpoint.doDestroySnapshots(0xd6aaa0, 0xc0004412c0, 0xc00057ca00, 0xc0005785a0, 0x6, 0x6, 0xb68940, 0xc5df01, 0xc000150a80) zrepl/endpoint/endpoint.go:785 +0x388 github.com/zrepl/zrepl/endpoint.(*Receiver).DestroySnapshots(0xc000127500, 0xd6aaa0, 0xc0004412c0, 0xc0002ca280, 0xc000150880, 0xd73ca0, 0xc00057c960) zrepl/endpoint/endpoint.go:751 +0xdb github.com/zrepl/zrepl/daemon/pruner.doOneAttemptExec(0xc000429980, 0xc000429958, 0xc0001cb180) zrepl/daemon/pruner/pruner.go:531 +0x51f github.com/zrepl/zrepl/daemon/pruner.doOneAttempt(0xc000429980, 0xc000429958) zrepl/daemon/pruner/pruner.go:486 +0x1064 github.com/zrepl/zrepl/daemon/pruner.(*Pruner).prune(0xc00011e280, 0xd6aaa0, 0xc0004412c0, 0x7f4906fff7e8, 0xc000127500, 0x7f4906fff738, 0xc0001324e0, 0xc000064420, 0x1, 0x1, ...) zrepl/daemon/pruner/pruner.go:214 +0x53 github.com/zrepl/zrepl/daemon/pruner.(*Pruner).Prune(...) zrepl/daemon/pruner/pruner.go:200 github.com/zrepl/zrepl/daemon/job.(*ActiveSide).do(0xc000268000, 0xd6a9e0, 0xc0002223c0) zrepl/daemon/job/active.go:482 +0x906 github.com/zrepl/zrepl/daemon/job.(*ActiveSide).Run(0xc000268000, 0xd6aaa0, 0xc000127080) zrepl/daemon/job/active.go:404 +0x289 github.com/zrepl/zrepl/daemon.(*jobs).start.func1(0xc000032200, 0xd73ca0, 0xc00000f2e0, 0xd6efa0, 0xc000268000, 0xd6aaa0, 0xc000126c90) zrepl/daemon/daemon.go:220 +0x121 created by github.com/zrepl/zrepl/daemon.(*jobs).start zrepl/daemon/daemon.go:216 +0x52e --- zfs/versions_destroy.go | 4 +++ zfs/versions_destroy_test.go | 62 ++++++++++++++++++++++++++++++++---- 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/zfs/versions_destroy.go b/zfs/versions_destroy.go index fc2baec..031af30 100644 --- a/zfs/versions_destroy.go +++ b/zfs/versions_destroy.go @@ -126,6 +126,10 @@ func buildBatches(reqs []*DestroySnapOp) [][]*DestroySnapOp { // batch must be on same Filesystem, panics otherwise func tryBatch(ctx context.Context, batch []*DestroySnapOp, d destroyer) error { + if len(batch) == 0 { + return nil + } + batchFS := batch[0].Filesystem batchNames := make([]string, len(batch)) for i := range batchNames { diff --git a/zfs/versions_destroy_test.go b/zfs/versions_destroy_test.go index 3249dd0..112ae83 100644 --- a/zfs/versions_destroy_test.go +++ b/zfs/versions_destroy_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "os/exec" + "regexp" "strings" "syscall" "testing" @@ -19,7 +20,7 @@ type mockBatchDestroy struct { mtx chainlock.L calls []string commaUnsupported bool - undestroyable string + undestroyable *regexp.Regexp randomerror string e2biglen int } @@ -38,17 +39,38 @@ func (m *mockBatchDestroy) Destroy(args []string) error { return &os.PathError{Err: syscall.E2BIG} // TestExcessiveArgumentsResultInE2BIG checks that this errors is produced } m.calls = append(m.calls, a) + + var snapnames []string if m.commaUnsupported { + snapnames = append(snapnames, a) if strings.Contains(a, ",") { return fmt.Errorf("unsupported syntax mock error") } + } else { + snapnames = append(snapnames, strings.Split(a, ",")...) } + fs, vt, firstsnapname, err := DecomposeVersionString(snapnames[0]) + if err != nil { + panic(err) + } + if vt != Snapshot { + panic(vt) + } + snapnames[0] = firstsnapname - if m.undestroyable != "" && strings.Contains(a, m.undestroyable) { + var undestroyable []string + if m.undestroyable != nil { + for _, snap := range snapnames { + if m.undestroyable.MatchString(snap) { + undestroyable = append(undestroyable, snap) + } + } + } + if len(undestroyable) > 0 { return &DestroySnapshotsError{ - Filesystem: "PLACEHOLDER", - Undestroyable: []string{m.undestroyable}, - Reason: []string{"undestroyable reason"}, + Filesystem: fs, + Undestroyable: undestroyable, + Reason: []string{"undestroyable mock with regexp " + m.undestroyable.String()}, } } if m.randomerror != "" && strings.Contains(a, m.randomerror) { @@ -82,7 +104,7 @@ func TestBatchDestroySnaps(t *testing.T) { nilErrs() mock := &mockBatchDestroy{ commaUnsupported: false, - undestroyable: "undestroyable", + undestroyable: regexp.MustCompile(`undestroyable`), randomerror: "randomerror", } @@ -118,11 +140,37 @@ func TestBatchDestroySnaps(t *testing.T) { ) }) + t.Run("all_undestroyable", func(t *testing.T) { + + opsTemplate := []*DestroySnapOp{ + &DestroySnapOp{"zroot/a", "foo", new(error)}, + &DestroySnapOp{"zroot/a", "bar", new(error)}, + } + + mock := &mockBatchDestroy{ + commaUnsupported: false, + undestroyable: regexp.MustCompile(`.*`), + } + + doDestroy(context.TODO(), opsTemplate, mock) + + defer mock.mtx.Lock().Unlock() + assert.Equal( + t, + []string{ + "zroot/a@bar,foo", // reordered snaps in lexicographical order + "zroot/a@bar", + "zroot/a@foo", + }, + mock.calls, + ) + }) + t.Run("comma_syntax_unsupported", func(t *testing.T) { nilErrs() mock := &mockBatchDestroy{ commaUnsupported: true, - undestroyable: "undestroyable", + undestroyable: regexp.MustCompile(`undestroyable`), randomerror: "randomerror", }