Unexpected object delete response code

alexvanin commented

2023-04-03 06:52:17 +00:00

Owner

Expected Behavior

When I try to delete already removed object, I receive OK according to specification: fc393d4605/object/service.proto (L99-L109)

Current Behavior

When I try to delete already removed object, I receive code = 2052 message = object already removed, which is unexpected because it isn't mentioned in the spec.

Possible Solution

Update the spec and move this issue in the frostfs-api repository, or update the code to return OK on delete operation if object is already removed.

Steps to Reproduce (for bugs)

Create container
Upload the object
Delete upload object
Delete upload object once again

Context

This issue affects error checks in S3 gateway when it updates object listing cache after object service operation: TrueCloudLab/frostfs-s3-gw#71

Your Environment

FrostFS DevEnv
Node version: ed28ce24

## Expected Behavior When I try to delete already removed object, I receive OK according to specification: https://git.frostfs.info/TrueCloudLab/frostfs-api/src/commit/fc393d4605e7e4c5545e882ef20d5dc171e25ce8/object/service.proto#L99-L109 ## Current Behavior When I try to delete already removed object, I receive `code = 2052 message = object already removed`, which is unexpected because it isn't mentioned in the spec. ## Possible Solution Update the spec and move this issue in the frostfs-api repository, or update the code to return OK on delete operation if object is already removed. ## Steps to Reproduce (for bugs) 1. Create container 2. Upload the object 3. Delete upload object 4. Delete upload object once again ## Context This issue affects error checks in S3 gateway when it updates object listing cache after object service operation: https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/71 ## Your Environment FrostFS DevEnv Node version: ed28ce24

alexvanin added the

triage

label 2023-04-03 06:52:17 +00:00

fyrchik was assigned by alexvanin

2023-04-03 06:52:17 +00:00

alexvanin referenced this issue from TrueCloudLab/frostfs-s3-gw

2023-04-03 06:54:29 +00:00

