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

blobstor: Try another sub-storage when the current one is full #2450

Merged

Conversation

cthulhu-rider
Copy link
Contributor

general improvement mentioned #2382 (comment)

Previously, `BlobStor.Put` always failed when the first sub-storage that
matched the policy failed. In particular, when `Blobovnicza` tree
(limited by size) was full, BlobStor didn' try to store data in the
`FSTree`. This behavior led to storage service denial.

Make `BlobStor.Put` to try next sub-storage when currently processed one
is full (returns `common.ErrNoSpace` error). The method still
immediately fails on any other error.

Closes nspcc-dev#2382.

Signed-off-by: Leonard Lyubich <[email protected]>
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #2450 (6473554) into master (f751d71) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 6473554 differs from pull request most recent head 9cabb44. Consider uploading reports for the commit 9cabb44 to get more accurate results

@@            Coverage Diff             @@
##           master    #2450      +/-   ##
==========================================
+ Coverage   29.41%   29.43%   +0.02%     
==========================================
  Files         399      399              
  Lines       30346    30353       +7     
==========================================
+ Hits         8925     8934       +9     
+ Misses      20680    20678       -2     
  Partials      741      741              
Impacted Files Coverage Δ
pkg/local_object_storage/blobstor/put.go 88.46% <100.00%> (+4.25%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Previously, `BlobStor` returned `ErrNoPlaceFound` when all sub-storages
were full. This error is related to policy compliance instead of free
space. To make component more responsive, the `ErrNoSpace` error should
be returned in these cases.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider cthulhu-rider force-pushed the bugfix/2382-blobovnicza-overflow branch from 7a3896b to 9cabb44 Compare July 21, 2023 16:27
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

Is it a good solution? How temporary is it? Without any (backward) migration, it seems strange to me. Don't we want to just simplify blobovniczas and then see if #2382 is done?

@roman-khimov
Copy link
Member

To me it fixes #2382, object will be stored successfully and it'll be found easily as well. bbczs refactoring is a different story.

@cthulhu-rider
Copy link
Contributor Author

Without any (backward) migration

u can create an issue for this, imo such routine would complicate things rather than optimize smth (data removal isn't so frequent like write load spikes are)

Don't we want to just #2453 blobovniczas

simplification doesn't prevent no space cases, so these changes are independent

Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

simplification doesn't prevent no space cases, so these changes are independent

But we are not sure that it is a "no space left" bug. Simplification would solve that misunderstanding.

imo such routine would complicate things rather than optimize smth

Without any error logs it may store dozen of gigabytes on the wrong (nonoptimal) storage.

@cthulhu-rider
Copy link
Contributor Author

But we are not sure that it is a "no space left" bug. Simplification would solve that misunderstanding.

this PR fixes particular problem: when blobovnicza says "i'm full" we must not deny the request and must fallback to any free sub-storage

Without any error logs it may store dozen of gigabytes on the wrong (nonoptimal) storage.

in practice, when blobovnicza is full, FS tree is unlikely to save us if it is on the same drive. In theory, blobstor doesn't operate with "optimality", it has list of sub-storages which can or can not accept the object. Right now, when blobovnicza overflowed, there is no clear event when we should overtake data from FS tree to it. If u can suggest any particular behavioral model, pls fill an issue. Right now service downtime is worse than optimality anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants