[#559] Return error from multipart deleting #574

mbiryukova · 2024-12-11T04:22:41Z

mbiryukova commented

2024-12-11 04:22:41 +00:00

Signed-off-by: Marina Biryukova m.biryukova@yadro.com

Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova self-assigned this 2024-12-11 04:22:41 +00:00

mbiryukova added 1 commit 2024-12-11 04:22:42 +00:00

[#559 ] Return error from multipart deleting

/ DCO (pull_request) Successful in 1m13s

Details

/ Vulncheck (pull_request) Successful in 3m19s

Details

/ Builds (pull_request) Successful in 2m2s

Details

/ Lint (pull_request) Successful in 4m7s

Details

/ Tests (pull_request) Successful in 2m6s

Details

5b10514bd3

Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova requested review from storage-services-developers 2024-12-11 04:22:42 +00:00

mbiryukova requested review from storage-services-committers 2024-12-11 04:22:42 +00:00

nzinkevich reviewed 2024-12-11 06:48:10 +00:00

api/layer/tombstone.go Outdated

					
				@ -48,3 +58,3 @@

						if err := n.putTombstoneObject(ctx, tomb, bkt); err != nil {

							n.reqLogger(ctx).Warn(logs.FailedToPutTombstoneObject, zap.String("cid", bkt.CID.EncodeToString()), zap.Error(err))

							errCh <- fmt.Errorf("put tombstone object: %w", err)

nzinkevich commented

2024-12-11 06:47:33 +00:00

It seems that we only return the first error from errCh in putTombstones. Should we leave log warnings for the rest?

It seems that we only return the first error from errCh in `putTombstones`. Should we leave log warnings for the rest?

👍 1

mbiryukova commented

2024-12-11 08:29:54 +00:00

I can add logging of all errors from channel in putTombstones

I can add logging of all errors from channel in `putTombstones`

alexvanin commented

2024-12-12 07:10:28 +00:00

I don't mind having logs here or log it all at once in putTombstones. But we should definitely log it.

I don't mind having logs here or log it all at once in `putTombstones`. But we should definitely log it.

alexvanin added this to the v0.32.0 milestone 2024-12-11 07:08:40 +00:00

a-savchuk reviewed 2024-12-11 08:35:28 +00:00

api/layer/layer.go Outdated

					
				@ -785,0 +786,4 @@

					oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, nodeVersion.OID, tokens)

					if err != nil {

						if !client.IsErrObjectAlreadyRemoved(err) && !client.IsErrObjectNotFound(err) {

							return fmt.Errorf("failed to list all object relations '%s': %w", nodeVersion.OID.EncodeToString(), err)

a-savchuk commented

2024-12-11 08:35:28 +00:00

Both container and object IDs are Stringers. There's no need to explicitly call EncodeToString when formatting or logging

Both container and object IDs are `Stringer`s. There's no need to explicitly call `EncodeToString` when formatting or logging

mbiryukova commented

2024-12-11 09:42:33 +00:00

Yes, but in most places it's called, so will leave the same

👍 1

dkirillov reviewed 2024-12-11 08:37:03 +00:00

api/layer/layer.go Outdated

					
				@ -785,0 +783,4 @@

						return fmt.Errorf("put tombstones with parts: %w", err)

					}

					oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, nodeVersion.OID, tokens)

dkirillov commented

2024-12-11 08:37:03 +00:00

Can we use something like this, to simplify a little?

diff --git a/api/layer/layer.go b/api/layer/layer.go
index e764d887..e16eb71b 100644
--- a/api/layer/layer.go
+++ b/api/layer/layer.go
@@ -762,38 +762,28 @@ func (n *Layer) removeCombinedObject(ctx context.Context, bkt *data.BucketInfo,
 		return fmt.Errorf("unmarshal combined object parts: %w", err)
 	}
 
+	ids := make([]oid.ID, 1, len(parts)+1)
+	ids[0] = nodeVersion.OID
+	for i, part := range parts {
+		ids[i+1] = part.OID
+	}
+
 	tokens := prepareTokensParameter(ctx, bkt.Owner)
 	members := make([]oid.ID, 0)
-	for _, part := range parts {
-		oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, part.OID, tokens)
+	for _, id := range ids {
+		oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, id, tokens)
 		if err == nil {
-			members = append(members, append(oids, part.OID)...)
+			members = append(members, append(oids, id)...)
 			continue
 		}
 
 		if !client.IsErrObjectAlreadyRemoved(err) && !client.IsErrObjectNotFound(err) {
-			return fmt.Errorf("failed to list all object relations '%s': %w", part.OID.EncodeToString(), err)
-		}
-
-		n.reqLogger(ctx).Warn(logs.FailedToListAllObjectRelations, zap.String("cid", bkt.CID.EncodeToString()),
-			zap.String("oid", part.OID.EncodeToString()), zap.Error(err))
-	}
-
-	if err = n.putTombstones(ctx, bkt, networkInfo, members); err != nil {
-		return fmt.Errorf("put tombstones with parts: %w", err)
-	}
-
-	oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, nodeVersion.OID, tokens)
-	if err != nil {
-		if !client.IsErrObjectAlreadyRemoved(err) && !client.IsErrObjectNotFound(err) {
-			return fmt.Errorf("failed to list all object relations '%s': %w", nodeVersion.OID.EncodeToString(), err)
+			return fmt.Errorf("failed to list all object relations '%s': %w", id.EncodeToString(), err)
 		}
 
 		n.reqLogger(ctx).Warn(logs.FailedToListAllObjectRelations, zap.String("cid", bkt.CID.EncodeToString()),
-			zap.String("oid", nodeVersion.OID.EncodeToString()), zap.Error(err))
-		return nil
+			zap.String("oid", id.EncodeToString()), zap.Error(err))
 	}