[#57] api: Clean up List and Name caches when object is missing in Tree service #71

fyrchik added this to the v0.37.0 milestone 2023-04-13 08:29:11 +00:00

fyrchik added the

discussion

label 2023-05-18 10:52:04 +00:00

aarifullin was assigned by fyrchik

2023-05-29 12:58:09 +00:00

fyrchik removed their assignment 2023-05-29 12:58:13 +00:00

aarifullin commented

2023-05-30 13:39:37 +00:00

Collaborator

@alexvanin, can you clarify how do you invoke removal of the object?

I do like that:

frostfs-cli object delete --cid 6RBcZcNi6onhNvqAKSgL9amabo15xrMrZ28U3X6qM7gV --oid CvAUxAitTscQD92Qk4xzd4pvk9A7ok1Vn7gFfQfbyq78 --wallet <some-wallet-path> --rpc-endpoint 's02.frostfs.devenv:8080'

I get the same message:

failed to get raw object header: read object header via client: status: code = 2052 message = object already removed

But delete command implicitly invokes Head remote procedure:

ReadOrOpenSession invokes OpenSessionViaClient that invokes collectObjectRelatives that invokes Head

And Head has another spec and here is no contradiction.

Maybe we should make the error more verbose?

@alexvanin, can you clarify how do you invoke removal of the object? I do like that: ``` frostfs-cli object delete --cid 6RBcZcNi6onhNvqAKSgL9amabo15xrMrZ28U3X6qM7gV --oid CvAUxAitTscQD92Qk4xzd4pvk9A7ok1Vn7gFfQfbyq78 --wallet <some-wallet-path> --rpc-endpoint 's02.frostfs.devenv:8080' ``` I get the same message: ``` failed to get raw object header: read object header via client: status: code = 2052 message = object already removed ``` But `delete` command implicitly invokes `Head` remote procedure: [ReadOrOpenSession](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-cli/modules/object/delete.go#L64) invokes [OpenSessionViaClient](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-cli/modules/object/util.go#L258) that invokes [collectObjectRelatives](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-cli/modules/object/util.go#L342) that invokes [Head](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-cli/modules/object/util.go#L357) And `Head` has another [spec](https://git.frostfs.info/TrueCloudLab/frostfs-api/src/commit/fc393d4605e7e4c5545e882ef20d5dc171e25ce8/object/service.proto#L132) and here is no contradiction. Maybe we should make the error more verbose?

alexvanin commented

2023-05-31 10:09:49 +00:00

Poster

Owner

@aarifullin Try to use SDK client to send direct Delete request without having extra Head requests. I get object already removed here.

The test below creates a SDK client, new FrostFS container, signs and upload the object without payload and then calls delete twice. The second attempt returns 2052 code.

func TestRemoveObjectTwice(t *testing.T) {
	ctx := context.Background()
	k, err := keys.NewPrivateKeyFromWIF("L32sMMkUcEfsWFdjoFNKAPtj7y6cfkcpHwG2pmTap68AmUt86ZTo")
	require.NoError(t, err)

	var owner user.ID
	user.IDFromKey(&owner, k.PrivateKey.PublicKey)

	// Create SDK Client.
	var prmInit client.PrmInit
	prmInit.SetDefaultPrivateKey(k.PrivateKey)

	var prmDial client.PrmDial
	prmDial.SetServerURI("s01.frostfs.devenv:8080")

	var cli client.Client
	cli.Init(prmInit)
	err = cli.Dial(ctx, prmDial)
	require.NoError(t, err)

	// Create container.
	var pp netmap.PlacementPolicy
	err = pp.DecodeString("REP 1")
	require.NoError(t, err)

	var cnr container.Container
	cnr.Init()
	cnr.SetOwner(owner)
	cnr.SetBasicACL(acl.PublicRWExtended)
	cnr.SetPlacementPolicy(pp)

	var prmContainerPut client.PrmContainerPut
	prmContainerPut.SetContainer(cnr)
	cnrPut, err := cli.ContainerPut(ctx, prmContainerPut)
	require.NoError(t, err)

	// Wait for container to persist.
	var ok bool
	for i := 0; i < 100; i++ {
		time.Sleep(200 * time.Millisecond)
		var prmContainerGet client.PrmContainerGet
		prmContainerGet.SetContainer(cnrPut.ID())
		cnrGet, err := cli.ContainerGet(ctx, prmContainerGet)
		require.NoError(t, err)
		if apistatus.IsSuccessful(cnrGet.Status()) {
			ok = true
			break
		}
	}
	require.True(t, ok)

	// Upload object.
	writer, err := cli.ObjectPutInit(ctx, client.PrmObjectPutInit{})
	require.NoError(t, err)

	var sha checksum.Checksum
	checksum.Calculate(&sha, checksum.SHA256, []byte{})

	var tz checksum.Checksum
	checksum.Calculate(&sha, checksum.TZ, []byte{})

	var header object.Object
	header.SetContainerID(cnrPut.ID())
	header.SetOwnerID(&owner)
	header.SetPayloadChecksum(sha)
	header.SetPayloadHomomorphicHash(tz)

	err = object.CalculateAndSetID(&header)
	require.NoError(t, err)
	err = object.CalculateAndSetSignature(k.PrivateKey, &header)
	require.NoError(t, err)

	ok = writer.WriteHeader(header)
	require.True(t, ok)

	objPut, err := writer.Close()
	require.NoError(t, err)
	require.True(t, apistatus.IsSuccessful(objPut.Status()))

	// Delete object.
	var prmObjectDelete client.PrmObjectDelete
	prmObjectDelete.ByID(objPut.StoredObjectID())
	prmObjectDelete.FromContainer(cnrPut.ID())

	del, err := cli.ObjectDelete(ctx, prmObjectDelete)
	require.NoError(t, err)
	require.True(t, apistatus.IsSuccessful(del.Status()))

	// Delete the same object once again.
	del, err = cli.ObjectDelete(ctx, prmObjectDelete)
	require.NoError(t, err)
	fmt.Println(del.Status()) // 2052 object already removed
	require.True(t, apistatus.IsSuccessful(del.Status()))
}

@aarifullin Try to use SDK client to send direct `Delete` request without having extra `Head` requests. I get `object already removed` here. The test below creates a SDK client, new FrostFS container, signs and upload the object without payload and then calls delete twice. The second attempt returns 2052 code. ```go func TestRemoveObjectTwice(t *testing.T) { ctx := context.Background() k, err := keys.NewPrivateKeyFromWIF("L32sMMkUcEfsWFdjoFNKAPtj7y6cfkcpHwG2pmTap68AmUt86ZTo") require.NoError(t, err) var owner user.ID user.IDFromKey(&owner, k.PrivateKey.PublicKey) // Create SDK Client. var prmInit client.PrmInit prmInit.SetDefaultPrivateKey(k.PrivateKey) var prmDial client.PrmDial prmDial.SetServerURI("s01.frostfs.devenv:8080") var cli client.Client cli.Init(prmInit) err = cli.Dial(ctx, prmDial) require.NoError(t, err) // Create container. var pp netmap.PlacementPolicy err = pp.DecodeString("REP 1") require.NoError(t, err) var cnr container.Container cnr.Init() cnr.SetOwner(owner) cnr.SetBasicACL(acl.PublicRWExtended) cnr.SetPlacementPolicy(pp) var prmContainerPut client.PrmContainerPut prmContainerPut.SetContainer(cnr) cnrPut, err := cli.ContainerPut(ctx, prmContainerPut) require.NoError(t, err) // Wait for container to persist. var ok bool for i := 0; i < 100; i++ { time.Sleep(200 * time.Millisecond) var prmContainerGet client.PrmContainerGet prmContainerGet.SetContainer(cnrPut.ID()) cnrGet, err := cli.ContainerGet(ctx, prmContainerGet) require.NoError(t, err) if apistatus.IsSuccessful(cnrGet.Status()) { ok = true break } } require.True(t, ok) // Upload object. writer, err := cli.ObjectPutInit(ctx, client.PrmObjectPutInit{}) require.NoError(t, err) var sha checksum.Checksum checksum.Calculate(&sha, checksum.SHA256, []byte{}) var tz checksum.Checksum checksum.Calculate(&sha, checksum.TZ, []byte{}) var header object.Object header.SetContainerID(cnrPut.ID()) header.SetOwnerID(&owner) header.SetPayloadChecksum(sha) header.SetPayloadHomomorphicHash(tz) err = object.CalculateAndSetID(&header) require.NoError(t, err) err = object.CalculateAndSetSignature(k.PrivateKey, &header) require.NoError(t, err) ok = writer.WriteHeader(header) require.True(t, ok) objPut, err := writer.Close() require.NoError(t, err) require.True(t, apistatus.IsSuccessful(objPut.Status())) // Delete object. var prmObjectDelete client.PrmObjectDelete prmObjectDelete.ByID(objPut.StoredObjectID()) prmObjectDelete.FromContainer(cnrPut.ID()) del, err := cli.ObjectDelete(ctx, prmObjectDelete) require.NoError(t, err) require.True(t, apistatus.IsSuccessful(del.Status())) // Delete the same object once again. del, err = cli.ObjectDelete(ctx, prmObjectDelete) require.NoError(t, err) fmt.Println(del.Status()) // 2052 object already removed require.True(t, apistatus.IsSuccessful(del.Status())) } ```

👍 1

aarifullin commented

2023-06-01 09:26:29 +00:00

Collaborator

@alexvanin, first of all, thank you for the written unittest :)

Let me omit some details and describe the situtaion briefly:

When client invokes ObjectDelete this causes handler invocation from the deleteSrv side:

object/delete/v2/service.go -> services/object/delete/delete.go -> object/delete/local.go -> formSplitInfo -> ... -> Head

So, again, we send Head request to the serivce :)

