Fix PutSingle with OID that was already removed #1579

aarifullin · 2024-12-23T12:03:08Z

aarifullin commented

2024-12-23 12:03:08 +00:00

Make raw PutSingle check status within response
Make metabase if EC-chunk's parent is already removed in fact;
EC-writer should check ObjectAlreadyRemoved errors

* Make raw `PutSingle` check status within response * Make metabase if EC-chunk's parent is already removed in fact; * EC-writer should check `ObjectAlreadyRemoved` errors Close #1512

aarifullin added the

bug

label 2024-12-23 12:03:08 +00:00

aarifullin added 2 commits 2024-12-23 12:03:08 +00:00

[#1512 ] object: Make raw PutSingle check status within response 7c8fa76636

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

[#1512 ] object: Fix put already removed object in EC-container

Tests and linters / Run gofumpt (pull_request) Successful in 3m27s

Details

DCO action / DCO (pull_request) Successful in 4m0s

Details

Vulncheck / Vulncheck (pull_request) Successful in 4m37s

Details

Tests and linters / Staticcheck (pull_request) Successful in 6m1s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 6m23s

Details

Tests and linters / Lint (pull_request) Successful in 6m28s

Details

Tests and linters / gopls check (pull_request) Successful in 6m36s

Details

Build / Build Components (pull_request) Successful in 6m57s

Details

Tests and linters / Tests (pull_request) Successful in 7m1s

Details

Tests and linters / Tests with -race (pull_request) Successful in 7m1s

Details

6347459a39

* Make metabase if EC-chunk's parent is already removed in fact;
* EC-writer should check `ObjectAlreadyRemoved` errors

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

aarifullin requested review from storage-core-committers 2024-12-23 12:03:09 +00:00

aarifullin requested review from storage-core-developers 2024-12-23 12:03:09 +00:00

fyrchik requested changes 2024-12-23 12:13:10 +00:00

Dismissed

pkg/local_object_storage/metabase/put.go Outdated

					
				@ -123,0 +128,4 @@

						pObj.SetContainerID(cnr)

						pObj.SetID(ech.Parent())

						if _, _, err := db.exists(tx, objectCore.AddressOf(pObj), oid.Address{}, currEpoch); err != nil && clientSDK.IsErrObjectAlreadyRemoved(err) {

fyrchik commented

2024-12-23 12:11:34 +00:00

I would argue that this check should replace the check at line 138, because tombstones are created only for parent for EC objects.
And we always know object kind, because we do not create tombstone marks if we have no info in the metabase.

I would argue that this check should replace the check at line 138, because tombstones are created only for parent for EC objects. And we always know object kind, because we do not create tombstone marks if we have no info in the metabase.

aarifullin commented

2024-12-23 13:11:13 +00:00

tombstones are created only for parent for EC objects.

That's why the change has been introduced.
ecWriter is chunking an EC-encoded object and sends these parts to Engine through Metabase. Meanwhile the only object that's marked as "removed" - the EC-parent.
So, db.exists(tx, objectCore.AddressOf(obj), oid.Address{}, currEpoch) is not able to catch this because we're checking chunk - graveyard doesn't contain info about chunks

Do you mean this check should be part of exists?

> tombstones are created only for parent for EC objects. That's why the change has been introduced. `ecWriter` is chunking an EC-encoded object and sends these parts to Engine through Metabase. Meanwhile the only object that's marked as "removed" - the EC-parent. So, `db.exists(tx, objectCore.AddressOf(obj), oid.Address{}, currEpoch)` is not able to catch this because we're checking chunk - graveyard doesn't contain info about chunks Do you mean this check should be part of `exists`?

fyrchik commented

2024-12-24 07:17:09 +00:00

I mean I you have checked exists(parent), you do not need to check exists(chunk) -- because you already know that it is a chunk and, as you wrote, graveyard doesn't contain info about chunks.

I mean I you have checked `exists(parent)`, you do not need to check `exists(chunk)` -- because you already know that it is a chunk and, as you wrote, `graveyard doesn't contain info about chunks`.

aarifullin commented

2024-12-24 18:33:16 +00:00

The changes in metabase are no longer relevant because @dstepanov-yadro has already found the problem with EC-objects and fixed it within exists (whilst I tried to fix that out of exists). Nevertheless, I made writePart check for ObjectAlreadyRemoved error to stop writing process and return explicit error to client

The changes in metabase are no longer relevant because @dstepanov-yadro has already found the problem with EC-objects and fixed it within `exists` (whilst I tried to fix that out of `exists`). Nevertheless, I made `writePart` check for `ObjectAlreadyRemoved` error to stop writing process and return explicit error to client

fyrchik marked this conversation as resolved

pkg/local_object_storage/metabase/put.go Outdated

					
				@ -123,0 +129,4 @@

						pObj.SetID(ech.Parent())

						if _, _, err := db.exists(tx, objectCore.AddressOf(pObj), oid.Address{}, currEpoch); err != nil && clientSDK.IsErrObjectAlreadyRemoved(err) {

							return PutRes{}, err

fyrchik commented

2024-12-23 12:11:59 +00:00

And if err != nil, but is NOT ErrObjectAlreadyRemoved we continue?

And if `err != nil`, but is NOT `ErrObjectAlreadyRemoved` we continue?

aarifullin commented

2024-12-23 13:23:30 +00:00

It looks really tricky, but any non-ObjectAlreadyRemoved-error is delegated to chunk processing further (as it is below) to not break the Put logic.
The alternative way - to introduce a separate bucket for EC-chunks in metabase. Literally, EC-"orphans" bucket and check a chunk in db.exists

It looks really tricky, but any non-`ObjectAlreadyRemoved`-error is delegated to chunk processing further (as it is below) to _not_ break the `Put` logic. The alternative way - to introduce a separate bucket for `EC`-chunks in metabase. Literally, EC-"orphans" bucket and check a chunk in `db.exists`

fyrchik commented

2024-12-24 07:14:37 +00:00

It can be a storage error, we cannot just ignore it.

aarifullin commented

2024-12-24 18:33:37 +00:00

#1579 (comment)

https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1579#issuecomment-63153

fyrchik marked this conversation as resolved

pkg/services/object/put/single.go Outdated

					
				@ -353,6 +354,9 @@ func (s *Service) redirectPutSingleRequest(ctx context.Context,

							err = fmt.Errorf("response verification failed: %w", err)

						}

						st := apistatus.FromStatusV2(resp.GetMetaHeader().GetStatus())

fyrchik commented

2024-12-23 12:12:59 +00:00

Could you point me at an example of where we already use similar logic?
These lines seem out of place to me.

Could you point me at an example of where we already use similar logic? These lines seem out of place to me.

aarifullin commented

2024-12-23 13:04:28 +00:00

https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/client/object_put_single.go#L162-L174
https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/client/common.go#L100

https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/client/object_put_single.go#L162-L174 https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/client/common.go#L100

fyrchik commented

2024-12-24 07:16:02 +00:00

Both links are from the SDK, I mean in the node.
This line is rather generic, I do not understand why PutSingle is special.

Both links are from the SDK, I mean in the node. This line is rather generic, I do not understand why `PutSingle` is special.

aarifullin commented

2024-12-24 07:43:11 +00:00

headRequest -> verifyResponse

[headRequest](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/get/v2/head_forwarder.go#L57) -> [verifyResponse](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/get/v2/util.go#L419-L429)

fyrchik marked this conversation as resolved

aarifullin force-pushed fix/put_single from 6347459a39 to efaa45216c

2024-12-24 18:21:03 +00:00

Compare

aarifullin force-pushed fix/put_single from efaa45216c to fe1d365e8f

2024-12-24 18:28:48 +00:00

Compare

dstepanov-yadro reviewed 2024-12-25 06:17:53 +00:00

pkg/services/object/put/single.go

					
				@ -354,2 +355,4 @@

						}

						st := apistatus.FromStatusV2(resp.GetMetaHeader().GetStatus())

						err = apistatus.ErrFromStatus(st)

dstepanov-yadro commented

2024-12-25 06:17:52 +00:00

		err = signature.VerifyServiceMessage(resp)
		if err != nil {
			err = fmt.Errorf("response verification failed: %w", err)
			return     <----------------------- add this
		}

		st := apistatus.FromStatusV2(resp.GetMetaHeader().GetStatus())
		err = apistatus.ErrFromStatus(st)

``` err = signature.VerifyServiceMessage(resp) if err != nil { err = fmt.Errorf("response verification failed: %w", err) return <----------------------- add this } st := apistatus.FromStatusV2(resp.GetMetaHeader().GetStatus()) err = apistatus.ErrFromStatus(st) ```

aarifullin commented

2024-12-25 08:23:54 +00:00

Correct! Fixed