-	members = append(oids, nodeVersion.OID)
 
 	return n.putTombstones(ctx, bkt, networkInfo, members)
 }

Can we use something like this, to simplify a little? ```diff diff --git a/api/layer/layer.go b/api/layer/layer.go index e764d887..e16eb71b 100644 --- a/api/layer/layer.go +++ b/api/layer/layer.go @@ -762,38 +762,28 @@ func (n *Layer) removeCombinedObject(ctx context.Context, bkt *data.BucketInfo, return fmt.Errorf("unmarshal combined object parts: %w", err) } + ids := make([]oid.ID, 1, len(parts)+1) + ids[0] = nodeVersion.OID + for i, part := range parts { + ids[i+1] = part.OID + } + tokens := prepareTokensParameter(ctx, bkt.Owner) members := make([]oid.ID, 0) - for _, part := range parts { - oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, part.OID, tokens) + for _, id := range ids { + oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, id, tokens) if err == nil { - members = append(members, append(oids, part.OID)...) + members = append(members, append(oids, id)...) continue } if !client.IsErrObjectAlreadyRemoved(err) && !client.IsErrObjectNotFound(err) { - return fmt.Errorf("failed to list all object relations '%s': %w", part.OID.EncodeToString(), err) - } - - n.reqLogger(ctx).Warn(logs.FailedToListAllObjectRelations, zap.String("cid", bkt.CID.EncodeToString()), - zap.String("oid", part.OID.EncodeToString()), zap.Error(err)) - } - - if err = n.putTombstones(ctx, bkt, networkInfo, members); err != nil { - return fmt.Errorf("put tombstones with parts: %w", err) - } - - oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, nodeVersion.OID, tokens) - if err != nil { - if !client.IsErrObjectAlreadyRemoved(err) && !client.IsErrObjectNotFound(err) { - return fmt.Errorf("failed to list all object relations '%s': %w", nodeVersion.OID.EncodeToString(), err) + return fmt.Errorf("failed to list all object relations '%s': %w", id.EncodeToString(), err) } n.reqLogger(ctx).Warn(logs.FailedToListAllObjectRelations, zap.String("cid", bkt.CID.EncodeToString()), - zap.String("oid", nodeVersion.OID.EncodeToString()), zap.Error(err)) - return nil + zap.String("oid", id.EncodeToString()), zap.Error(err)) } - members = append(oids, nodeVersion.OID) return n.putTombstones(ctx, bkt, networkInfo, members) } ```

mbiryukova commented

2024-12-11 09:24:53 +00:00

nodeVersion.OID is deleted separately so that if errors occur during removal of parts, combined object is not deleted. It will allow to remove multipart again

`nodeVersion.OID` is deleted separately so that if errors occur during removal of parts, combined object is not deleted. It will allow to remove multipart again

alexvanin commented

2024-12-12 06:51:59 +00:00

We can leave a comment in code about this idea.

// First gateway tries to delete all object parts.
// In case of errors, abort multipart removal.
for _, part := range parts {
...
// If all parts were removed successfully, remove multipart linking object.
// Do not delete this object first, because gateway won't be able to find parts.
oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, nodeVersion.OID, tokens)

We can leave a comment in code about this idea. ```go // First gateway tries to delete all object parts. // In case of errors, abort multipart removal. for _, part := range parts { ... // If all parts were removed successfully, remove multipart linking object. // Do not delete this object first, because gateway won't be able to find parts. oids, err := relations.ListAllRelations(ctx, n.frostFS.Relations(), bkt.CID, nodeVersion.OID, tokens) ```

dkirillov commented

2024-12-12 14:02:00 +00:00

I just meant to do this in one for (or extract duplicated code to function)

I just meant to do this in one `for` (or extract duplicated code to function)

dkirillov marked this conversation as resolved

dkirillov reviewed 2024-12-11 08:40:35 +00:00

api/layer/multipart_upload.go Outdated

					
				@ -586,12 +586,16 @@ func (n *Layer) deleteUploadedParts(ctx context.Context, bkt *data.BucketInfo, p

							if err != nil {

								n.reqLogger(ctx).Warn(logs.FailedToListAllObjectRelations, zap.String("cid", bkt.CID.EncodeToString()),

									zap.String("oid", info.OID.EncodeToString()), zap.Error(err))

								continue

dkirillov commented

2024-12-11 08:40:34 +00:00

Shouldn't we also check error and if it isn't object already removed or object not found then return error?

Shouldn't we also check error and if it isn't `object already removed` or `object not found` then return error?

mbiryukova commented

2024-12-11 09:19:44 +00:00

I did it the way it was before not to change behavior

I did it the way it was [before](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/51322cccdf54e0e7bd4fd3fb0acd5553606498f1/api/layer/multipart_upload.go#L568) not to change behavior

dkirillov marked this conversation as resolved

alexvanin approved these changes 2024-12-12 07:20:03 +00:00

Dismissed

alexvanin left a comment

Overall looks good to me. Let's add some logs and we should be fine.

mbiryukova force-pushed feature/multipart_tombstone from 5b10514bd3 to 6755ae912d

2024-12-12 07:48:32 +00:00

Compare

mbiryukova dismissed alexvanin's review 2024-12-12 07:48:32 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin approved these changes 2024-12-12 10:04:34 +00:00