Fix: Pyramid delete before open #4062

N-o-Z · 2022-08-31T12:43:50Z

Closes Pyramid: Fix delete file before open #4061

itaiad200 · 2022-08-31T12:50:29Z

pkg/pyramid/tier_fs.go

+	rcb := tfs.fileLocker.lock(fileRef.fsRelativePath)
+	defer rcb() // release callback


Suggested change

rcb := tfs.fileLocker.lock(fileRef.fsRelativePath)

defer rcb() // release callback

releaseFileLock := tfs.fileLocker.lock(fileRef.fsRelativePath)

defer releaseFileLock()

nopcoder · 2022-09-01T07:39:39Z

pkg/pyramid/file_locker.go

+// fileLocker allows file locking to avoid race conditions between cache rejection/eviction
+// and the file usage by TierFS
+type fileLocker struct {
+	wgMap *sync.Map


Consider using map+mutex (usually it will work faster)

Using a map with mutex will remove the extra allocation of waitgroup in case there is already a waitgroup on the path

N-o-Z · 2022-09-01T15:06:54Z

@itaiad200 @nopcoder Please see new code

nopcoder · 2022-09-01T15:25:45Z

pkg/pyramid/file_tracker.go

+	if val, ok := t.refMap[string(path)]; ok {
+		ref = val + 1
+		t.refMap[string(path)] = ref
+	} else {
+		t.refMap[string(path)] = ref
+	}


Suggested change

if val, ok := t.refMap[string(path)]; ok {

ref = val + 1

t.refMap[string(path)] = ref

} else {

t.refMap[string(path)] = ref

}

if val, ok := t.refMap[string(path)]; ok {

ref += val

}

t.refMap[string(path)] = ref

nopcoder · 2022-09-01T15:29:23Z

pkg/pyramid/file_tracker.go

+func newFileTracker() *fileTracker {
+	return &fileTracker{
+		refMap: map[string]int{},
+		mu:     sync.Mutex{},


can remove the mu initialization it is a value type no need initialize it with the same value

nopcoder · 2022-09-01T15:30:25Z

pkg/pyramid/file_tracker.go

+	}
+}
+
+func (t *fileTracker) dec(path params.RelativePath, fn decCallback) {


I think we can remove the 'fn' argument here - if dec returns true/false or count - we can defer call to 'dec' and delete the file.

nopcoder · 2022-09-01T15:39:06Z

pkg/pyramid/tier_fs.go

@@ -114,7 +115,12 @@ func (tfs *TierFS) removeFromLocal(rPath params.RelativePath, filesize int64) {
 	// This will be called by the cache eviction mechanism during entry insert.
 	// We don't want to wait while the file is being removed from the local disk.
 	evictionHistograms.WithLabelValues(tfs.fsName).Observe(float64(filesize))
-	go tfs.removeFromLocalInternal(rPath)
+	// Decrement the reference count on the file. When the ref count reaches -1, The file will be deleted
+	tfs.fileTracker.dec(rPath, func(ref int) {


I think we need a 'delete' marker on the map - we like that for every time we called open we inc/dec in case the file wasn't found - it means that evict delete it.
if we like a better visibility I think the tracker should understand when to perform the callback or make sure that every time you call dec you also check and delete the file.

Something like:
tracker := NewTracker(deleteCallback) // callback to the implementation on how to delete file from local

closer := tracker.Open(path); // return a callback
defer closer() // closer will take the lock remove the ref count when 0 and remove from map will check if a delete flag is mark and call the callback

tracker.Delete(path) // mark the file for delete or delete by keeping the lock

itaidavid · 2022-09-01T18:39:45Z

pkg/pyramid/file_tracker.go

+	if ref == -1 {
+		delete(t.refMap, string(path))
+	}


Why is -1 the point of eviction and not 0?

What is the meaning of dec an non existing path?

0 Doesn't mean the file was flagged for eviction - it just means that nobody is currently trying to open it.
When we trigger the eviction function it performs a decrement on the file without a prior increment - which will eventually after all the open requests completed will cause it to reach -1. This is the indication that the file was requested for deletion and only then the logic will trigger

itaidavid · 2022-09-01T18:48:06Z

pkg/pyramid/tier_fs.go

+		if ref == -1 {
+			tfs.removeFromLocalInternal(rPath)


What happens if ref returns as -1 and before the file is removed, it is acquired again, by another thread.
I think it will still be removed, as the mutex protects the ref-count, but nothing prevents ref increase, after the -1 is returned and removeFromLocalInternal is fired

This cannot happen because removeFromLocalInternal happens in the scope of dec where we have a mutex lock on the map.
What will happen is that on dec the entry will be deleted from the map, and the file deleted from storage. Only after that the lock will be released - the inc will happen and a new entry in the map will be created for the file

itaidavid

Can you please add some tests for this? Apart from helping in verifying this fix, it can also help in understanding what flows/use-cases are relevant, and by that to really understand the problem.

Another thing, I am not sure this fix really protect against deleting the file by one thread, from under the legs of another. Please see comment inside

N-o-Z · 2022-09-02T10:58:28Z

Please see revised code

itaiad200

Blocking this PR until Sunday

nopcoder

Looks better - comments related on how we handle delete

nopcoder · 2022-09-02T11:18:32Z

pkg/pyramid/file_tracker.go

+func newFileTracker() *fileTracker {
+	return &fileTracker{
+		refMap: map[string]int{},
+		mu:     sync.Mutex{},


can remove the mu initialization it is a value type no need initialize it with the same value

nopcoder · 2022-09-02T11:20:26Z

pkg/pyramid/file_tracker.go

+		if val.ref == 0 && val.deleted {
+			t.innerDelete(path)
+		}


when val.ref == 0 - we need to delete it from the map

when we delete it from the map and val.deleted true - we need to invoke the callback

nopcoder · 2022-09-02T11:24:26Z

pkg/pyramid/file_tracker.go

+		val.deleted = true
+		if val.ref == 0 {
+			t.innerDelete(path)
+		}


On Delete - when path in map - mark it 'deleted' = true.
When path not in map - delete the file.
All while map is locked.

nopcoder · 2022-09-02T11:26:00Z

pkg/pyramid/tier_fs.go

@@ -256,19 +261,12 @@ func (tfs *TierFS) openFile(ctx context.Context, fileRef localFileRef, fh *os.Fi
 	}

 	if !tfs.eviction.Store(fileRef.fsRelativePath, stat.Size()) {


OnReject is called also in case method returns false?

Both OnEvict and OnReject CB are configured to run the same OnEvict method

Right - but for a rejected file you will call tfs.fileTracker.Delete(rPath) twice. Here and in the OnReject callback (which calls tfs.removeFromLocal)

From what I see onReject is called only in processItems (NewCache, Clear)
This flow is for Set

Now sure I understand. onReject is called whenever a file isn't being stored (decided by their policy) - so here we would definitely call Delete twice for a rejected file.

ristrettoEviction Store calls cache Set which in turn calls SetWithTTL.
In the SetWithTTL flow there are several places where the function return false without calling onReject CB.

Right - onReject only gets called when the eviction policy decides to reject. All other rejections are not called.

nopcoder

lgtm!

pkg/pyramid/file_tracker_test.go

itaidavid · 2022-09-04T01:59:59Z

pkg/pyramid/file_tracker_test.go

+
+var testFileMap map[string]bool
+
+func TestTracker(t *testing.T) {


Awesome 🤩

itaidavid

Thanks for the recent changers. Excellent test

itaiad200

See comment and question inside.
Also - how was this tested? I don't think our current tests cover cache being filled up and many concurrent calls to the same paths.

itaiad200 · 2022-09-05T10:18:21Z

pkg/pyramid/tier_fs.go

@@ -256,19 +261,12 @@ func (tfs *TierFS) openFile(ctx context.Context, fileRef localFileRef, fh *os.Fi
 	}

 	if !tfs.eviction.Store(fileRef.fsRelativePath, stat.Size()) {


Right - but for a rejected file you will call tfs.fileTracker.Delete(rPath) twice. Here and in the OnReject callback (which calls tfs.removeFromLocal)

itaiad200 · 2022-09-05T10:20:48Z

pkg/pyramid/tier_fs.go

-		errorsTotal.WithLabelValues(tfs.fsName, "DirRemoval")
-	}
+	// Delete Dir async
+	go func() {


Why are we changing it to async?

removeFromLocalInternal was called asynchronously to not delay the flow on the delete dir operation. Now that it is called out of context, I've modified only the call to delete dir to be perfomed async

N-o-Z · 2022-09-05T11:56:35Z

See comment and question inside. Also - how was this tested? I don't think our current tests cover cache being filled up and many concurrent calls to the same paths.

I added a unit test for the file tracker. It is true that we currently don't have test that perform concurrent operations but the unit test should cover this specific scenario

itaiad200 · 2022-09-05T12:27:30Z

pkg/pyramid/tier_fs.go

@@ -256,19 +261,12 @@ func (tfs *TierFS) openFile(ctx context.Context, fileRef localFileRef, fh *os.Fi
 	}

 	if !tfs.eviction.Store(fileRef.fsRelativePath, stat.Size()) {


Now sure I understand. onReject is called whenever a file isn't being stored (decided by their policy) - so here we would definitely call Delete twice for a rejected file.

itaiad200 · 2022-09-05T12:31:56Z

See comment and question inside. Also - how was this tested? I don't think our current tests cover cache being filled up and many concurrent calls to the same paths.

I added a unit test for the file tracker. It is true that we currently don't have test that perform concurrent operations but the unit test should cover this specific scenario

I don't think that it will. We have an issue when the cache gets filled up. If we don't have a unit test we need to test it manually to verify that we don't encounter bugs when the cache gets filled up and that the cache size stays within its limits (no untracked files) with some deviation as we're not guaranteed to be always within bounds (there are short periods where we might cross the limits).

Co-authored-by: itai-david <90712874+itai-david@users.noreply.github.com>

itaiad200

Tested together on a real scenario - works amazing!

N-o-Z added bug Something isn't working exclude-changelog PR description should not be included in next release changelog team/versioning-engine Team versioning engine labels Aug 31, 2022

N-o-Z requested review from nopcoder, itaiad200, eden-ohana and itaidavid August 31, 2022 12:43

N-o-Z self-assigned this Aug 31, 2022

itaiad200 approved these changes Aug 31, 2022

View reviewed changes

nopcoder suggested changes Sep 1, 2022

View reviewed changes

N-o-Z force-pushed the fix/tier-fs-evict-before-open-4061 branch from 45ec8b9 to 61311b8 Compare September 1, 2022 15:05

N-o-Z requested review from itaiad200 and nopcoder September 1, 2022 15:06

nopcoder reviewed Sep 1, 2022

View reviewed changes

itaidavid reviewed Sep 1, 2022

View reviewed changes

itaidavid suggested changes Sep 1, 2022

View reviewed changes

N-o-Z force-pushed the fix/tier-fs-evict-before-open-4061 branch from 493a979 to dba5ab8 Compare September 2, 2022 10:58

N-o-Z requested review from nopcoder and itaidavid September 2, 2022 10:58

itaiad200 requested changes Sep 2, 2022

View reviewed changes

nopcoder suggested changes Sep 2, 2022

View reviewed changes

N-o-Z requested a review from nopcoder September 2, 2022 12:01

nopcoder approved these changes Sep 2, 2022

View reviewed changes

itaidavid reviewed Sep 4, 2022

View reviewed changes

pkg/pyramid/file_tracker_test.go Outdated Show resolved Hide resolved

itaidavid reviewed Sep 4, 2022

View reviewed changes

pkg/pyramid/file_tracker_test.go

var testFileMap map[string]bool

func TestTracker(t *testing.T) {

Copy link

Contributor

itaidavid Sep 4, 2022

Choose a reason for hiding this comment

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

Awesome 🤩

itaidavid approved these changes Sep 4, 2022

View reviewed changes

N-o-Z force-pushed the fix/tier-fs-evict-before-open-4061 branch from 6d505b8 to 330efb3 Compare September 4, 2022 08:44

N-o-Z requested a review from itaiad200 September 5, 2022 09:52

itaiad200 requested changes Sep 5, 2022

View reviewed changes

N-o-Z force-pushed the fix/tier-fs-evict-before-open-4061 branch from 330efb3 to ae40f62 Compare September 5, 2022 11:47

N-o-Z requested a review from itaiad200 September 5, 2022 11:56

itaiad200 requested changes Sep 5, 2022

View reviewed changes

N-o-Z and others added 7 commits September 6, 2022 11:00

Fix: Pyramid delete before open

b8693ce

Second attempt

ca84030

Fixes 1

e159020

Fixes 2

09f4064

Fixes 3

a9ab0cc

Fixes 4

1d8096c

Update pkg/pyramid/file_tracker_test.go

a12a92b

Co-authored-by: itai-david <90712874+itai-david@users.noreply.github.com>

N-o-Z force-pushed the fix/tier-fs-evict-before-open-4061 branch from c3bbb2c to a12a92b Compare September 6, 2022 08:00

itaiad200 approved these changes Sep 6, 2022

View reviewed changes

N-o-Z added include-changelog PR description should be included in next release changelog and removed exclude-changelog PR description should not be included in next release changelog labels Sep 6, 2022

N-o-Z merged commit d1873a0 into master Sep 6, 2022

N-o-Z deleted the fix/tier-fs-evict-before-open-4061 branch September 6, 2022 09:54

		rcb := tfs.fileLocker.lock(fileRef.fsRelativePath)
		defer rcb() // release callback

		@@ -256,19 +261,12 @@ func (tfs TierFS) openFile(ctx context.Context, fileRef localFileRef, fh os.Fi
		}

		if !tfs.eviction.Store(fileRef.fsRelativePath, stat.Size()) {


		var testFileMap map[string]bool

		func TestTracker(t *testing.T) {

Fix: Pyramid delete before open #4062

Fix: Pyramid delete before open #4062

Conversation

N-o-Z commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaidavid left a comment

Choose a reason for hiding this comment

N-o-Z commented Sep 2, 2022

itaiad200 left a comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaidavid left a comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Sep 5, 2022

Choose a reason for hiding this comment

itaiad200 commented Sep 5, 2022

itaiad200 left a comment

Choose a reason for hiding this comment