Let's discuss would be it correct to process the error here to support delete spec?

@alexvanin, first of all, thank you for the written unittest :) Let me omit some details and describe the situtaion briefly: When client invokes `ObjectDelete` this causes handler invocation from the `deleteSrv` side: [object/delete/v2/service.go](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/delete/v2/service.go#L36) -> [services/object/delete/delete.go](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/delete/delete.go#L12) -> [object/delete/local.go](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/delete/local.go#L15) -> [formSplitInfo](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/delete/local.go#L46) -> ... -> [Head](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/delete/util.go#L42) So, again, we send `Head` request to the serivce :) Let's discuss would be it correct to process the error [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/delete/local.go#L46) to support delete spec?

alexvanin commented

2023-06-06 07:58:16 +00:00

Poster

Owner

@aarifullin Oh, I get it now, thanks.

Personally I think node should follow a specification there and process object already removed error in formSpitInfo(). This function is used only in delete handler, so it should be ok to update it. Something like

	switch {
	default:
		exec.status = statusUndefined
		exec.err = err

		exec.log.Debug(logs.DeleteCouldNotComposeSplitInfo,
			zap.String("error", err.Error()),
		)
	case err == nil, client.IsErrObjectAlreadyRemoved(err):
		exec.status = statusOK
		exec.err = nil
	
	}

/cc @TrueCloudLab/storage-core-committers

@aarifullin Oh, I get it now, thanks. Personally I think node should follow a specification there and process `object already removed` error in `formSpitInfo()`. This function is used only in delete handler, so it should be ok to update it. Something like ```go switch { default: exec.status = statusUndefined exec.err = err exec.log.Debug(logs.DeleteCouldNotComposeSplitInfo, zap.String("error", err.Error()), ) case err == nil, client.IsErrObjectAlreadyRemoved(err): exec.status = statusOK exec.err = nil } ``` /cc @TrueCloudLab/storage-core-committers

aarifullin referenced this issue from TrueCloudLab/frostfs-api

2023-06-07 10:09:05 +00:00

[#28] object: Fix Delete rpc specification #28

aarifullin referenced this issue

2023-06-07 10:10:13 +00:00

[#197] object: Make Delete method return correct status #433

aarifullin commented

2023-06-07 10:14:11 +00:00

Collaborator

@aarifullin Oh, I get it now, thanks.

Personally I think node should follow a specification there and process object already removed error in formSpitInfo(). This function is used only in delete handler, so it should be ok to update it. Something like
	switch {
	default:
		exec.status = statusUndefined
		exec.err = err

		exec.log.Debug(logs.DeleteCouldNotComposeSplitInfo,
			zap.String("error", err.Error()),
		)
	case err == nil, client.IsErrObjectAlreadyRemoved(err):
		exec.status = statusOK
		exec.err = nil
	
	}
/cc @TrueCloudLab/storage-core-committers

Okay. I agreed with that and with this PR the status is correct but still as I mentioned here the problem is also with CLI because of the Head request (collectObjectRelatives is within cli and isn't related to splitInfo). Do we agree with this behavior? Or we put the same processing?

> @aarifullin Oh, I get it now, thanks. > > Personally I think node should follow a specification there and process `object already removed` error in `formSpitInfo()`. This function is used only in delete handler, so it should be ok to update it. Something like > > ```go > switch { > default: > exec.status = statusUndefined > exec.err = err > > exec.log.Debug(logs.DeleteCouldNotComposeSplitInfo, > zap.String("error", err.Error()), > ) > case err == nil, client.IsErrObjectAlreadyRemoved(err): > exec.status = statusOK > exec.err = nil > > } > ``` > > /cc @TrueCloudLab/storage-core-committers Okay. I agreed with that and with this [PR](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/433) the status is correct but still as I mentioned [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/197#issuecomment-11500) the problem is also with CLI because of the Head request (collectObjectRelatives is within cli and isn't related to splitInfo). Do we agree with this behavior? Or we put the same processing?

aarifullin referenced this issue

2023-06-13 08:29:15 +00:00

[#197] object: Make Delete method return correct status #433

fyrchik referenced this issue from a commit

2023-06-30 12:58:59 +00:00

[#197] object: Make Delete method return correct status

aarifullin closed this issue

2023-07-31 09:56:52 +00:00

Unexpected object delete response code #197

Expected Behavior

Current Behavior

Possible Solution

Steps to Reproduce (for bugs)

Context

Your Environment