client: SetCopiesNumber is not backward compatible #93

New issue

Closed

opened 2023-06-12 20:27:53 +00:00 by alexvanin · 3 comments

alexvanin commented

2023-06-12 20:27:53 +00:00

Owner

Client package defines SetCopiesNumber function on object put initial parameter. This function should be deprecated in favor of SetCopiesNumberByVector but its still there to keep compatibility. However this function does not provide compatibility when it is used with zero value.

Expected Behavior

Calling SetCopiesNumber with zero value enforces storage to store all object copies before sending a reply.

Current Behavior

Calling SetCopiesNumber with zero values enforces storage to not store object at all.

Possible Solution

With protocol update nil value of placement vector works the same as zero value before, so the code can look like this

func (x *PrmObjectPutInit) SetCopiesNumber(copiesNumber uint32) {
	if copiesNumber == 0 {
		x.copyNum = nil
	} else {
		x.copyNum = []uint32{copiesNumber}
	}
}

Steps to Reproduce (for bugs)

Create a new public container.
Run the test below with created container ID.

func TestUploadObjectWithZeroCopies(t *testing.T) {
	cnrIDValue := "FWTtDc15AA3ZH5e1jjweL5F2keK5pjQy7iks5q5CU1iP"
	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)

	var cnrID cid.ID
	err = cnrID.DecodeString(cnrIDValue)
	require.NoError(t, err)

	// Upload object.
	var prmObjectInit client.PrmObjectPutInit
	prmObjectInit.SetCopiesNumber(0)
	writer, err := cli.ObjectPutInit(ctx, prmObjectInit)
	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(cnrID)
	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()))

	// Head upload object.
	var prmObjectHead client.PrmObjectHead
	prmObjectHead.ByID(objPut.StoredObjectID())
	prmObjectHead.FromContainer(cnrID)

	hdr, err := cli.ObjectHead(ctx, prmObjectHead)
	require.NoError(t, err)
	require.True(t, apistatus.IsSuccessful(hdr.Status()))
}

Context

This issue was found when I tried to use newer version of storage nodes with software that imports SDK with version 237b90f744f3.

Regression

Regression introduced in #46

Your Environment

dev-env with the storage node version: g5b75432c

/cc @TrueCloudLab/storage-core-committers @TrueCloudLab/storage-core-developers

Client package defines `SetCopiesNumber` function on object put initial parameter. This function should be deprecated in favor of `SetCopiesNumberByVector` but its still there to keep compatibility. However this function does not provide compatibility when it is used with zero value. ## Expected Behavior Calling `SetCopiesNumber` with zero value enforces storage to store all object copies before sending a reply. ## Current Behavior Calling `SetCopiesNumber` with zero values enforces storage to not store object at all. ## Possible Solution With protocol update `nil` value of placement vector works the same as zero value before, so the code can look like this ```go func (x *PrmObjectPutInit) SetCopiesNumber(copiesNumber uint32) { if copiesNumber == 0 { x.copyNum = nil } else { x.copyNum = []uint32{copiesNumber} } } ``` ## Steps to Reproduce (for bugs) 1. Create a new public container. 2. Run the test below with created container ID. ```go func TestUploadObjectWithZeroCopies(t *testing.T) { cnrIDValue := "FWTtDc15AA3ZH5e1jjweL5F2keK5pjQy7iks5q5CU1iP" 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) var cnrID cid.ID err = cnrID.DecodeString(cnrIDValue) require.NoError(t, err) // Upload object. var prmObjectInit client.PrmObjectPutInit prmObjectInit.SetCopiesNumber(0) writer, err := cli.ObjectPutInit(ctx, prmObjectInit) 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(cnrID) 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())) // Head upload object. var prmObjectHead client.PrmObjectHead prmObjectHead.ByID(objPut.StoredObjectID()) prmObjectHead.FromContainer(cnrID) hdr, err := cli.ObjectHead(ctx, prmObjectHead) require.NoError(t, err) require.True(t, apistatus.IsSuccessful(hdr.Status())) } ``` ## Context This issue was found when I tried to use newer version of storage nodes with software that imports SDK with version `237b90f744f3`. ## Regression Regression introduced in #46 ## Your Environment - dev-env with the storage node version: `g5b75432c` /cc @TrueCloudLab/storage-core-committers @TrueCloudLab/storage-core-developers

alexvanin added the

bug

label 2023-06-12 20:27:53 +00:00

alexvanin commented

2023-06-12 20:29:30 +00:00

Author

Owner

Also, should we mark SetCopiesNumber as deprecated? I don't think this function should be supported in future versions of SDK.

Also, should we mark `SetCopiesNumber` as deprecated? I don't think this function should be supported in future versions of SDK.

dkirillov commented

2023-06-13 06:31:06 +00:00

Member

Why don't we fix node rather than SDK?

As I remeber we decided that one value in vector should behave as previous. So set 0 (using SetCopiesNumber) or []int{0} (using SetCopiesNumberByVector) should pass []int{0} to the node which will handle this case properly.

Why don't we fix [node](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/221/files) rather than SDK? As I remeber we decided that one value in vector should behave as previous. So set `0` (using `SetCopiesNumber`) or `[]int{0}` (using `SetCopiesNumberByVector`) should pass `[]int{0}` to the node which will handle this case properly.

alexvanin referenced this issue from TrueCloudLab/frostfs-node

2023-06-27 07:51:01 +00:00

Node does not store object with `[0]` copies numbers #473

alexvanin commented

2023-06-27 11:13:47 +00:00

Author

Owner

Done in TrueCloudLab/frostfs-node#474

Done in https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/474

alexvanin closed this issue

2023-06-27 11:13:47 +00:00