From f2437f7ae962bae537666bc0914bc9db9a5ddb75 Mon Sep 17 00:00:00 2001 From: Dmitrii Stepanov Date: Fri, 13 Oct 2023 16:28:04 +0300 Subject: [PATCH] [#734] shard: Fix Delete method Due to the flushing data from the writecache to the storage and simultaneous deletion, a partial deletion situation is possible. So as a solution, deletion is allowed only when the object is in storage, because object will be deleted from writecache by flush goroutine. Signed-off-by: Dmitrii Stepanov --- pkg/local_object_storage/shard/delete.go | 37 ++++++++----------- pkg/local_object_storage/shard/delete_test.go | 34 +++++++++++------ pkg/local_object_storage/shard/gc.go | 2 +- 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/pkg/local_object_storage/shard/delete.go b/pkg/local_object_storage/shard/delete.go index ea481300..e24bd854 100644 --- a/pkg/local_object_storage/shard/delete.go +++ b/pkg/local_object_storage/shard/delete.go @@ -2,15 +2,12 @@ package shard import ( "context" - "errors" "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor/common" meta "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/metabase" - "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/writecache" tracingPkg "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/tracing" "git.frostfs.info/TrueCloudLab/frostfs-observability/tracing" - "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client" oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -34,8 +31,7 @@ func (p *DeletePrm) SetAddresses(addr ...oid.Address) { p.addr = append(p.addr, addr...) } -// Delete removes data from the shard's writeCache, metaBase and -// blobStor. +// Delete removes data from the shard's metaBase and// blobStor. func (s *Shard) Delete(ctx context.Context, prm DeletePrm) (DeleteRes, error) { ctx, span := tracing.StartSpanFromContext(ctx, "Shard.Delete", trace.WithAttributes( @@ -47,10 +43,10 @@ func (s *Shard) Delete(ctx context.Context, prm DeletePrm) (DeleteRes, error) { s.m.RLock() defer s.m.RUnlock() - return s.delete(ctx, prm) + return s.delete(ctx, prm, false) } -func (s *Shard) delete(ctx context.Context, prm DeletePrm) (DeleteRes, error) { +func (s *Shard) delete(ctx context.Context, prm DeletePrm, skipFailed bool) (DeleteRes, error) { if s.info.Mode.ReadOnly() { return DeleteRes{}, ErrReadOnlyMode } else if s.info.Mode.NoMetabase() { @@ -65,12 +61,18 @@ func (s *Shard) delete(ctx context.Context, prm DeletePrm) (DeleteRes, error) { default: } - s.deleteObjectFromWriteCacheSafe(ctx, addr) - - s.deleteFromBlobstorSafe(ctx, addr) + if err := s.deleteFromBlobstor(ctx, addr); err != nil { + if skipFailed { + continue + } + return result, err + } if err := s.deleteFromMetabase(ctx, addr); err != nil { - return result, err // stop on metabase error ? + if skipFailed { + continue + } + return result, err } result.deleted++ } @@ -78,16 +80,7 @@ func (s *Shard) delete(ctx context.Context, prm DeletePrm) (DeleteRes, error) { return result, nil } -func (s *Shard) deleteObjectFromWriteCacheSafe(ctx context.Context, addr oid.Address) { - if s.hasWriteCache() { - err := s.writeCache.Delete(ctx, addr) - if err != nil && !client.IsErrObjectNotFound(err) && !errors.Is(err, writecache.ErrReadOnly) { - s.log.Warn(logs.ShardCantDeleteObjectFromWriteCache, zap.Error(err)) - } - } -} - -func (s *Shard) deleteFromBlobstorSafe(ctx context.Context, addr oid.Address) { +func (s *Shard) deleteFromBlobstor(ctx context.Context, addr oid.Address) error { var sPrm meta.StorageIDPrm sPrm.SetAddress(addr) @@ -97,6 +90,7 @@ func (s *Shard) deleteFromBlobstorSafe(ctx context.Context, addr oid.Address) { zap.Stringer("object", addr), zap.String("error", err.Error()), zap.String("trace_id", tracingPkg.GetTraceID(ctx))) + return err } storageID := res.StorageID() @@ -111,6 +105,7 @@ func (s *Shard) deleteFromBlobstorSafe(ctx context.Context, addr oid.Address) { zap.String("error", err.Error()), zap.String("trace_id", tracingPkg.GetTraceID(ctx))) } + return err } func (s *Shard) deleteFromMetabase(ctx context.Context, addr oid.Address) error { diff --git a/pkg/local_object_storage/shard/delete_test.go b/pkg/local_object_storage/shard/delete_test.go index 3421ac9e..910528f8 100644 --- a/pkg/local_object_storage/shard/delete_test.go +++ b/pkg/local_object_storage/shard/delete_test.go @@ -52,13 +52,18 @@ func testShardDelete(t *testing.T, hasWriteCache bool) { _, err = testGet(t, sh, getPrm, hasWriteCache) require.NoError(t, err) - _, err = sh.Delete(context.TODO(), delPrm) - require.NoError(t, err) + if hasWriteCache { + require.Eventually(t, func() bool { + _, err = sh.Delete(context.Background(), delPrm) + return err == nil + }, 30*time.Second, 100*time.Millisecond) + } else { + _, err = sh.Delete(context.Background(), delPrm) + require.NoError(t, err) + } - require.Eventually(t, func() bool { - _, err = sh.Get(context.Background(), getPrm) - return client.IsErrObjectNotFound(err) - }, time.Second, 50*time.Millisecond) + _, err = sh.Get(context.Background(), getPrm) + require.True(t, client.IsErrObjectNotFound(err)) }) t.Run("small object", func(t *testing.T) { @@ -78,12 +83,17 @@ func testShardDelete(t *testing.T, hasWriteCache bool) { _, err = sh.Get(context.Background(), getPrm) require.NoError(t, err) - _, err = sh.Delete(context.Background(), delPrm) - require.NoError(t, err) + if hasWriteCache { + require.Eventually(t, func() bool { + _, err = sh.Delete(context.Background(), delPrm) + return err == nil + }, 10*time.Second, 100*time.Millisecond) + } else { + _, err = sh.Delete(context.Background(), delPrm) + require.NoError(t, err) + } - require.Eventually(t, func() bool { - _, err = sh.Get(context.Background(), getPrm) - return client.IsErrObjectNotFound(err) - }, time.Second, 50*time.Millisecond) + _, err = sh.Get(context.Background(), getPrm) + require.True(t, client.IsErrObjectNotFound(err)) }) } diff --git a/pkg/local_object_storage/shard/gc.go b/pkg/local_object_storage/shard/gc.go index 13ab39ae..0ce99b47 100644 --- a/pkg/local_object_storage/shard/gc.go +++ b/pkg/local_object_storage/shard/gc.go @@ -297,7 +297,7 @@ func (s *Shard) removeGarbage(pctx context.Context) (result gcRunResult) { deletePrm.SetAddresses(buf...) // delete accumulated objects - res, err := s.delete(ctx, deletePrm) + res, err := s.delete(ctx, deletePrm, true) result.deleted = res.deleted result.failedToDelete = uint64(len(buf)) - res.deleted