From 19262730c1d69419a5e206b05f0abf51ab432633 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Thu, 18 Jul 2024 16:54:40 +0300 Subject: [PATCH] [#431] Delete all split version at once Previously after split we can get two `null` versioned object with the same key and deleting such key removes only one node/object. Signed-off-by: Denis Kirillov --- api/handler/delete_test.go | 17 +++++ api/layer/layer.go | 132 ++++++++++++++++++++++++++----------- internal/logs/logs.go | 1 + 3 files changed, 112 insertions(+), 38 deletions(-) diff --git a/api/handler/delete_test.go b/api/handler/delete_test.go index 362a29a..7231a55 100644 --- a/api/handler/delete_test.go +++ b/api/handler/delete_test.go @@ -346,6 +346,23 @@ func TestDeleteObjectSuspended(t *testing.T) { require.False(t, existInMockedFrostFS(tc, bktInfo, objInfo), "object exists but shouldn't") } +func TestDeleteObjectSuspendedNull(t *testing.T) { + tc := prepareHandlerContext(t) + + bktName, objName := "bucket-for-removal", "object-to-delete" + bktInfo, objInfo := createBucketAndObject(tc, bktName, objName) + + putBucketVersioning(t, tc, bktName, true) + putObject(tc, bktName, objName) + + putBucketVersioning(t, tc, bktName, false) + + deleteObject(t, tc, bktName, objName, emptyVersion) + checkNotFound(t, tc, bktName, objName, emptyVersion) + + require.False(t, existInMockedFrostFS(tc, bktInfo, objInfo), "object exists but shouldn't") +} + func TestDeleteMarkers(t *testing.T) { tc := prepareHandlerContext(t) diff --git a/api/layer/layer.go b/api/layer/layer.go index 07bc263..9091ca6 100644 --- a/api/layer/layer.go +++ b/api/layer/layer.go @@ -6,9 +6,11 @@ import ( "crypto/rand" "encoding/json" "encoding/xml" + stderrors "errors" "fmt" "io" "net/url" + "sort" "strconv" "strings" "time" @@ -16,6 +18,7 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" + s3errors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/encryption" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs" @@ -660,17 +663,26 @@ func getRandomOID() (oid.ID, error) { func (n *layer) deleteObject(ctx context.Context, bkt *data.BucketInfo, settings *data.BucketSettings, obj *VersionedObject) *VersionedObject { if len(obj.VersionID) != 0 || settings.Unversioned() { - var nodeVersion *data.NodeVersion - if nodeVersion, obj.Error = n.getNodeVersionToDelete(ctx, bkt, obj); obj.Error != nil { + var nodeVersions []*data.NodeVersion + if nodeVersions, obj.Error = n.getNodeVersionsToDelete(ctx, bkt, obj); obj.Error != nil { return n.handleNotFoundError(bkt, obj) } - if obj.DeleteMarkVersion, obj.Error = n.removeOldVersion(ctx, bkt, nodeVersion, obj); obj.Error != nil { - return n.handleObjectDeleteErrors(ctx, bkt, obj, nodeVersion.ID) + for _, nodeVersion := range nodeVersions { + if obj.DeleteMarkVersion, obj.Error = n.removeOldVersion(ctx, bkt, nodeVersion, obj); obj.Error != nil { + if !client.IsErrObjectAlreadyRemoved(obj.Error) && !client.IsErrObjectNotFound(obj.Error) { + return obj + } + n.reqLogger(ctx).Debug(logs.CouldntDeleteObjectFromStorageContinueDeleting, + zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error(obj.Error)) + } + + if obj.Error = n.treeService.RemoveVersion(ctx, bkt, nodeVersion.ID); obj.Error != nil { + return obj + } } - obj.Error = n.treeService.RemoveVersion(ctx, bkt, nodeVersion.ID) - n.cache.CleanListCacheEntriesContainingObject(obj.Name, bkt.CID) + n.cache.DeleteObjectName(bkt.CID, bkt.Name, obj.Name) return obj } @@ -683,20 +695,30 @@ func (n *layer) deleteObject(ctx context.Context, bkt *data.BucketInfo, settings if settings.VersioningSuspended() { obj.VersionID = data.UnversionedObjectVersionID - var nullVersionToDelete *data.NodeVersion - if lastVersion.IsUnversioned { - if !lastVersion.IsDeleteMarker { - nullVersionToDelete = lastVersion - } - } else if nullVersionToDelete, obj.Error = n.getNodeVersionToDelete(ctx, bkt, obj); obj.Error != nil { + var nodeVersions []*data.NodeVersion + if nodeVersions, obj.Error = n.getNodeVersionsToDelete(ctx, bkt, obj); obj.Error != nil { if !isNotFoundError(obj.Error) { return obj } } - if nullVersionToDelete != nil { - if obj.DeleteMarkVersion, obj.Error = n.removeOldVersion(ctx, bkt, nullVersionToDelete, obj); obj.Error != nil { - return n.handleObjectDeleteErrors(ctx, bkt, obj, nullVersionToDelete.ID) + for _, nodeVersion := range nodeVersions { + if nodeVersion.ID == lastVersion.ID && nodeVersion.IsDeleteMarker { + continue + } + + if !nodeVersion.IsDeleteMarker { + if obj.DeleteMarkVersion, obj.Error = n.removeOldVersion(ctx, bkt, nodeVersion, obj); obj.Error != nil { + if !client.IsErrObjectAlreadyRemoved(obj.Error) && !client.IsErrObjectNotFound(obj.Error) { + return obj + } + n.reqLogger(ctx).Debug(logs.CouldntDeleteObjectFromStorageContinueDeleting, + zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error(obj.Error)) + } + } + + if obj.Error = n.treeService.RemoveVersion(ctx, bkt, nodeVersion.ID); obj.Error != nil { + return obj } } } @@ -744,36 +766,70 @@ func (n *layer) handleNotFoundError(bkt *data.BucketInfo, obj *VersionedObject) return obj } -func (n *layer) handleObjectDeleteErrors(ctx context.Context, bkt *data.BucketInfo, obj *VersionedObject, nodeID uint64) *VersionedObject { - if !client.IsErrObjectAlreadyRemoved(obj.Error) && !client.IsErrObjectNotFound(obj.Error) { - return obj - } - - n.reqLogger(ctx).Debug(logs.CouldntDeleteObjectFromStorageContinueDeleting, - zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error(obj.Error)) - - obj.Error = n.treeService.RemoveVersion(ctx, bkt, nodeID) - if obj.Error == nil { - n.cache.DeleteObjectName(bkt.CID, bkt.Name, obj.Name) - } - - return obj -} - func isNotFoundError(err error) bool { return errors.IsS3Error(err, errors.ErrNoSuchKey) || errors.IsS3Error(err, errors.ErrNoSuchVersion) } -func (n *layer) getNodeVersionToDelete(ctx context.Context, bkt *data.BucketInfo, obj *VersionedObject) (*data.NodeVersion, error) { - objVersion := &ObjectVersion{ - BktInfo: bkt, - ObjectName: obj.Name, - VersionID: obj.VersionID, - NoErrorOnDeleteMarker: true, +func (n *layer) getNodeVersionsToDelete(ctx context.Context, bkt *data.BucketInfo, obj *VersionedObject) ([]*data.NodeVersion, error) { + var versionsToDelete []*data.NodeVersion + versions, err := n.treeService.GetVersions(ctx, bkt, obj.Name) + if err != nil { + if stderrors.Is(err, ErrNodeNotFound) { + return nil, fmt.Errorf("%w: %s", s3errors.GetAPIError(s3errors.ErrNoSuchKey), err.Error()) + } + return nil, err } - return n.getNodeVersion(ctx, objVersion) + if len(versions) == 0 { + return nil, fmt.Errorf("%w: there isn't tree node with requested version id", s3errors.GetAPIError(s3errors.ErrNoSuchVersion)) + } + + sort.Slice(versions, func(i, j int) bool { + return versions[i].Timestamp < versions[j].Timestamp + }) + + var matchFn func(nv *data.NodeVersion) bool + + switch { + case obj.VersionID == data.UnversionedObjectVersionID: + matchFn = func(nv *data.NodeVersion) bool { + return nv.IsUnversioned + } + case len(obj.VersionID) == 0: + latest := versions[len(versions)-1] + if latest.IsUnversioned { + matchFn = func(nv *data.NodeVersion) bool { + return nv.IsUnversioned + } + } else { + matchFn = func(nv *data.NodeVersion) bool { + return nv.ID == latest.ID + } + } + default: + matchFn = func(nv *data.NodeVersion) bool { + return nv.OID.EncodeToString() == obj.VersionID + } + } + + var oids []string + for _, v := range versions { + if matchFn(v) { + versionsToDelete = append(versionsToDelete, v) + if !v.IsDeleteMarker { + oids = append(oids, v.OID.EncodeToString()) + } + } + } + + if len(versionsToDelete) == 0 { + return nil, fmt.Errorf("%w: there isn't tree node with requested version id", s3errors.GetAPIError(s3errors.ErrNoSuchVersion)) + } + + n.reqLogger(ctx).Debug(logs.GetTreeNodeToDelete, zap.Stringer("cid", bkt.CID), zap.Strings("oids", oids)) + + return versionsToDelete, nil } func (n *layer) getLastNodeVersion(ctx context.Context, bkt *data.BucketInfo, obj *VersionedObject) (*data.NodeVersion, error) { diff --git a/internal/logs/logs.go b/internal/logs/logs.go index 4d4b4c4..b723ffc 100644 --- a/internal/logs/logs.go +++ b/internal/logs/logs.go @@ -87,6 +87,7 @@ const ( FailedToSubmitTaskToPool = "failed to submit task to pool" // Warn in ../../api/layer/object.go CouldNotFetchObjectMeta = "could not fetch object meta" // Warn in ../../api/layer/object.go GetTreeNode = "get tree node" // Debug in ../../api/layer/tagging.go + GetTreeNodeToDelete = "get tree node to delete" // Debug in ../../api/layer/tagging.go CouldntPutBucketInfoIntoCache = "couldn't put bucket info into cache" // Warn in ../../api/layer/cache.go CouldntAddObjectToCache = "couldn't add object to cache" // Warn in ../../api/layer/cache.go CouldntCacheAccessControlOperation = "couldn't cache access control operation" // Warn in ../../api/layer/cache.go