New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/Unlock objects on deletion lock object #1461

Merged

fyrchik merged 13 commits into nspcc-dev:fix/correct-lock-objects from carpawell:fix/unlock-objects-on-deletion-LOCK-object

Jun 17, 2022

Member

carpawell commented May 31, 2022 •

edited

Loading

Related to #1227.

Lock object could not exist without expiration.
Lock object is impossible to delete.
2nd has an exception: control service.

Why two new options:

Regular removal: WithTombstoneAddress/ WithGCMark.
Removal by control service: WithForceGCMark to be able to delete Lock objects and unlock locked objects.
Removal of tombstones/expired SGs/expired Locks by GC: WithGCMark and WithoutLockObjectHandling (the last is kinda optimization, we do not need to check if an object is a Lock object if we know that is it a TS/SG AND we must NOT check whether it is a Lock if we KNOW that we remove expired lock).

Also, I did not want to remove TS/SG/Expired LOCKS with WithForceGCMark since it is not forced removal in fact.

Question: does the SE need WithForceRemoval option or every StorageEngine.Delete call could be considered as a "force removal"?

carpawell requested review from cthulhu-rider, fyrchik and alexvanin

May 31, 2022 20:26

carpawell mentioned this pull request

Fix: Unlock locked object on its lock removal #1440

Closed

Member Author

carpawell commented Jun 1, 2022

Question[2]: do we need a separate error status for "lock object removal"?

Contributor

KirillovDenis commented Jun 2, 2022 •

edited

Loading

I tested this PR and found out that the removal of the lock object doesn't produce any error (though the lock is still not removed). Is it expected?

Member Author

carpawell commented Jun 6, 2022 •

edited

Loading

Is it expected?

@KirillovDenis, no, fixed. Also, I guess, we will add a separate status code for such cases in another PR.

carpawell marked this pull request as ready for review

June 6, 2022 14:57

fyrchik reviewed

View reviewed changes

pkg/local_object_storage/metabase/inhume.go Outdated Show resolved Hide resolved

pkg/local_object_storage/shard/inhume.go Outdated Show resolved Hide resolved

pkg/local_object_storage/engine/inhume.go Show resolved Hide resolved

cmd/neofs-cli/modules/lock.go Outdated

Comment on lines 50 to 51

		expEpoch, err := cmd.Flags().GetUint64(lockExpiresOnFlag)
		common.ExitOnErr(cmd, "Incorrect expiration epoch: %w", err)

Contributor

fyrchik Jun 7, 2022

We could also support +X format, which means relative to the current epoch.
The examples are here

neofs-node/cmd/neofs-cli/modules/bearer/create.go

Line 144 in 79d72a6

// parseEpoch parses epoch argument. Second return value is true if

Member Author

carpawell Jun 8, 2022

good UX improvement

required to move that funcs to the common pkg and reorganized client pkg a little: d70bc5a

Contributor

fyrchik Jun 15, 2022

ParseEpoch is ok, but moving GetSDKClientByFlag to common is not worth IMO. CAn we move GetCurrentEpoch to some other package? Or just export from modules/bearer package, there is a number of places we do a similar thing.

Member Author

carpawell Jun 15, 2022 •

edited

Loading

IMO, internal/client/sdk.go is smth strange: pkg exists only to hold the wrapper around SDK client and we have somehow added sdk.go file that returns raw SDK client

but ok, moved GetCurrentEpoch to the internal/client/sdk.go since it is a kinda helper that works with SDK client

ParseEpoch moved to the common

GetSDKClientByFlag kept at its previous place

pkg/local_object_storage/metabase/inhume.go Outdated Show resolved Hide resolved

pkg/local_object_storage/shard/lock_test.go Outdated Show resolved Hide resolved

pkg/local_object_storage/engine/lock_test.go Show resolved Hide resolved

fyrchik assigned carpawell

carpawell requested a review from fyrchik

June 8, 2022 10:32

alexvanin mentioned this pull request

Consider unremovable LOCK objects nspcc-dev/neofs-s3-gw#381

