Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor improvements in file cache #333

Merged
merged 7 commits into from
Oct 11, 2024
Merged

Minor improvements in file cache #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
490f4c4
Enable previously flaky test and cleanup
foodprocessor Sep 28, 2024
376f819
Shorten test times a bit further
foodprocessor Sep 28, 2024
8d7e600
Re-enable tests that used to be flaky
foodprocessor Sep 28, 2024
a3d677f
Test IsCached to make sure it is false when we expect it to be, since…
foodprocessor Sep 30, 2024
ee21e15
Remove race condition caused by holding an open handle.
foodprocessor Sep 30, 2024
3e53b4c
Don't try to fight the zero timeout with an open handle. It just caus…
foodprocessor Sep 30, 2024
bccb851
Use a better test name
foodprocessor Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 20 additions & 35 deletions component/file_cache/file_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -941,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)
Expand Down Expand Up @@ -1252,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)?
Ferelith-maker marked this conversation as resolved.
Show resolved Hide resolved
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})
Expand Down Expand Up @@ -1358,14 +1351,13 @@ 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)

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
Expand All @@ -1377,52 +1369,45 @@ 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
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
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
}

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"
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)
// Path in fake storage and file cache should be updated
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

suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: openHandle})

time.Sleep(100 * time.Millisecond) // Wait for the cache cleanup to occur
suite.assert.NoFileExists(suite.cache_path + "/" + dst) // Dst shall not exists in cache
time.Sleep(100 * time.Millisecond) // Wait for the cache cleanup to occur
Dabnsky marked this conversation as resolved.
Show resolved Hide resolved
// 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 + "/" + src)
suite.assert.NoFileExists(suite.cache_path + "/" + dst)
}

func (suite *fileCacheTestSuite) TestTruncateFileNotInCache() {
Expand Down Expand Up @@ -1452,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"
Expand Down
4 changes: 1 addition & 3 deletions component/file_cache/file_cache_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
17 changes: 5 additions & 12 deletions component/file_cache/lru_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,6 @@ type lruPolicy struct {
}

const (
// Check for file expiry in below number of seconds
CacheTimeoutCheckInterval = 5
Dabnsky marked this conversation as resolved.
Show resolved Hide resolved

// Check for disk usage in below number of minutes
DiskUsageCheckInterval = 1
)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -189,8 +183,6 @@ func (p *lruPolicy) CachePurge(name string) {
p.deleteEvent <- name
}

// Due to a race condition, this may return a false positive,
Ferelith-maker marked this conversation as resolved.
Show resolved Hide resolved
// but it will not return a false negative.
func (p *lruPolicy) IsCached(name string) bool {
log.Trace("lruPolicy::IsCached : %s", name)

Expand Down Expand Up @@ -352,18 +344,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

Expand Down
Loading