From cbd0a31a34aa1be649bd4277ff016d521c45238a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Wed, 24 Mar 2021 17:17:54 +0200 Subject: [PATCH] compact: clean up directories thoroughly (#3869) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * compact: clean up directories properly I couldn't stop thinking about this code for some reason and I have figured that I had missed one case in #3031. We need to also clean up the directories in compaction groups. A compaction could fail leaving some new directory with a random ULID on the disk. Before attempting to do another compaction loop, we need to remove it as well because the compaction process always produces a new, unique directory. Signed-off-by: Giedrius Statkevičius * Remove all non expected dirs. Signed-off-by: Bartlomiej Plotka * Addressed comment. Signed-off-by: Bartlomiej Plotka Co-authored-by: Bartlomiej Plotka --- pkg/compact/compact.go | 4 +++- pkg/runutil/runutil.go | 31 ++++++++++++++++++++++++------- pkg/runutil/runutil_test.go | 34 +++++++++++++++++++++------------- test/e2e/compact_test.go | 24 +++++++++++++++++++++++- 4 files changed, 71 insertions(+), 22 deletions(-) diff --git a/pkg/compact/compact.go b/pkg/compact/compact.go index 7b0c6c9084..23ee8c39b0 100644 --- a/pkg/compact/compact.go +++ b/pkg/compact/compact.go @@ -957,7 +957,9 @@ func (c *BucketCompactor) Compact(ctx context.Context) (rerr error) { ignoreDirs := []string{} for _, gr := range groups { - ignoreDirs = append(ignoreDirs, gr.Key()) + for _, grID := range gr.IDs() { + ignoreDirs = append(ignoreDirs, filepath.Join(gr.Key(), grID.String())) + } } if err := runutil.DeleteAll(c.compactDir, ignoreDirs...); err != nil { diff --git a/pkg/runutil/runutil.go b/pkg/runutil/runutil.go index 97171bce82..f726eb25c3 100644 --- a/pkg/runutil/runutil.go +++ b/pkg/runutil/runutil.go @@ -55,6 +55,7 @@ import ( "io/ioutil" "os" "path/filepath" + "strings" "time" "github.com/go-kit/kit/log" @@ -162,6 +163,7 @@ func ExhaustCloseWithErrCapture(err *error, r io.ReadCloser, format string, a .. // DeleteAll deletes all files and directories inside the given // dir except for the ignoreDirs directories. +// NOTE: DeleteAll is not idempotent. func DeleteAll(dir string, ignoreDirs ...string) error { entries, err := ioutil.ReadDir(dir) if err != nil { @@ -169,6 +171,7 @@ func DeleteAll(dir string, ignoreDirs ...string) error { } var groupErrs errutil.MultiError + var matchingIgnores []string for _, d := range entries { if !d.IsDir() { if err := os.RemoveAll(filepath.Join(dir, d.Name())); err != nil { @@ -177,22 +180,36 @@ func DeleteAll(dir string, ignoreDirs ...string) error { continue } - var found bool - for _, id := range ignoreDirs { - if id == d.Name() { - found = true - break + // ignoreDirs might be multi-directory paths. + matchingIgnores = matchingIgnores[:0] + ignore := false + for _, ignoreDir := range ignoreDirs { + id := strings.Split(ignoreDir, "/") + if id[0] == d.Name() { + if len(id) == 1 { + ignore = true + break + } + matchingIgnores = append(matchingIgnores, filepath.Join(id[1:]...)) } } - if !found { + if ignore { + continue + } + + if len(matchingIgnores) == 0 { if err := os.RemoveAll(filepath.Join(dir, d.Name())); err != nil { groupErrs.Add(err) } + continue + } + if err := DeleteAll(filepath.Join(dir, d.Name()), matchingIgnores...); err != nil { + groupErrs.Add(err) } } - if groupErrs != nil { + if groupErrs.Err() != nil { return errors.Wrap(groupErrs.Err(), "delete file/dir") } return nil diff --git a/pkg/runutil/runutil_test.go b/pkg/runutil/runutil_test.go index 908a3398a3..a50efbd356 100644 --- a/pkg/runutil/runutil_test.go +++ b/pkg/runutil/runutil_test.go @@ -125,35 +125,43 @@ func TestCloseMoreThanOnce(t *testing.T) { testutil.Equals(t, true, lc.WasCalled) } -func TestClearsDirectoriesFilesProperly(t *testing.T) { +func TestDeleteAll(t *testing.T) { dir, err := ioutil.TempDir("", "example") testutil.Ok(t, err) t.Cleanup(func() { - os.RemoveAll(dir) + testutil.Ok(t, os.RemoveAll(dir)) }) - f, err := os.Create(filepath.Join(dir, "test123")) + f, err := os.Create(filepath.Join(dir, "file1")) testutil.Ok(t, err) testutil.Ok(t, f.Close()) - testutil.Ok(t, os.MkdirAll(filepath.Join(dir, "01EHBQRN4RF0HSRR1772KW0TN8"), os.ModePerm)) - testutil.Ok(t, os.MkdirAll(filepath.Join(dir, "01EHBQRN4RF0HSRR1772KW1TN8"), os.ModePerm)) - f, err = os.Create(filepath.Join(dir, "01EHBQRN4RF0HSRR1772KW0TN9")) + testutil.Ok(t, os.MkdirAll(filepath.Join(dir, "a"), os.ModePerm)) + testutil.Ok(t, os.MkdirAll(filepath.Join(dir, "b"), os.ModePerm)) + testutil.Ok(t, os.MkdirAll(filepath.Join(dir, "c", "innerc"), os.ModePerm)) + f, err = os.Create(filepath.Join(dir, "a", "file2")) + testutil.Ok(t, err) + testutil.Ok(t, f.Close()) + f, err = os.Create(filepath.Join(dir, "c", "file3")) testutil.Ok(t, err) testutil.Ok(t, f.Close()) - testutil.Ok(t, DeleteAll(dir, "01EHBQRN4RF0HSRR1772KW0TN9", "01EHBQRN4RF0HSRR1772KW0TN8")) + testutil.Ok(t, DeleteAll(dir, "file1", "a", filepath.Join("c", "innerc"))) - _, err = os.Stat(filepath.Join(dir, "test123")) + // Deleted. + _, err = os.Stat(filepath.Join(dir, "file1")) testutil.Assert(t, os.IsNotExist(err)) - - _, err = os.Stat(filepath.Join(dir, "01EHBQRN4RF0HSRR1772KW0TN9")) + _, err = os.Stat(filepath.Join(dir, "b/")) testutil.Assert(t, os.IsNotExist(err)) - - _, err = os.Stat(filepath.Join(dir, "01EHBQRN4RF0HSRR1772KW1TN8/")) + _, err = os.Stat(filepath.Join(dir, "file3")) testutil.Assert(t, os.IsNotExist(err)) - _, err = os.Stat(filepath.Join(dir, "01EHBQRN4RF0HSRR1772KW0TN8/")) + // Exists. + _, err = os.Stat(filepath.Join(dir, "a", "file2")) + testutil.Ok(t, err) + _, err = os.Stat(filepath.Join(dir, "a/")) + testutil.Ok(t, err) + _, err = os.Stat(filepath.Join(dir, "c", "innerc")) testutil.Ok(t, err) } diff --git a/test/e2e/compact_test.go b/test/e2e/compact_test.go index 1934af2393..57a9951353 100644 --- a/test/e2e/compact_test.go +++ b/test/e2e/compact_test.go @@ -539,8 +539,26 @@ func TestCompactWithStoreGateway(t *testing.T) { // No replica label with overlaps should halt compactor. This test is sequential // because we do not want two Thanos Compact instances deleting the same partially - // uploaded blocks and blocks with deletion marks. + // uploaded blocks and blocks with deletion marks. We also check that Thanos Compactor + // deletes directories inside of a compaction group that do not belong there. { + // Precreate a directory. It should be deleted. + // In a hypothetical scenario, the directory could be a left-over from + // a compaction that had crashed. + p := filepath.Join(s.SharedDir(), "data", "compact", "expect-to-halt", "compact") + + testutil.Assert(t, len(blocksWithHashes) > 0) + + m, err := block.DownloadMeta(ctx, logger, bkt, blocksWithHashes[0]) + testutil.Ok(t, err) + + randBlockDir := filepath.Join(p, compact.DefaultGroupKey(m.Thanos), "ITISAVERYRANDULIDFORTESTS0") + testutil.Ok(t, os.MkdirAll(randBlockDir, os.ModePerm)) + + f, err := os.Create(filepath.Join(randBlockDir, "index")) + testutil.Ok(t, err) + testutil.Ok(t, f.Close()) + c, err := e2ethanos.NewCompactor(s.SharedDir(), "expect-to-halt", svcConfig, nil) testutil.Ok(t, err) testutil.Ok(t, s.StartAndWaitReady(c)) @@ -578,6 +596,10 @@ func TestCompactWithStoreGateway(t *testing.T) { ensureGETStatusCode(t, http.StatusOK, "http://"+path.Join(c.HTTPEndpoint(), "loaded")) testutil.Ok(t, s.Stop(c)) + + _, err = os.Stat(randBlockDir) + testutil.NotOk(t, err) + testutil.Assert(t, os.IsNotExist(err)) } // Sequential because we want to check that Thanos Compactor does not