TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 6 Projects Releases 1 Activity Actions

Fix big object deletion #896

Merged
fyrchik merged 5 commits from dstepanov-yadro/frostfs-node:fix/test_big_object_delete_is_flaky into master 2024-01-11 12:32:11 +00:00
Conversation 2 Commits 5 Files changed 72 +356 -178
dstepanov-yadro commented 2024-01-09 09:42:21 +00:00
Member
Copy link

  1. There was a bug: when GC deletes virtual (parent) object of complex object, it drops GC mark, but does nothing with parent object, so split info returns for Get/Exists requests.

  2. Also changed test logger. Now test related logs grouped after test. There are two differences from the previous implementation:

  • log records start with logger.go 139: prefix, it is testing package behaviour
  • log records don't colored by log levels
    But the ability to see logs for a specific test is more important, in my opinion.
  1. Fixed some tests, when storage engine's GC continues to work after test completed (caught by test -race after logger change)

Closes #895

1. There was a bug: when GC deletes virtual (parent) object of complex object, it drops GC mark, but does nothing with parent object, so split info returns for Get/Exists requests. 2. Also changed test logger. Now test related logs grouped after test. There are two differences from the previous implementation: - log records start with `logger.go 139: ` prefix, it is `testing` package behaviour - log records don't colored by log levels But the ability to see logs for a specific test is more important, in my opinion. 3. Fixed some tests, when storage engine's GC continues to work after test completed (caught by `test -race` after logger change) Closes #895
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from e2debba928 to 8ec5eb85f1 2024-01-09 13:27:09 +00:00 Compare
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from 4521ed1090 to ad47b177b5 2024-01-09 14:28:17 +00:00 Compare
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from d84d846af9 to 9e78baa3da 2024-01-10 09:16:14 +00:00 Compare
dstepanov-yadro reviewed 2024-01-10 09:26:53 +00:00
pkg/local_object_storage/engine/list_test.go Outdated
@ -83,3 +83,1 @@
t.Cleanup(func() {
e.Close(context.Background())
})
defer func() {
dstepanov-yadro commented 2024-01-10 09:26:53 +00:00
Author
Member
Copy link

https://github.com/golang/go/issues/40908

https://github.com/golang/go/issues/40908
fyrchik commented 2024-01-10 10:03:56 +00:00
Owner
Copy link

This is exactly why we got rid of zaptest and used zap.L(), these problems occured all over the tests (and writing t.Cleanup in the constructor is much easier that to remember writing it everywhere else.

This is exactly why we got rid of `zaptest` and used `zap.L()`, these problems occured all over the tests (and writing `t.Cleanup` in the constructor is much easier that to remember writing it everywhere else.
fyrchik commented 2024-01-10 10:07:05 +00:00
Owner
Copy link

#621

https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/621
dstepanov-yadro commented 2024-01-10 10:31:12 +00:00
Author
Member
Copy link

There was only one place in the entire project that needed to be fixed. But clear logs make sense for all of tests.

There was only one place in the entire project that needed to be fixed. But clear logs make sense for all of tests.
fyrchik commented 2024-01-10 12:30:20 +00:00
Owner
Copy link

Debatable: we do not need logs at all and when debugging tests usually a single test can be run.
Actually, I see lots of Cleanup with Close inside, they could trigger race detector later at some point.

I don't like returning the behaviour which clearly has problems and which we have intentionally fixed at some point.

Debatable: we do not need logs at all and when debugging tests usually a single test can be run. Actually, I see lots of `Cleanup` with `Close` inside, they could trigger race detector later at some point. I don't like returning the behaviour which clearly has problems and which we have intentionally fixed at some point.
dstepanov-yadro commented 2024-01-10 13:04:44 +00:00
Author
Member
Copy link

I disagree with the statement: we do not need logs at all. For several tasks already, I needed normal logs of falling tests.

I disagree with the statement: `we do not need logs at all`. For several tasks already, I needed normal logs of falling tests.
fyrchik commented 2024-01-10 15:28:47 +00:00
Owner
Copy link

Ok, but reverting a fix to a real problem is not the right approach here.

Ok, but reverting a fix to a real problem is not the right approach here.
dstepanov-yadro commented 2024-01-10 15:48:30 +00:00
Author
Member
Copy link

Now there is no problem: race condition for t.Cleanup is actual only for engine after Init().
Looks like it was an inappropriate fix.

Now there is no problem: race condition for `t.Cleanup` is actual only for engine after `Init()`. Looks like it was an inappropriate fix.
dstepanov-yadro commented 2024-01-10 16:10:05 +00:00
Author
Member
Copy link

Also see this comment (testing.go: 1580):

		// Do not lock t.done to allow race detector to detect race in case
		// the user does not appropriately synchronize a goroutine.

As far as I understand Cleanup requires all test background goroutines must be stopped. So using Cleanup for engine.Close is invalid usage.

Also see this comment (testing.go: 1580): ``` // Do not lock t.done to allow race detector to detect race in case // the user does not appropriately synchronize a goroutine. ``` As far as I understand `Cleanup` requires all test background goroutines must be stopped. So using `Cleanup` for `engine.Close` is invalid usage.
fyrchik commented 2024-01-10 16:33:42 +00:00
Owner
Copy link

I am not sure the problems is gone now. The problem is that

  1. In logger we read done field cc85462b3d/src/testing/testing.go (L1017)
  2. done is written to intentionally without a mutex in cc85462b3d/src/testing/testing.go (L1580)

If you do rg 'Cleanup\(' -A4 over the codebase, there are multiple calls to releaseShard in Cleanup (and to writecache etc.), because we currently use Cleanup() in tests. Are you sure there are no goroutines in Shard which can log and run until Close() is called? In the writecache?

Or here, is it different from the list_test.go situation: cbc78a8efb/pkg/local_object_storage/engine/shards_test.go (L16) ?

I would rather see a discussion first.

I am not sure the problems is gone now. The problem is that 1. In logger we read `done` field https://github.com/golang/go/blob/cc85462b3d23193e4861813ea85e254cfe372403/src/testing/testing.go#L1017 2. `done` is written to intentionally without a mutex in https://github.com/golang/go/blob/cc85462b3d23193e4861813ea85e254cfe372403/src/testing/testing.go#L1580 If you do `rg 'Cleanup\(' -A4` over the codebase, there are multiple calls to `releaseShard` in `Cleanup` (and to writecache etc.), because we currently use `Cleanup()` in tests. Are you _sure_ there are no goroutines in `Shard` which can log and run until `Close()` is called? In the writecache? Or here, is it different from the `list_test.go` situation: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/cbc78a8efb72c40c7e39cccdc0aed4dc387fb053/pkg/local_object_storage/engine/shards_test.go#L16 ? I would rather see a discussion first.
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from 9e78baa3da to f658b085e7 2024-01-10 09:27:57 +00:00 Compare
dstepanov-yadro changed title from WIP: Fix big object delete test to Fix big object deletion 2024-01-10 09:28:19 +00:00
dstepanov-yadro requested review from storage-core-committers 2024-01-10 09:28:25 +00:00
acid-ant approved these changes 2024-01-10 10:03:04 +00:00
pkg/local_object_storage/metabase/delete.go Outdated
@ -330,0 +333,4 @@
garbageBKT := tx.Bucket(garbageBucketName)
key := make([]byte, addressKeySize)
addrKey := addressKey(object.AddressOf(obj), key)
if garbageBKT != nil {
acid-ant commented 2024-01-10 10:02:27 +00:00
Member
Copy link

Why not to check it earlier?

Why not to check it earlier?
dstepanov-yadro commented 2024-01-10 10:25:35 +00:00
Author
Member
Copy link

Fixed

Fixed
acid-ant marked this conversation as resolved
dstepanov-yadro requested review from storage-core-developers 2024-01-10 10:16:27 +00:00
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from f658b085e7 to 1dc53e0120 2024-01-10 10:24:43 +00:00 Compare
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from 1dc53e0120 to cbc78a8efb 2024-01-10 10:32:40 +00:00 Compare
aarifullin approved these changes 2024-01-10 10:39:00 +00:00
aarifullin left a comment
Member
Copy link

LGTM

LGTM
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from cbc78a8efb to 9494abdb69 2024-01-11 07:22:40 +00:00 Compare
dstepanov-yadro reviewed 2024-01-11 07:24:04 +00:00
pkg/local_object_storage/blobovnicza/get_test.go Outdated
@ -18,4 +17,0 @@
t.Cleanup(func() {
blz.Close()
os.RemoveAll(filename)
dstepanov-yadro commented 2024-01-11 07:24:04 +00:00
Author
Member
Copy link

t.TempDir will be removed by testing engine.

`t.TempDir` will be removed by testing engine.
dstepanov-yadro reviewed 2024-01-11 07:26:13 +00:00
pkg/local_object_storage/blobstor/perf_test.go Outdated
@ -25,10 +25,6 @@ func (s storage) open(b *testing.B) common.Storage {
require.NoError(b, st.Open(false))
require.NoError(b, st.Init())
b.Cleanup(func() {
dstepanov-yadro commented 2024-01-11 07:26:13 +00:00
Author
Member
Copy link

Close call moved to caller.

`Close` call moved to caller.
fyrchik approved these changes 2024-01-11 07:28:04 +00:00
fyrchik commented 2024-01-11 07:28:31 +00:00
Owner
Copy link

Tests with -race fail, disregard approval.

Tests with `-race` fail, disregard approval.
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from 9494abdb69 to 284ef903ac 2024-01-11 07:42:00 +00:00 Compare
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from 284ef903ac to 0cfe98b9be 2024-01-11 08:09:59 +00:00 Compare
dstepanov-yadro force-pushed fix/test_big_object_delete_is_flaky from 0cfe98b9be to 50e648e3b1 2024-01-11 08:10:52 +00:00 Compare
dstepanov-yadro commented 2024-01-11 09:23:32 +00:00
Author
Member
Copy link

Tests with -race fail, disregard approval.

Fixed

> Tests with `-race` fail, disregard approval. Fixed
fyrchik approved these changes 2024-01-11 12:32:03 +00:00
fyrchik merged commit a2ab373a0a into master 2024-01-11 12:32:11 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
aarifullin
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 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#896
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/test_big_object_delete_is_flaky"

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?