Skip to content

Implement generic tests for storage components#1718

Merged
fyrchik merged 10 commits intonspcc-dev:masterfrom
fyrchik:storage-tests
Aug 30, 2022
Merged

Implement generic tests for storage components#1718
fyrchik merged 10 commits intonspcc-dev:masterfrom
fyrchik:storage-tests

Conversation

@fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Aug 22, 2022

DRAFT, TODO:

  • Check IgnoreErrors flag for iteration over blobstor components.
  • Fix Raw flag to Get to return only raw data.
  • Add a test for testing put/get with and without compression.
  • Control tests for blobstor components.
  • Check that Put fails for blobstor components in readonly, but Get succeeds.
  • Test concurrent Put/Get/Delete for testing with -race flag.
  • Remove all already existing testing code, that is covered by generic tests.

@carpawell @cthulhu-rider @realloc I feel like something is missing for Open/SetMode/Close tests, may be you can suggest some other things that could possibly happen?

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #1718 (489594f) into master (c7c1c25) will increase coverage by 0.52%.
The diff coverage is 82.35%.

❗ Current head 489594f differs from pull request most recent head cdaafdc. Consider uploading reports for the commit cdaafdc to get more accurate results

@@            Coverage Diff             @@
##           master    #1718      +/-   ##
==========================================
+ Coverage   32.60%   33.13%   +0.52%     
==========================================
  Files         338      350      +12     
  Lines       22820    23224     +404     
==========================================
+ Hits         7441     7695     +254     
- Misses      14754    14897     +143     
- Partials      625      632       +7     
Impacted Files Coverage Δ
pkg/local_object_storage/blobovnicza/iterate.go 56.45% <ø> (-14.57%) ⬇️
pkg/local_object_storage/shard/control.go 77.86% <50.00%> (-1.21%) ⬇️
pkg/local_object_storage/blobstor/fstree/fstree.go 68.26% <63.63%> (+10.66%) ⬆️
pkg/local_object_storage/writecache/writecache.go 79.24% <84.61%> (ø)
...object_storage/blobstor/blobovniczatree/iterate.go 86.66% <93.33%> (+23.98%) ⬆️
..._object_storage/blobstor/blobovniczatree/delete.go 68.18% <100.00%> (+10.36%) ⬆️
...cal_object_storage/blobstor/blobovniczatree/get.go 66.66% <100.00%> (+5.79%) ⬆️
...cal_object_storage/blobstor/blobovniczatree/put.go 68.18% <100.00%> (+15.80%) ⬆️
...kg/local_object_storage/blobstor/fstree/control.go 100.00% <100.00%> (+100.00%) ⬆️
... and 41 more

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

@fyrchik
Copy link
Contributor Author

fyrchik commented Aug 23, 2022

Ready for review. Decided to postpone other points -- some of them require refactoring, others can affect stability, and the remaining need to be first discussed. I will create tasks.

carpawell
carpawell previously approved these changes Aug 26, 2022
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.

if it is ready, change commit messages then?

for i, component := range components {
if err := component.Open(false); err != nil {
if component == s.metaBase {
// We must first open all other components to avoid
Copy link
Member

Choose a reason for hiding this comment

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

can we simplify that code: just place meta in the first place? or you want to keep that info explicitly via the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but first need to think whether it affects anything. Personally, I think the order doesn't matter in terms of readability, as in future we would like to handle e.g. write cache failures.

@carpawell
Copy link
Member

maybe you can suggest some other things that could possibly happen?

@fyrchik, changing the configuration b/w opens?

@fyrchik
Copy link
Contributor Author

fyrchik commented Aug 29, 2022

The interface doesn't allow this, and, as you noticed, the proper support has not yet been implemented.

fyrchik added 10 commits August 29, 2022 07:23
All components must be opened first, before any other operation is
performed.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
Use them for writecache as a simple example.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
It is used only once, makes sense to inline.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
This tests check that each blobstor component behaves similarly when
same methods are being used. It is intended to serve as a specification
for all future components.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
If the file doesn't exist, return `apistatus.ObjectNotFound`.
First check is still there as a shortcut.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
This check should occur on the shard level, but because
blobstor components expose `Open(readOnly bool)` interface,
it is reasonable to expect an error here.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
The blobovniczatree test also checks the amount of objects we can put,
so leave it here.

Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
@fyrchik fyrchik merged commit 78bf17c into nspcc-dev:master Aug 30, 2022
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.

Implement generic tests for storage components Storage node panic while setting maintenance status

2 participants