From 490f4c4b52cece9638fdbb1c10c090b37a3d2b41 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 27 Sep 2024 20:03:10 -0600 Subject: [PATCH 1/7] Enable previously flaky test and cleanup --- component/file_cache/file_cache_test.go | 15 +-------------- component/file_cache/lru_policy.go | 15 +++++---------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index c1779153e..e134875a1 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -871,12 +871,7 @@ func (suite *fileCacheTestSuite) TestOpenFileNotInCache() { time.Sleep(10 * time.Millisecond) _, err = os.Stat(filepath.Join(suite.cache_path, path)) } - // TODO: find out why this delayed eviction check fails in CI on Windows sometimes - if runtime.GOOS == "windows" { - fmt.Println("Skipping TestOpenFileNotInCache eviction check on Windows (flaky)") - } else { - suite.assert.True(os.IsNotExist(err)) - } + suite.assert.True(os.IsNotExist(err)) handle, err = suite.fileCache.OpenFile(internal.OpenFileOptions{Name: path, Flags: os.O_RDWR, Mode: suite.fileCache.defaultPermission}) suite.assert.NoError(err) @@ -1365,7 +1360,6 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() { src := "source4" dst := "destination4" createHandle, _ := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: src, Mode: 0666}) - openHandle, _ := suite.fileCache.OpenFile(internal.OpenFileOptions{Name: src, Mode: 0666}) suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: createHandle}) // Path should be in the file cache @@ -1382,8 +1376,6 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() { suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist suite.assert.FileExists(suite.fake_storage_path + "/" + dst) // Dst does exist - suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: openHandle}) - time.Sleep(1 * time.Second) // Check once before the cache cleanup that file exists suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall exists in cache @@ -1393,11 +1385,6 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() { func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { defer suite.cleanupTest() - suite.cleanupTest() - - config := fmt.Sprintf("file_cache:\n path: %s\n offload-io: true\n timeout-sec: 0\n\nloopbackfs:\n path: %s", - suite.cache_path, suite.fake_storage_path) - suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual) src := "source5" dst := "destination5" diff --git a/component/file_cache/lru_policy.go b/component/file_cache/lru_policy.go index 4a9e34bf3..9799625c3 100644 --- a/component/file_cache/lru_policy.go +++ b/component/file_cache/lru_policy.go @@ -74,9 +74,6 @@ type lruPolicy struct { } const ( - // Check for file expiry in below number of seconds - CacheTimeoutCheckInterval = 5 - // Check for disk usage in below number of minutes DiskUsageCheckInterval = 1 ) @@ -126,13 +123,10 @@ func (p *lruPolicy) StartPolicy() error { p.diskUsageMonitor = time.Tick(time.Duration(DiskUsageCheckInterval * time.Minute)) } - // Only start the timeoutMonitor if evictTime is non-zero. - // If evictTime=0, we delete on invalidate so there is no need for a timeout monitor signal to be sent. log.Info("lruPolicy::StartPolicy : Policy set with %v timeout", p.cacheTimeout) - if p.cacheTimeout != 0 { - p.cacheTimeoutMonitor = time.Tick(time.Duration(time.Duration(p.cacheTimeout) * time.Second)) - } + // if timeout is zero time.Tick will return nil + p.cacheTimeoutMonitor = time.Tick(time.Duration(time.Duration(p.cacheTimeout) * time.Second)) go p.clearCache() go p.asyncCacheValid() @@ -352,18 +346,19 @@ func (p *lruPolicy) updateMarker() { p.Lock() node := p.lastMarker + // remove lastMarker from linked list if node.next != nil { node.next.prev = node.prev } - if node.prev != nil { node.prev.next = node.next } + // and insert it at the head node.prev = nil node.next = p.head p.head.prev = node p.head = node - + // swap lastMarker with currMarker p.lastMarker = p.currMarker p.currMarker = node From 376f8197dd67fdbddf7fc95e19916906e122c02c Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 27 Sep 2024 21:32:57 -0600 Subject: [PATCH 2/7] Shorten test times a bit further --- component/file_cache/file_cache_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index e134875a1..6007eb10e 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -936,7 +936,7 @@ func (suite *fileCacheTestSuite) TestCloseFile() { func (suite *fileCacheTestSuite) TestCloseFileTimeout() { defer suite.cleanupTest() suite.cleanupTest() // teardown the default file cache generated - cacheTimeout := 5 + cacheTimeout := 1 config := fmt.Sprintf("file_cache:\n path: %s\n offload-io: true\n timeout-sec: %d\n\nloopbackfs:\n path: %s", suite.cache_path, cacheTimeout, suite.fake_storage_path) suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual) @@ -1353,7 +1353,7 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() { defer suite.cleanupTest() suite.cleanupTest() - config := fmt.Sprintf("file_cache:\n path: %s\n offload-io: true\n timeout-sec: 2\n\nloopbackfs:\n path: %s", + config := fmt.Sprintf("file_cache:\n path: %s\n offload-io: true\n timeout-sec: 1\n\nloopbackfs:\n path: %s", suite.cache_path, suite.fake_storage_path) suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual) @@ -1376,10 +1376,10 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() { suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist suite.assert.FileExists(suite.fake_storage_path + "/" + dst) // Dst does exist - time.Sleep(1 * time.Second) // Check once before the cache cleanup that file exists + time.Sleep(500 * time.Millisecond) // Check once before the cache cleanup that file exists suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall exists in cache - time.Sleep(2 * time.Second) // Wait for the cache cleanup to occur + time.Sleep(1 * time.Second) // Wait for the cache cleanup to occur suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall not exists in cache } From 8d7e60036d09b687daedad40502fc7e2e8b2e582 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 27 Sep 2024 22:12:20 -0600 Subject: [PATCH 3/7] Re-enable tests that used to be flaky --- component/file_cache/file_cache_test.go | 4 +--- component/file_cache/file_cache_windows_test.go | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index 6007eb10e..403d36d8c 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -1247,9 +1247,7 @@ func (suite *fileCacheTestSuite) TestGetAttrCase4() { time.Sleep(10 * time.Millisecond) _, err = os.Stat(filepath.Join(suite.cache_path, file)) } - // TODO: why is check test flaky (on both platforms)? - fmt.Println("Skipping TestGetAttrCase4 eviction check (flaky).") - // suite.assert.True(os.IsNotExist(err)) + suite.assert.True(os.IsNotExist(err)) // open the file in parallel and try getting the size of file while open is on going go suite.fileCache.OpenFile(internal.OpenFileOptions{Name: file, Mode: 0666}) diff --git a/component/file_cache/file_cache_windows_test.go b/component/file_cache/file_cache_windows_test.go index 29b83af4f..ad62f1957 100644 --- a/component/file_cache/file_cache_windows_test.go +++ b/component/file_cache/file_cache_windows_test.go @@ -120,9 +120,7 @@ func (suite *fileCacheWindowsTestSuite) TestChownNotInCache() { time.Sleep(10 * time.Millisecond) _, err = os.Stat(suite.cache_path + "/" + path) } - // this check is flaky in our CI pipeline on Windows, so skip it - fmt.Println("Skipping TestChownNotInCache IsNotExist check on Windows because it's flaky.") - // suite.assert.True(os.IsNotExist(err)) + suite.assert.True(os.IsNotExist(err)) // Path should be in fake storage suite.assert.FileExists(suite.fake_storage_path + "/" + path) From a3d677fb5b60d9e5cb38f94d3e808cb41e688acd Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Mon, 30 Sep 2024 16:12:14 -0600 Subject: [PATCH 4/7] Test IsCached to make sure it is false when we expect it to be, since it no longer returns false positives. --- component/file_cache/file_cache_test.go | 5 ++++- component/file_cache/lru_policy.go | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index 403d36d8c..f52064a03 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -1369,6 +1369,7 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() { err := suite.fileCache.RenameFile(internal.RenameFileOptions{Src: src, Dst: dst}) suite.assert.NoError(err) // Path in fake storage and file cache should be updated + suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, src))) suite.assert.NoFileExists(suite.cache_path + "/" + src) // Src does not exist suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall exists in cache suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist @@ -1399,6 +1400,7 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { err := suite.fileCache.RenameFile(internal.RenameFileOptions{Src: src, Dst: dst}) suite.assert.NoError(err) // Path in fake storage and file cache should be updated + suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, src))) suite.assert.NoFileExists(suite.cache_path + "/" + src) // Src does not exist suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall exists in cache suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist @@ -1406,7 +1408,8 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: openHandle}) - time.Sleep(100 * time.Millisecond) // Wait for the cache cleanup to occur + time.Sleep(100 * time.Millisecond) // Wait for the cache cleanup to occur + suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, dst))) suite.assert.NoFileExists(suite.cache_path + "/" + dst) // Dst shall not exists in cache } diff --git a/component/file_cache/lru_policy.go b/component/file_cache/lru_policy.go index 9799625c3..ce003c402 100644 --- a/component/file_cache/lru_policy.go +++ b/component/file_cache/lru_policy.go @@ -183,8 +183,6 @@ func (p *lruPolicy) CachePurge(name string) { p.deleteEvent <- name } -// Due to a race condition, this may return a false positive, -// but it will not return a false negative. func (p *lruPolicy) IsCached(name string) bool { log.Trace("lruPolicy::IsCached : %s", name) From ee21e15311d22387c46f8798365b7acebb0c6f82 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Mon, 30 Sep 2024 17:48:47 -0600 Subject: [PATCH 5/7] Remove race condition caused by holding an open handle. --- component/file_cache/file_cache_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index f52064a03..263f2d446 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -1399,6 +1399,8 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { // RenameFile err := suite.fileCache.RenameFile(internal.RenameFileOptions{Src: src, Dst: dst}) suite.assert.NoError(err) + + suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: openHandle}) // Path in fake storage and file cache should be updated suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, src))) suite.assert.NoFileExists(suite.cache_path + "/" + src) // Src does not exist @@ -1406,8 +1408,6 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist suite.assert.FileExists(suite.fake_storage_path + "/" + dst) // Dst does exist - suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: openHandle}) - time.Sleep(100 * time.Millisecond) // Wait for the cache cleanup to occur suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, dst))) suite.assert.NoFileExists(suite.cache_path + "/" + dst) // Dst shall not exists in cache From 3e53b4c7844bddd9ff390df7b6ad7f42f7e866ae Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Mon, 30 Sep 2024 17:58:34 -0600 Subject: [PATCH 6/7] Don't try to fight the zero timeout with an open handle. It just causes even more race conditions. --- component/file_cache/file_cache_test.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index 263f2d446..da18fcbce 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -1387,30 +1387,27 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { src := "source5" dst := "destination5" - createHandle, _ := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: src, Mode: 0666}) - openHandle, _ := suite.fileCache.OpenFile(internal.OpenFileOptions{Name: src, Mode: 0666}) - suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: createHandle}) + handle, _ := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: src, Mode: 0666}) + suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: handle}) - // Path should be in the file cache - suite.assert.FileExists(suite.cache_path + "/" + src) - // Path should be in fake storage + // Path _might_ be in the file cache + // Path _should_ be in fake storage suite.assert.FileExists(suite.fake_storage_path + "/" + src) // RenameFile err := suite.fileCache.RenameFile(internal.RenameFileOptions{Src: src, Dst: dst}) suite.assert.NoError(err) - suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: openHandle}) - // Path in fake storage and file cache should be updated - suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, src))) - suite.assert.NoFileExists(suite.cache_path + "/" + src) // Src does not exist - suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall exists in cache + // Dst _might_ exist in cache, and the rename should be complete in fake storage suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist suite.assert.FileExists(suite.fake_storage_path + "/" + dst) // Dst does exist time.Sleep(100 * time.Millisecond) // Wait for the cache cleanup to occur + // cache should be completely clean + suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, src))) suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, dst))) - suite.assert.NoFileExists(suite.cache_path + "/" + dst) // Dst shall not exists in cache + suite.assert.NoFileExists(suite.cache_path + "/" + src) + suite.assert.NoFileExists(suite.cache_path + "/" + dst) } func (suite *fileCacheTestSuite) TestTruncateFileNotInCache() { From bccb85104d43031ff9db78c39b539f6e3209919d Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Mon, 30 Sep 2024 18:03:35 -0600 Subject: [PATCH 7/7] Use a better test name --- component/file_cache/file_cache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index da18fcbce..761896049 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -1437,7 +1437,7 @@ func (suite *fileCacheTestSuite) TestTruncateFileNotInCache() { suite.assert.EqualValues(info.Size(), size) } -func (suite *fileCacheTestSuite) TestTruncateFileInCache() { +func (suite *fileCacheTestSuite) TestTruncateFileCase3() { defer suite.cleanupTest() // Setup path := "file31"