From 233a6c9b2491d7a4516632bf09e2cce447b7aed7 Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Tue, 25 Feb 2020 14:54:35 +0100 Subject: [PATCH 1/7] Skip deleting files that we just deleted We see this happening with Swift. Because the consistency of swift is eventual, swift sometimes didn't process the deletion of the meta file yet, and so it turns up in the bkt.Iter(). The second deletion then causes a 404 and compaction fails. Signed-off-by: Wim Fournier --- pkg/block/block.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/block/block.go b/pkg/block/block.go index 740b3d6306..efb8706576 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -156,6 +156,11 @@ func deleteDir(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir if strings.HasSuffix(name, objstore.DirDelim) { return deleteDir(ctx, logger, bkt, name) } + metaFile := path.Join(id.String(), MetaFilename) + if name == metaFile { + // skip this file if we found it, it was already deleted + continue + } if err := bkt.Delete(ctx, name); err != nil { return err } From 76b394790ddc23827f0f3a6e99ed01ebc6fcfb58 Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Tue, 25 Feb 2020 15:28:58 +0100 Subject: [PATCH 2/7] return, as this is a func. Add debug log and comment Signed-off-by: Wim Fournier --- pkg/block/block.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/block/block.go b/pkg/block/block.go index efb8706576..a4ee061890 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -157,9 +157,9 @@ func deleteDir(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir return deleteDir(ctx, logger, bkt, name) } metaFile := path.Join(id.String(), MetaFilename) - if name == metaFile { - // skip this file if we found it, it was already deleted - continue + if name == metaFile { // the metaFile was already deleted in Delete(), but might still appear in the Iter() list + level.Debug(logger).Log("msg", "skipping deletion of meta file, as it should already be deleted", "file", name, "bucket", bkt.Name()) + return nil } if err := bkt.Delete(ctx, name); err != nil { return err From a493eb8d55e2b035b88ca6617299a5a3186a320f Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Thu, 27 Feb 2020 08:47:17 +0100 Subject: [PATCH 3/7] fixing build: wrong parameter name Signed-off-by: Wim Fournier --- pkg/block/block.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/block/block.go b/pkg/block/block.go index a4ee061890..b5d9e28a98 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -156,7 +156,7 @@ func deleteDir(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir if strings.HasSuffix(name, objstore.DirDelim) { return deleteDir(ctx, logger, bkt, name) } - metaFile := path.Join(id.String(), MetaFilename) + metaFile := path.Join(dir, MetaFilename) if name == metaFile { // the metaFile was already deleted in Delete(), but might still appear in the Iter() list level.Debug(logger).Log("msg", "skipping deletion of meta file, as it should already be deleted", "file", name, "bucket", bkt.Name()) return nil From f3d25fdc38f18c72979d210b67b6f265ff8251f9 Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Thu, 27 Feb 2020 08:59:01 +0100 Subject: [PATCH 4/7] fix lint Signed-off-by: Wim Fournier --- pkg/block/block.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/block/block.go b/pkg/block/block.go index b5d9e28a98..77eff1b9a7 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -157,7 +157,7 @@ func deleteDir(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir return deleteDir(ctx, logger, bkt, name) } metaFile := path.Join(dir, MetaFilename) - if name == metaFile { // the metaFile was already deleted in Delete(), but might still appear in the Iter() list + if name == metaFile { // the metaFile was already deleted in Delete(), but might still appear in the Iter() list. level.Debug(logger).Log("msg", "skipping deletion of meta file, as it should already be deleted", "file", name, "bucket", bkt.Name()) return nil } From bda73cdedc98b5318ef795d16323053bb28b33ca Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Mon, 2 Mar 2020 15:16:57 +0100 Subject: [PATCH 5/7] Refactor deleteDir into deleteDirRec and add a parameter for a function that allows to keep certain files. Signed-off-by: Wim Fournier --- pkg/block/block.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/block/block.go b/pkg/block/block.go index 77eff1b9a7..220585219d 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -145,20 +145,25 @@ func Delete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, id ulid level.Debug(logger).Log("msg", "deleted file", "file", metaFile, "bucket", bkt.Name()) } - return deleteDir(ctx, logger, bkt, id.String()) + // Delete the bucket, but skip the metaFile, if found. As we just deleted that. + return deleteDirRec(ctx, logger, bkt, id.String(), func(name string) bool { + if name == metaFile { + return true + } + return false + }) } -// deleteDir removes all objects prefixed with dir from the bucket. +// deleteDirRec removes all objects prefixed with dir from the bucket. It skips objects that return true for the passed keep func // NOTE: For objects removal use `block.Delete` strictly. -func deleteDir(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir string) error { +func deleteDirRec(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir string, keep func(name string) bool) error { return bkt.Iter(ctx, dir, func(name string) error { // If we hit a directory, call DeleteDir recursively. if strings.HasSuffix(name, objstore.DirDelim) { - return deleteDir(ctx, logger, bkt, name) + return deleteDirRec(ctx, logger, bkt, name, keep) } - metaFile := path.Join(dir, MetaFilename) - if name == metaFile { // the metaFile was already deleted in Delete(), but might still appear in the Iter() list. - level.Debug(logger).Log("msg", "skipping deletion of meta file, as it should already be deleted", "file", name, "bucket", bkt.Name()) + if keep(name) { + level.Debug(logger).Log("msg", "skipping deletion of object, as requested by keep()", "file", name, "bucket", bkt.Name()) return nil } if err := bkt.Delete(ctx, name); err != nil { From 5c786ecae2b19351e17c006cb2043b4854fe321f Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Mon, 2 Mar 2020 15:35:23 +0100 Subject: [PATCH 6/7] Fix lint Signed-off-by: Wim Fournier --- pkg/block/block.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/block/block.go b/pkg/block/block.go index 220585219d..748b7a1a22 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -147,10 +147,7 @@ func Delete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, id ulid // Delete the bucket, but skip the metaFile, if found. As we just deleted that. return deleteDirRec(ctx, logger, bkt, id.String(), func(name string) bool { - if name == metaFile { - return true - } - return false + return name == metaFile }) } From e4f43a98e81c6536ffec7ebe6f492a105323edf9 Mon Sep 17 00:00:00 2001 From: Wim Fournier Date: Mon, 2 Mar 2020 20:45:30 +0100 Subject: [PATCH 7/7] implementing suggested fixes Signed-off-by: Wim Fournier --- pkg/block/block.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/block/block.go b/pkg/block/block.go index 748b7a1a22..df83f17649 100644 --- a/pkg/block/block.go +++ b/pkg/block/block.go @@ -145,13 +145,13 @@ func Delete(ctx context.Context, logger log.Logger, bkt objstore.Bucket, id ulid level.Debug(logger).Log("msg", "deleted file", "file", metaFile, "bucket", bkt.Name()) } - // Delete the bucket, but skip the metaFile, if found. As we just deleted that. + // Delete the bucket, but skip the metaFile as we just deleted that. This is required for eventual object storages (list after write). return deleteDirRec(ctx, logger, bkt, id.String(), func(name string) bool { return name == metaFile }) } -// deleteDirRec removes all objects prefixed with dir from the bucket. It skips objects that return true for the passed keep func +// deleteDirRec removes all objects prefixed with dir from the bucket. It skips objects that return true for the passed keep function. // NOTE: For objects removal use `block.Delete` strictly. func deleteDirRec(ctx context.Context, logger log.Logger, bkt objstore.Bucket, dir string, keep func(name string) bool) error { return bkt.Iter(ctx, dir, func(name string) error { @@ -160,7 +160,6 @@ func deleteDirRec(ctx context.Context, logger log.Logger, bkt objstore.Bucket, d return deleteDirRec(ctx, logger, bkt, name, keep) } if keep(name) { - level.Debug(logger).Log("msg", "skipping deletion of object, as requested by keep()", "file", name, "bucket", bkt.Name()) return nil } if err := bkt.Delete(ctx, name); err != nil {