local_object_storage: Guarantee graves removal when handling expired tombstones #1481
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1481
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "a-savchuk/frostfs-node:remove-dangling-locks"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#1445
To summarize,
I've described the old and new behavior of GC in #1445. Feel free to ask any questions and argue.
0013294c1b
to93c9ec3256
93c9ec3256
to45cbd497cd
45cbd497cd
to6738de7df7
@ -139,0 +165,4 @@
})
}
func (e *StorageEngine) dropOrphanLocks(ctx context.Context, cnt cid.ID, locks []oid.ID) (locksToKeep []oid.ID) {
Why it is named
drop
when it is used to find IDs?I've changed the solution drastically. Please see the task and PR description for more information
6738de7df7
tobe8b99e3bc
be8b99e3bc
to163c3dba71
163c3dba71
to34df21cc4a
34df21cc4a
toe2a6663ef6
42681223d0
toe2d26cd213
e2d26cd213
to0092a49f8f
0092a49f8f
tode1ef66a21
de1ef66a21
toccdcd67ec3
WIP: policer: Handle oprhan locked bucket and graveyard bucket recordsto WIP: local_object_storage: Guarantee consistency when handling expired tombstones and lock objectsWIP: local_object_storage: Guarantee consistency when handling expired tombstones and lock objectsto local_object_storage: Guarantee consistency when handling expired tombstones and lock objectsccdcd67ec3
to220dc8af30
220dc8af30
to5b43f2a1f0
5b43f2a1f0
to4c73f1be9c
4c73f1be9c
toe12c763de6
local_object_storage: Guarantee consistency when handling expired tombstones and lock objectsto local_object_storage: Guarantee graves removal when handling expired tombstones@ -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
Fixed
@ -42,0 +44,4 @@
return fmt.Errorf("get expiration epoch: %w", err)
}
if !has {
return errors.New("tombstone has no expiration epoch")
To var
Fixed
@ -298,2 +301,4 @@
}
// TODO(@a-savchuk): This condition should be checked
// in the beginning because of possible race with GC.
Please, avoid orphaned
TODO
- create an issue and put a link here.Fixed
e12c763de6
tobc8daa618f
bc8daa618f
todf19fedf54
df19fedf54
to00f6d72999
@ -23,1 +26,4 @@
}
// GetExpirationEpoch returns the expiration epoch of the object.
func GetExpirationEpoch(obj *objectSDK.Object) (has bool, epoch uint64, err error) {
Not a big deal, but the prototype with return values
(epoch uint64, found bool, err error)
(has -> found
) looks more natural for Go. WDYT?Yes, looks better. Fixed
(optional) Also the name looks to me like
ExpirationEpochOf
(likeAddressOf
) and withoutGet
prefix.00f6d72999
to66f7c86a55
New commits pushed, approval review dismissed automatically according to repository settings
@ -23,1 +26,4 @@
}
// GetExpirationEpoch returns the expiration epoch of the object.
func GetExpirationEpoch(obj *objectSDK.Object) (has bool, epoch uint64, err error) {
ExpirationEpoch
and reorderhas
andepoch
parameters,ok bool
is usually the second (see maps).Done
@ -24,0 +30,4 @@
attrs := obj.Attributes()
if header := obj.ECHeader(); header != nil {
attrs = header.ParentAttributes()
Why do we take parent attributes for chunk?
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 {
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.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())
I don't like
rand
in this context -- it can lead to a flaky test.What about making it some constant instead?
Fixed
@ -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)
Old code was future proof and didn't panic.
Now we can panic if
len(v) < addressKeySize
.The difference with
addressKey[:..]
is thatlen(v)
is not statically known.Use
bytes.HasPrefix
instead@ -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 {
We already have a valid and idiomatic way to return multiple values.
Why have you decided to use pointers?
We have
encoding functions that can reuse byte buffers
func addressKey(addr oid.Address, key []byte) []byte {
addr.Container().Encode(key)
addr.Object().Encode(key[cidSize:])
return key[:addressKeySize]
}
decoding functions that can reuse decoded values (the case we're currently discussing)
func decodeAddressFromKey(dst *oid.Address, k []byte) error {
I think all of these are optimization, and I tried to follow this existing approach. However, neither
decodeAddressFromKey
from the example above nordecodeTombstoneKeyWithExpEpoch
are used in a way that reuses decoded valuesThus, should I keep it or change?
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.
Done
@ -381,0 +382,4 @@
batches[typ] = make([]oid.Address, 0, batchSize)
}
errGroup.Go(func() error {
You do:
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).
However, the line (2) will be executed after (0), so on (2) the number of running goroutines is at least 1
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 batchesOh, right
@ -42,0 +46,4 @@
return fmt.Errorf("get expiration epoch: %w", err)
}
if !has {
return errObjectHasNoExpirationEpoch
Currently all
GetExpirationEpoch
callers return error on!has
, does the second return value has any value (pun not intended)?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
@ -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) {
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
I understand your motivation, but having a separate setter may cause some problems. Consider, the following:
Inhume
andLock
operationsfor 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:
Inhume
to fail if expiration epoch isn't set, because I won't be able to write testsIf this is still an issue, could we find another solution? Maybe change the setter name or something else?
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 theisSet
flag that come along with a field? Using flag is safer than using pointers that could indicate if a field is set or notExample
Done
66f7c86a55
to514a9c8ef0
514a9c8ef0
to3d4264e4c6
3d4264e4c6
to31e4536e9c
a-savchuk referenced this pull request2025-01-28 07:16:35 +00:00
31e4536e9c
toed88c61303
@ -381,0 +386,4 @@
expErr := s.getExpiredObjects(egCtx, epoch, func(o *meta.ExpiredObject) {
typ := o.Type()
if !slices.Contains(knownTypes, typ) {
Why not just check for existence of the
key
inmap
?Fixed
ed88c61303
to47f4c51403
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.