Closed

carpawell added 4 commits

June 16, 2022 16:06


          [#1461] node: Unlock locked object on its lock removal

3036c52

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] cli: Reorganize common functions

265b86e

Make `ParseEpoch` and `GetCurrentEpoch` funcs public and move the first one
to the `common` package.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] cli: Require LOCK object to be expired

d54411a

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] node: Do not accept lock object w/o expiration

3fb76ae

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

fyrchik reviewed

View reviewed changes

pkg/local_object_storage/metabase/inhume.go Outdated Show resolved Hide resolved

cmd/neofs-cli/internal/client/sdk.go Outdated Show resolved Hide resolved

pkg/local_object_storage/metabase/inhume.go Outdated Show resolved Hide resolved

fyrchik approved these changes

View reviewed changes

carpawell added 9 commits

June 17, 2022 10:45


          [#1461] node: Allow force LOCK removal

96a1197

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] engine: Fix LOCK test

f811c5d

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] meta: Add lock tests

3ecc9de

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] shard: Add lock tests

bb8cf66

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] shard: Fix option naming

`WitDeletedLockCallback` => `WithDeletedLockCallback`.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] engine: Add force lock removal tests

015da82

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] meta: Return error on lock object removal

f4492d4

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] engine, policer: Force remove objects w/o container

92621ec

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


          [#1461] cli: Add context to GetCurrentEpoch helper

5105b0f

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

fyrchik merged commit 398eb80 into nspcc-dev:fix/correct-lock-objects

carpawell deleted the fix/unlock-objects-on-deletion-LOCK-object branch

June 19, 2022 12:40

carpawell added a commit that referenced this pull request


          [#1461] node: Unlock locked object on its lock removal

501c05e

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] cli: Require LOCK object to be expired

ce8b4da

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] node: Do not accept lock object w/o expiration

87c03ad

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] node: Allow force LOCK removal

8f76861

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] engine: Fix LOCK test

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] meta: Add lock tests

be990e2

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] shard: Add lock tests

0eba6db

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] shard: Fix option naming

2ba7cb7

`WitDeletedLockCallback` => `WithDeletedLockCallback`.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] engine: Add force lock removal tests

4203a83

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] meta: Return error on lock object removal

da4bd43

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] engine, policer: Force remove objects w/o container

1322b41

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] cli: Add context to GetCurrentEpoch helper

8015b2b

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] engine: Clarify force removal

b9fcd4d

Document force removal behaviour in all the Storage engine parts.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit that referenced this pull request


          [#1461] shard: Add a separate ErrLockObjectRemoval

c10231a

Do not return `meta.ErrLockObjectRemoval` from shard's methods, add shard's
own error for that.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] node: Unlock locked object on its lock removal

c5792a0

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] cli: Reorganize common functions

e1747de

Make `ParseEpoch` and `GetCurrentEpoch` funcs public and move the first one
to the `common` package.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] cli: Require LOCK object to be expired

95dca87

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] node: Do not accept lock object w/o expiration

0cf3fee

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] node: Allow force LOCK removal

a89dca4

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] engine: Fix LOCK test

24dda02

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] meta: Add lock tests

797cbdf

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] shard: Add lock tests

36dfa8b

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] shard: Fix option naming

cd3f3f5

`WitDeletedLockCallback` => `WithDeletedLockCallback`.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] engine: Add force lock removal tests

85c3166

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] meta: Return error on lock object removal

fda4525

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] engine, policer: Force remove objects w/o container

875d326

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] cli: Add context to GetCurrentEpoch helper

ee51ad6

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] engine: Clarify force removal

7fd881d

Document force removal behaviour in all the Storage engine parts.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request


          [nspcc-dev#1461] shard: Add a separate ErrLockObjectRemoval

812746a

Do not return `meta.ErrLockObjectRemoval` from shard's methods, add shard's
own error for that.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

roman-khimov mentioned this pull request

Lock object tests nspcc-dev/neofs-testcases#550

Closed

cthulhu-rider mentioned this pull request

Fix/drop control call #2873

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet