writecache: Fix metric values #759

dstepanov-yadro · 2023-10-27T09:15:46Z

dstepanov-yadro commented

2023-10-27 09:15:46 +00:00

Closes #725

Main problem is that c.metrics.Evict (also c.metrics.Delete and c.metrics.Add) changes actual count metric value, but the same object can be flushed twice, so in this case metric value can be negative.

Now only one place left with metrics update - c.estimateCacheSize().

Thanx to @elebedeva for troubleshooting, bug reproduce and fix suggestion.

Closes #725 Main problem is that `c.metrics.Evict` (also `c.metrics.Delete` and `c.metrics.Add`) changes actual count metric value, but the same object can be flushed twice, so in this case metric value can be negative. Now only one place left with metrics update - `c.estimateCacheSize()`. Thanx to @elebedeva for troubleshooting, bug reproduce and fix suggestion.

dstepanov-yadro reviewed 2023-10-27 09:20:58 +00:00

pkg/local_object_storage/writecache/writecachebbolt/flush.go Outdated

					
				@ -60,6 +60,7 @@ func (c *cache) runFlushLoop(ctx context.Context) {

							case <-tt.C:

								c.flushSmallObjects(ctx)

								tt.Reset(defaultFlushInterval)

								c.estimateCacheSize()

dstepanov-yadro commented

2023-10-27 09:20:58 +00:00

Scenario

Cache DB contains two object
Goroutine 1: flush and delete object 1, decrements counter, counter value = 1
Goroutine 2: flush and delete object 2, decrements counter, counter value = 0, set metric value to 0
Goroutine 1: set metric value to 1

Now metric will be updated every 1 second in case of empty cache.

Scenario Cache DB contains two object Goroutine 1: flush and delete object 1, decrements counter, counter value = 1 Goroutine 2: flush and delete object 2, decrements counter, counter value = 0, set metric value to 0 Goroutine 1: set metric value to 1 Now metric will be updated every 1 second in case of empty cache.

fyrchik commented

2023-10-27 09:27:40 +00:00

In you scenario we have metric=1, when it should be 0. What is the scenario for the negative metric?

dstepanov-yadro commented

2023-10-27 09:33:50 +00:00

Described in main comment.

When we flush the same object twice: we can put the same object in channel twice, if worker haven't deleted it yet. c.metrics.Evict is called twice, so metric counter also was decremented twice, because recordDeleted is not checked in c.metrics.Evict: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/writecache/writecachebbolt/storage.go#L81

Described in main comment. When we flush the same object twice: we can put the same object in channel twice, if worker haven't deleted it yet. `c.metrics.Evict` is called twice, so metric counter also was decremented twice, because `recordDeleted` is not checked in `c.metrics.Evict`: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/writecache/writecachebbolt/storage.go#L81

fyrchik marked this conversation as resolved

dstepanov-yadro requested review from storage-core-committers 2023-10-27 09:21:32 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-10-27 09:21:32 +00:00

dstepanov-yadro force-pushed fix/wc_counter from d368a5aeba to d4b6ebe7e7

2023-10-27 09:23:04 +00:00

Compare

fyrchik approved these changes 2023-10-27 09:28:18 +00:00

pkg/local_object_storage/writecache/writecachebbolt/storage.go

					
				@ -73,7 +73,7 @@ func (c *cache) deleteFromDB(key string) {

					err := c.db.Batch(func(tx *bbolt.Tx) error {

						b := tx.Bucket(defaultBucket)

						key := []byte(key)

						recordDeleted = !recordDeleted && b.Get(key) != nil