local_object_storage: Guarantee graves removal when handling expired tombstones #1481

Open
a-savchuk wants to merge 7 commits from a-savchuk/frostfs-node:remove-dangling-locks into master
Member

#1445

To summarize,

  • Add expiration epoch to records in graveyard bucket (let's call these records graves)
    • Fix existing tests because now inhume operation requires expiration epoch
    • Add a test: iterate graveyard which is populated with graves of both old and new format, i. e. without and with expiration epoch, respectively
  • Change the GC behavior
    • Change naming to better reflect the GC behavior
    • Remove expired objects of all types in one worker, before objects of different types were handled in different go-routines
    • Handle graves with expiration epoch separately from others: remove them without searching for the related tombstone
    • Add test:
      • GC handle graves without and with expiration epoch

I've described the old and new behavior of GC in #1445. Feel free to ask any questions and argue.

#1445 To summarize, - Add expiration epoch to records in graveyard bucket (let's call these records _graves_) - Fix existing tests because now inhume operation requires expiration epoch - Add a test: iterate graveyard which is populated with graves of both old and new format, i. e. without and with expiration epoch, respectively - Change the GC behavior - Change naming to better reflect the GC behavior - Remove expired objects of all types in one worker, before objects of different types were handled in different go-routines - Handle graves with expiration epoch separately from others: remove them without searching for the related tombstone - Add test: - GC handle graves without and with expiration epoch I've described the old and new behavior of GC in #1445. **Feel free to ask any questions and argue.**
a-savchuk force-pushed remove-dangling-locks from 0013294c1b to 93c9ec3256 2024-11-08 12:23:49 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 93c9ec3256 to 45cbd497cd 2024-11-10 21:16:47 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 45cbd497cd to 6738de7df7 2024-11-11 09:35:48 +00:00 Compare
acid-ant reviewed 2024-11-12 08:05:39 +00:00
@ -139,0 +165,4 @@
})
}
func (e *StorageEngine) dropOrphanLocks(ctx context.Context, cnt cid.ID, locks []oid.ID) (locksToKeep []oid.ID) {
Member

Why it is named drop when it is used to find IDs?

Why it is named `drop` when it is used to find IDs?
Author
Member

I've changed the solution drastically. Please see the task and PR description for more information

I've changed the solution drastically. Please see the task and PR description for more information
a-savchuk force-pushed remove-dangling-locks from 6738de7df7 to be8b99e3bc 2024-12-15 23:50:41 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from be8b99e3bc to 163c3dba71 2024-12-16 22:19:35 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 163c3dba71 to 34df21cc4a 2024-12-17 07:38:09 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 34df21cc4a to e2a6663ef6 2024-12-17 10:52:15 +00:00 Compare
a-savchuk added 3 commits 2024-12-18 20:48:27 +00:00
Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>
Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>
[#1445] shard/gc: Remove graves
Some checks failed
Vulncheck / Vulncheck (pull_request) Successful in 3m6s
Pre-commit hooks / Pre-commit (pull_request) Successful in 3m35s
Tests and linters / gopls check (pull_request) Successful in 4m0s
Tests and linters / Run gofumpt (pull_request) Successful in 6m26s
DCO action / DCO (pull_request) Successful in 6m59s
Build / Build Components (pull_request) Successful in 7m56s
Tests and linters / Staticcheck (pull_request) Successful in 8m0s
Tests and linters / Lint (pull_request) Successful in 9m14s
Tests and linters / Tests with -race (pull_request) Failing after 12m32s
Tests and linters / Tests (pull_request) Failing after 16m43s
42681223d0
Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>
a-savchuk force-pushed remove-dangling-locks from 42681223d0 to e2d26cd213 2024-12-19 20:47:18 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from e2d26cd213 to 0092a49f8f 2024-12-20 08:23:48 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 0092a49f8f to de1ef66a21 2024-12-20 09:33:31 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from de1ef66a21 to ccdcd67ec3 2024-12-20 09:35:01 +00:00 Compare
a-savchuk changed title from WIP: policer: Handle oprhan locked bucket and graveyard bucket records to WIP: local_object_storage: Guarantee consistency when handling expired tombstones and lock objects 2024-12-20 09:38:23 +00:00
a-savchuk changed title from WIP: local_object_storage: Guarantee consistency when handling expired tombstones and lock objects to local_object_storage: Guarantee consistency when handling expired tombstones and lock objects 2024-12-20 11:45:03 +00:00
requested reviews from storage-core-committers, storage-core-developers 2024-12-20 11:45:04 +00:00
a-savchuk force-pushed remove-dangling-locks from ccdcd67ec3 to 220dc8af30 2024-12-23 12:57:22 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 220dc8af30 to 5b43f2a1f0 2024-12-24 09:24:51 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 5b43f2a1f0 to 4c73f1be9c 2024-12-24 10:01:29 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 4c73f1be9c to e12c763de6 2024-12-24 10:10:17 +00:00 Compare
a-savchuk changed title from local_object_storage: Guarantee consistency when handling expired tombstones and lock objects to local_object_storage: Guarantee graves removal when handling expired tombstones 2024-12-24 10:17:48 +00:00
dstepanov-yadro requested changes 2024-12-27 14:40:57 +00:00
Dismissed
@ -314,0 +345,4 @@
err := db.boltDB.Batch(func(tx *bbolt.Tx) error {
graveyard := tx.Bucket(graveyardBucketName)
if graveyard == nil {
return errors.New("no graveyard bucket")

To var

To var
Author
Member

Fixed

Fixed
dstepanov-yadro marked this conversation as resolved
@ -42,0 +44,4 @@
return fmt.Errorf("get expiration epoch: %w", err)
}
if !has {
return errors.New("tombstone has no expiration epoch")

To var

To var
Author
Member

Fixed

Fixed
dstepanov-yadro marked this conversation as resolved
acid-ant reviewed 2025-01-09 07:56:54 +00:00
@ -298,2 +301,4 @@
}
// TODO(@a-savchuk): This condition should be checked
// in the beginning because of possible race with GC.
Member

Please, avoid orphaned TODO - create an issue and put a link here.

Please, avoid orphaned `TODO` - create an issue and put a link here.
Author
Member

Fixed

Fixed
a-savchuk force-pushed remove-dangling-locks from e12c763de6 to bc8daa618f 2025-01-14 10:11:53 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from bc8daa618f to df19fedf54 2025-01-14 12:11:43 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from df19fedf54 to 00f6d72999 2025-01-14 12:27:14 +00:00 Compare
dstepanov-yadro approved these changes 2025-01-14 12:58:50 +00:00
Dismissed
aarifullin reviewed 2025-01-15 07:52:11 +00:00
@ -23,1 +26,4 @@
}
// GetExpirationEpoch returns the expiration epoch of the object.
func GetExpirationEpoch(obj *objectSDK.Object) (has bool, epoch uint64, err error) {
Member

Not a big deal, but the prototype with return values (epoch uint64, found bool, err error) (has -> found) looks more natural for Go. WDYT?

Not a big deal, but the prototype with return values `(epoch uint64, found bool, err error)` (`has -> found`) looks more natural for Go. WDYT?
Author
Member

Yes, looks better. Fixed

Yes, looks better. Fixed
Owner

(optional) Also the name looks to me like ExpirationEpochOf (like AddressOf) and without Get prefix.

(optional) Also the name looks to me like `ExpirationEpochOf` (like `AddressOf`) and without `Get` prefix.
aarifullin marked this conversation as resolved
a-savchuk force-pushed remove-dangling-locks from 00f6d72999 to 66f7c86a55 2025-01-15 08:55:55 +00:00 Compare
a-savchuk dismissed dstepanov-yadro's review 2025-01-15 08:55:55 +00:00
Reason:

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

fyrchik requested changes 2025-01-15 09:03:58 +00:00
@ -23,1 +26,4 @@
}
// GetExpirationEpoch returns the expiration epoch of the object.
func GetExpirationEpoch(obj *objectSDK.Object) (has bool, epoch uint64, err error) {
Owner
  1. I suggest naming it ExpirationEpoch and reorder has and epoch parameters, ok bool is usually the second (see maps).
  2. Without context this looks like unnecessary complexity, could you merge it with the first commit you use this function in?
  3. This function needs tests.
1. I suggest naming it `ExpirationEpoch` and reorder `has` and `epoch` parameters, `ok bool` is usually the second (see maps). 2. Without context this looks like unnecessary complexity, could you merge it with the first commit you use this function in? 3. This function needs tests.
Author
Member

Done

Done
@ -24,0 +30,4 @@
attrs := obj.Attributes()
if header := obj.ECHeader(); header != nil {
attrs = header.ParentAttributes()
Owner

Why do we take parent attributes for chunk?

Why do we take parent attributes for chunk?
Author
Member

I followed a similar function in the metabase. Fixed it, don't use the EC header because it doesn't make much sense in the context where this function is now used - specifically, for tombstones and lock objects

I followed a similar function in the metabase. Fixed it, don't use the EC header because it doesn't make much sense in the context where this function is now used - specifically, for tombstones and lock objects
@ -314,0 +346,4 @@
err := db.boltDB.Batch(func(tx *bbolt.Tx) error {
graveyard := tx.Bucket(graveyardBucketName)
if graveyard == nil {
Owner

Is it really an error?
So no bucket, big deal.
graveyard.Delete line already doesn't check whether the grave exists.
And if we have no bucket, then the graves are already removed.

Is it really an error? So no bucket, big deal. `graveyard.Delete` line already doesn't check whether the grave exists. And if we have no bucket, then the graves are already removed.

graveyard is static bucket, so it must exist. So nil pointer panic looks ok for me as in other places.

`graveyard` is static bucket, so it must exist. So nil pointer panic looks ok for me as in other places.
Author
Member

For now, I made it return if there’s no graveyard bucket

For now, I made it return if there’s no graveyard bucket
@ -146,3 +148,3 @@
inhumePrm.SetAddresses(object.AddressOf(obj1), object.AddressOf(obj2))
inhumePrm.SetTombstoneAddress(addrTombstone)
inhumePrm.SetTombstoneAddress(addrTombstone, rand.Uint64())
Owner

I don't like rand in this context -- it can lead to a flaky test.
What about making it some constant instead?

I don't like `rand` in this context -- it can lead to a flaky test. What about making it some constant instead?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -427,3 +444,3 @@
// check if graveyard has record with key corresponding
// to tombstone address (at least one)
targetIsTomb = bytes.Equal(v, addressKey)
targetIsTomb = bytes.Equal(v[:addressKeySize], addressKey)
Owner

Old code was future proof and didn't panic.
Now we can panic if len(v) < addressKeySize.
The difference with addressKey[:..] is that len(v) is not statically known.

Old code was future proof and didn't panic. Now we can panic if `len(v) < addressKeySize`. The difference with `addressKey[:..]` is that `len(v)` is not statically known.
Author
Member

Use bytes.HasPrefix instead

Use `bytes.HasPrefix` instead
fyrchik marked this conversation as resolved
@ -312,0 +339,4 @@
// - tombstone address + expiration epoch
//
// Expiration epoch is set to [NoExpirationEpoch] if the key doesn't have it.
func decodeTombstoneKeyWithExpEpoch(addr *oid.Address, expEpoch *uint64, src []byte) error {
Owner

We already have a valid and idiomatic way to return multiple values.
Why have you decided to use pointers?

We already have a valid and idiomatic way to return multiple values. Why have you decided to use pointers?
Author
Member

We have

I think all of these are optimization, and I tried to follow this existing approach. However, neither decodeAddressFromKey from the example above nor decodeTombstoneKeyWithExpEpoch are used in a way that reuses decoded values

Thus, should I keep it or change?

We have - encoding functions that can reuse byte buffers https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/05fd999162f30042a0021f14a250aa3305b79f29/pkg/local_object_storage/metabase/util.go#L244-L248 - decoding functions that can reuse decoded values (the case we're currently discussing) https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/05fd999162f30042a0021f14a250aa3305b79f29/pkg/local_object_storage/metabase/util.go#L251 I think all of these are optimization, and I tried to follow this existing approach. However, neither `decodeAddressFromKey` from the example above nor `decodeTombstoneKeyWithExpEpoch` are used in a way that reuses decoded values Thus, should I keep it or change?
Owner

It could make sense for oid.Address because it is 64-bytes long.
Honestly, I would not write it this way, if I were to write it know -- if there would be perfomance problems here and/or noticeable improvements will be observed, a separate commit could introduce the optimization.

It could make sense for `oid.Address` because it is 64-bytes long. Honestly, I would not write it this way, if I were to write it know -- if there would be perfomance problems here and/or noticeable improvements will be observed, a separate commit could introduce the optimization.
Author
Member

Done

Done
@ -381,0 +382,4 @@
batches[typ] = make([]oid.Address, 0, batchSize)
}
errGroup.Go(func() error {
Owner

You do:

errGroup.Go(func() error {
   errGroup.Go(...) // 1
})
errGroup.Wait() // 2

If line (2) executes before line (1), it seems the code won't do what it intends to do (finish all processing before returning from the function).

You do: ``` errGroup.Go(func() error { errGroup.Go(...) // 1 }) errGroup.Wait() // 2 ``` If line (2) executes before line (1), it seems the code won't do what it intends to do (finish all processing before returning from the function).
Author
Member

However, the line (2) will be executed after (0), so on (2) the number of running goroutines is at least 1

errGroup.Go(func() error { // 0
   errGroup.Go(...) // 1
})
errGroup.Wait() // 2

We already use this approach in GC. As I understand this decision, it's the way to handle workers cancellation on an error: we do not use context.WithCancel and write all cancellation logic from scratch, instead we use an error group that is able to cancel workers on an error. Also, we have the minimal size of an error group that equals to 2, so at least one goroutine will be handling object batches

However, the line (2) will be executed after (0), so on (2) the number of running goroutines is at least 1 ``` errGroup.Go(func() error { // 0 errGroup.Go(...) // 1 }) errGroup.Wait() // 2 ``` We [already use](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/05fd999162f30042a0021f14a250aa3305b79f29/pkg/local_object_storage/shard/gc.go#L367-L398) this approach in GC. As I understand this decision, it's the way to handle workers cancellation on an error: we do not use `context.WithCancel` and write all cancellation logic from scratch, instead we use an error group that is able to cancel workers on an error. Also, we have the minimal size of an error group that equals to 2, so at least one goroutine will be handling object batches
Owner

Oh, right

Oh, right
fyrchik marked this conversation as resolved
@ -42,0 +46,4 @@
return fmt.Errorf("get expiration epoch: %w", err)
}
if !has {
return errObjectHasNoExpirationEpoch
Owner

Currently all GetExpirationEpoch callers return error on !has, does the second return value has any value (pun not intended)?

Currently all `GetExpirationEpoch` callers return error on `!has`, does the second return value has any value (pun not intended)?
Author
Member

For now, no, it doesn't, because expiration epoch is required for tombstones and lock objects. Later this distinction, an object has no epoch or we failed, may have some sense, but for now it doesn't

Fixed

For now, no, it doesn't, because expiration epoch is required for tombstones and lock objects. Later this distinction, an object has no epoch or we failed, may have some sense, but for now it doesn't Fixed
fyrchik marked this conversation as resolved
aarifullin reviewed 2025-01-15 09:58:46 +00:00
@ -120,3 +122,3 @@
// addr should not be nil.
// Should not be called along with SetGCMark.
func (p *InhumePrm) SetTombstoneAddress(addr oid.Address) {
func (p *InhumePrm) SetTombstoneAddress(addr oid.Address, expEpoch uint64) {
Member

SetTombstoneAddress has been, obviously, responsible for setting tombstone address :) And I think we shouldn't change its prototype as you don't set tombstone's address relying on expiration epoch.
Please, introduce separate setter for expEpoch

`SetTombstoneAddress` has been, obviously, responsible for setting tombstone address :) And I think we shouldn't change its prototype as you don't set tombstone's address relying on expiration epoch. Please, introduce separate setter for `expEpoch`
Author
Member

I understand your motivation, but having a separate setter may cause some problems. Consider, the following:

  • After this fix expiration, the expiration epoch is meant to be a required parameter for both the Inhume and Lock operations
  • However, I can't simply check it's set or not and then fail if it isn't, because I'd like to write tests where the graveyard bucket is populated with graves of both the old and new formats - i. e., with and without the epoch. For example:

    for i := range numGraves / 2 {
    expEpochs = append(expEpochs, uint64(i))
    expEpochs = append(expEpochs, meta.NoExpirationEpoch)
    }

When the expiration epoch is set together with the tombstone address it's explicit for user/developer. To summarize:

  • I don't want Inhume to fail if expiration epoch isn't set, because I won't be able to write tests
  • I want setting expiration epoch to be explicit

If this is still an issue, could we find another solution? Maybe change the setter name or something else?

I understand your motivation, but having a separate setter may cause some problems. Consider, the following: - After this fix expiration, the expiration epoch is meant to be a required parameter for both the `Inhume` and `Lock` operations - However, I can't simply check it's set or not and then fail if it isn't, because I'd like to write tests where the graveyard bucket is populated with graves of both the old and new formats - i. e., with and without the epoch. For example: https://git.frostfs.info/a-savchuk/frostfs-node/src/commit/514a9c8ef0d3eaaa2c41cef5c46f9ca48a02cab8/pkg/local_object_storage/metabase/graveyard_test.go#L495-L498 When the expiration epoch is set together with the tombstone address it's _explicit_ for user/developer. To summarize: - I don't want `Inhume` to fail if expiration epoch isn't set, because I won't be able to write tests - I want setting expiration epoch to be explicit If this is still an issue, could we find another solution? Maybe change the setter name or something else?
Member

I can't simply check it's set or not and then fail if it isn't

Okay, if I understood you correctly, then you're struggling the problem that you can't know if the expiration epoch has been set or not?
What if consider the approach used in frostfs-sdk-go with the isSet flag that come along with a field? Using flag is safer than using pointers that could indicate if a field is set or not

Example

type Token struct {
	targetUserSet bool
	targetUser    user.ID

	eaclTableSet bool
	eaclTable    eacl.Table

	lifetimeSet   bool
	iat, nbf, exp uint64

	sigSet bool
	sig    refs.Signature

	apeOverrideSet bool
	apeOverride    APEOverride

	impersonate bool
}
> I can't simply check it's set or not and then fail if it isn't Okay, if I understood you correctly, then you're struggling the problem that you can't know if the expiration epoch has been set or not? What if consider the approach used in `frostfs-sdk-go` with the `isSet` flag that come along with a field? Using flag is safer than using pointers that could indicate if a field is set or not Example ```go type Token struct { targetUserSet bool targetUser user.ID eaclTableSet bool eaclTable eacl.Table lifetimeSet bool iat, nbf, exp uint64 sigSet bool sig refs.Signature apeOverrideSet bool apeOverride APEOverride impersonate bool } ```
Author
Member

Done

Done
a-savchuk force-pushed remove-dangling-locks from 66f7c86a55 to 514a9c8ef0 2025-01-16 12:20:18 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 514a9c8ef0 to 3d4264e4c6 2025-01-21 10:05:06 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 3d4264e4c6 to 31e4536e9c 2025-01-22 18:52:18 +00:00 Compare
a-savchuk force-pushed remove-dangling-locks from 31e4536e9c to ed88c61303 2025-01-31 15:46:45 +00:00 Compare
acid-ant reviewed 2025-02-03 09:31:56 +00:00
@ -381,0 +386,4 @@
expErr := s.getExpiredObjects(egCtx, epoch, func(o *meta.ExpiredObject) {
typ := o.Type()
if !slices.Contains(knownTypes, typ) {
Member

Why not just check for existence of the key in map?

Why not just check for existence of the `key` in `map`?
Author
Member

Fixed

Fixed
acid-ant marked this conversation as resolved
a-savchuk force-pushed remove-dangling-locks from ed88c61303 to 47f4c51403 2025-02-03 10:49:23 +00:00 Compare
acid-ant approved these changes 2025-02-03 11:41:15 +00:00
All checks were successful
DCO action / DCO (pull_request) Successful in 39s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 57s
Required
Details
Build / Build Components (pull_request) Successful in 1m24s
Required
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m35s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 1m59s
Required
Details
Tests and linters / Run gofumpt (pull_request) Successful in 3m14s
Required
Details
Tests and linters / Tests (pull_request) Successful in 1m54s
Required
Details
Tests and linters / gopls check (pull_request) Successful in 3m35s
Required
Details
Tests and linters / Lint (pull_request) Successful in 3m49s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 4m36s
Required
Details
This pull request doesn't have enough approvals yet. 1 of 2 approvals granted.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u remove-dangling-locks:a-savchuk-remove-dangling-locks
git checkout a-savchuk-remove-dangling-locks
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1481
No description provided.