Skip to content
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.

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

Fix/Do not return expired objects #1634

Merged
merged 2 commits into from
Aug 4, 2022
Merged

Fix/Do not return expired objects #1634

merged 2 commits into from
Aug 4, 2022

Conversation

carpawell
Copy link
Member

@carpawell carpawell commented Jul 27, 2022

Closes #1617.

@carpawell carpawell added bug Something isn't working neofs-storage Storage node application issues labels Jul 27, 2022
@carpawell carpawell self-assigned this Jul 27, 2022
@carpawell carpawell marked this pull request as ready for review July 27, 2022 18:43
@carpawell carpawell requested a review from fyrchik July 27, 2022 18:48
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #1634 (0599286) into master (a97dee0) will increase coverage by 0.01%.
The diff coverage is 69.56%.

❗ Current head 0599286 differs from pull request most recent head 030fc3c. Consider uploading reports for the commit 030fc3c to get more accurate results

@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
+ Coverage   33.17%   33.19%   +0.01%     
==========================================
  Files         332      332              
  Lines       22709    22744      +35     
==========================================
+ Hits         7534     7549      +15     
- Misses      14557    14576      +19     
- Partials      618      619       +1     
Impacted Files Coverage Δ
pkg/local_object_storage/engine/delete.go 62.22% <0.00%> (ø)
pkg/local_object_storage/engine/exists.go 42.30% <0.00%> (-3.53%) ⬇️
pkg/local_object_storage/engine/get.go 62.79% <0.00%> (-2.27%) ⬇️
pkg/local_object_storage/engine/head.go 68.83% <0.00%> (-3.78%) ⬇️
pkg/local_object_storage/engine/inhume.go 64.07% <0.00%> (ø)
pkg/local_object_storage/engine/lock.go 48.00% <0.00%> (-2.00%) ⬇️
pkg/local_object_storage/engine/put.go 53.84% <0.00%> (-1.71%) ⬇️
pkg/local_object_storage/shard/control.go 78.90% <0.00%> (ø)
pkg/local_object_storage/shard/errors.go 50.00% <0.00%> (-16.67%) ⬇️
pkg/local_object_storage/shard/exists.go 0.00% <ø> (ø)
... and 16 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

pkg/local_object_storage/metabase/db.go Show resolved Hide resolved
pkg/local_object_storage/metabase/exists.go Outdated Show resolved Hide resolved
pkg/local_object_storage/metabase/exists.go Outdated Show resolved Hide resolved
pkg/local_object_storage/metabase/exists.go Outdated Show resolved Hide resolved
expirationBucket := tx.Bucket(attributeBucketName(addr.Container(), objectV2.SysAttributeExpEpoch))
if expirationBucket != nil {
// bucket that contains objects that expire in the current epoch
currEpochBkt := expirationBucket.Bucket([]byte(strconv.FormatUint(currEpoch, 10)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the objects that expired in the previous epoch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've added comment to that func:

// we check only if the object is expired in the current
// epoch since it is considered the only corner case: the
// GC is expected to collect all the objects that have
// expired previously for less than the one epoch duration

i expect that GC is able to collect all the expired objects in one epoch (one hour currently), so the only problem objects are the object that expire in the current epoch but have not been collected by the GC yet. theoretically, we could check more than one epoch (we do not have index that could say what objects are expired, only the objects that has exact expiration epoch), so we should know how deep to check OR we should add new expiration index

i thought about force new epoch event that could lead to some problems. we may add some threshold for that cases (e.g. check if an object was not expired in the last 5 epoch)

what do you think?

pkg/local_object_storage/writecache/init.go Outdated Show resolved Hide resolved
@carpawell carpawell requested a review from fyrchik July 28, 2022 17:41
@@ -89,7 +90,7 @@ func (db *DB) delete(tx *bbolt.Tx, addr oid.Address, refCounter referenceCounter
}

// unmarshal object, work only with physically stored (raw == true) objects
obj, err := db.get(tx, addr, false, true)
obj, err := db.get(tx, addr, false, true, currEpoch)
Copy link
Contributor

@fyrchik fyrchik Jul 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we ever delete an expired object from the metabase?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

third arg (false here) controls checking object status. if an object is presented, it will be deleted, despite its status

@KirillovDenis
Copy link
Contributor

Resolve conflicts please

cthulhu-rider
cthulhu-rider previously approved these changes Aug 4, 2022
pkg/local_object_storage/metabase/db.go Outdated Show resolved Hide resolved
It allows performing expiration checks on the stored objects.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
If an object has not been marked for removal by the GC in the current epoch
yet but has already expired, respond with `ErrObjectNotFound` api status.
Also, optimize shard iteration: a node must stop any iteration if the object
 is found but gonna be removed soon.
All the checks are performed by the Metabase.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell merged commit 156ba85 into nspcc-dev:master Aug 4, 2022
carpawell added a commit that referenced this pull request Aug 4, 2022
It allows performing expiration checks on the stored objects.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell deleted the fix/do-not-return-exired-objects branch August 4, 2022 13:31
@alexchetaev alexchetaev added the U3 Regular label Aug 23, 2022
cthulhu-rider pushed a commit that referenced this pull request Sep 13, 2022
Do not treat objects with expiration as expired by default.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
It allows performing expiration checks on the stored objects.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
If an object has not been marked for removal by the GC in the current epoch
yet but has already expired, respond with `ErrObjectNotFound` api status.
Also, optimize shard iteration: a node must stop any iteration if the object
 is found but gonna be removed soon.
All the checks are performed by the Metabase.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
Do not treat objects with expiration as expired by default.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working neofs-storage Storage node application issues U3 Regular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check expiration on Get/GetRange/etc
5 participants