Skip to content

Commit

Permalink
Add negative cache entry after deleting object/folderCache entry upda…
Browse files Browse the repository at this point in the history
…te (#2822)

* Add negative cache entry on deletion

* Add remaining changes

* Added TODO

* Added explanatory comments

* Fixed tests by adding a random directory

* Fixed tests by adding a random directory

* Rename dirName variable
codechanges authored Dec 30, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 8f471b4 commit 6ae0e39
Showing 6 changed files with 60 additions and 21 deletions.
15 changes: 11 additions & 4 deletions internal/storage/caching/fast_stat_bucket.go
Original file line number Diff line number Diff line change
@@ -367,8 +367,12 @@ func (b *fastStatBucket) UpdateObject(
func (b *fastStatBucket) DeleteObject(
ctx context.Context,
req *gcs.DeleteObjectRequest) (err error) {
b.invalidate(req.Name)
err = b.wrapped.DeleteObject(ctx, req)
if err != nil {
b.invalidate(req.Name)
} else {
b.addNegativeEntry(req.Name)
}
return
}

@@ -391,11 +395,14 @@ func (b *fastStatBucket) MoveObject(ctx context.Context, req *gcs.MoveObjectRequ

func (b *fastStatBucket) DeleteFolder(ctx context.Context, folderName string) error {
err := b.wrapped.DeleteFolder(ctx, folderName)
// In case of an error; invalidate the cached entry. This will make sure that
// gcsfuse is not caching possibly erroneous status of the folder and next
// call will hit GCS backend to probe the latest status.
if err != nil {
return err
b.invalidate(folderName)
} else {
b.addNegativeEntryForFolder(folderName)
}
// TODO: Caching negative entries for both objects and folders will be implemented together due to test failures.
b.invalidate(folderName)
return err
}

9 changes: 6 additions & 3 deletions internal/storage/caching/fast_stat_bucket_test.go
Original file line number Diff line number Diff line change
@@ -906,8 +906,8 @@ func (t *DeleteObjectTest) WrappedSucceeds() {
const name = ""
var err error

// Erase
ExpectCall(t.cache, "Erase")(Any())
// AddNegativeEntry
ExpectCall(t.cache, "AddNegativeEntry")(Any(), Any())

// Wrapped
ExpectCall(t.wrapped, "DeleteObject")(Any(), Any()).
@@ -1003,9 +1003,10 @@ func init() { RegisterTestSuite(&DeleteFolderTest{}) }

func (t *DeleteFolderTest) Test_DeleteFolder_Success() {
const name = "some-name"
ExpectCall(t.cache, "AddNegativeEntryForFolder")(name, Any()).
WillOnce(Return())
ExpectCall(t.wrapped, "DeleteFolder")(Any(), name).
WillOnce(Return(nil))
ExpectCall(t.cache, "Erase")(name).WillOnce(Return())

err := t.bucket.DeleteFolder(context.TODO(), name)

@@ -1014,6 +1015,8 @@ func (t *DeleteFolderTest) Test_DeleteFolder_Success() {

func (t *DeleteFolderTest) Test_DeleteFolder_Failure() {
const name = "some-name"
// Erase
ExpectCall(t.cache, "Erase")(Any())
ExpectCall(t.wrapped, "DeleteFolder")(Any(), name).
WillOnce(Return(fmt.Errorf("mock error")))

29 changes: 16 additions & 13 deletions tools/integration_tests/implicit_dir/local_file_test.go
Original file line number Diff line number Diff line change
@@ -37,21 +37,23 @@ var (
// //////////////////////////////////////////////////////////////////////

func TestNewFileUnderImplicitDirectoryShouldNotGetSyncedToGCSTillClose(t *testing.T) {
testDirPath = setup.SetupTestDirectory(testDirName)
CreateImplicitDir(ctx, storageClient, testDirName, t)
testBaseDirName := path.Join(testDirName, operations.GetRandomName(t))
testDirPath = setup.SetupTestDirectoryRecursive(testBaseDirName)
CreateImplicitDir(ctx, storageClient, testBaseDirName, t)
fileName := path.Join(ImplicitDirName, FileName1)

_, fh := CreateLocalFileInTestDir(ctx, storageClient, testDirPath, fileName, t)
operations.WriteWithoutClose(fh, FileContents, t)
ValidateObjectNotFoundErrOnGCS(ctx, storageClient, testDirName, fileName, t)
ValidateObjectNotFoundErrOnGCS(ctx, storageClient, testBaseDirName, fileName, t)

// Validate.
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh, testDirName, fileName, FileContents, t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh, testBaseDirName, fileName, FileContents, t)
}

func TestReadDirForImplicitDirWithLocalFile(t *testing.T) {
testDirPath = setup.SetupTestDirectory(testDirName)
CreateImplicitDir(ctx, storageClient, testDirName, t)
testBaseDirName := path.Join(testDirName, operations.GetRandomName(t))
testDirPath = setup.SetupTestDirectoryRecursive(testBaseDirName)
CreateImplicitDir(ctx, storageClient, testBaseDirName, t)
fileName1 := path.Join(ImplicitDirName, FileName1)
fileName2 := path.Join(ImplicitDirName, FileName2)
_, fh1 := CreateLocalFileInTestDir(ctx, storageClient, testDirPath, fileName1, t)
@@ -66,8 +68,8 @@ func TestReadDirForImplicitDirWithLocalFile(t *testing.T) {
operations.VerifyFileEntry(entries[1], FileName2, 0, t)
operations.VerifyFileEntry(entries[2], ImplicitFileName1, GCSFileSize, t)
// Close the local files.
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh1, testDirName, fileName1, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh2, testDirName, fileName2, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh1, testBaseDirName, fileName1, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh2, testBaseDirName, fileName2, "", t)
}

func TestRecursiveListingWithLocalFiles(t *testing.T) {
@@ -80,7 +82,8 @@ func TestRecursiveListingWithLocalFiles(t *testing.T) {
// mntDir/implicit/foo2 --- file
// mntDir/implicit/implicitFile1 --- file

testDirPath = setup.SetupTestDirectory(testDirName)
testBaseDirName := path.Join(testDirName, operations.GetRandomName(t))
testDirPath = setup.SetupTestDirectoryRecursive(testBaseDirName)
fileName2 := path.Join(ExplicitDirName, ExplicitFileName1)
fileName3 := path.Join(ImplicitDirName, FileName2)
// Create local file in mnt/ dir.
@@ -89,7 +92,7 @@ func TestRecursiveListingWithLocalFiles(t *testing.T) {
operations.CreateDirectory(path.Join(testDirPath, ExplicitDirName), t)
_, fh2 := CreateLocalFileInTestDir(ctx, storageClient, testDirPath, fileName2, t)
// Create implicit dir with 1 local file1 and 1 synced file.
CreateImplicitDir(ctx, storageClient, testDirName, t)
CreateImplicitDir(ctx, storageClient, testBaseDirName, t)
_, fh3 := CreateLocalFileInTestDir(ctx, storageClient, testDirPath, fileName3, t)

// Recursively list mntDir/ directory.
@@ -135,7 +138,7 @@ func TestRecursiveListingWithLocalFiles(t *testing.T) {
if err != nil {
t.Errorf("filepath.WalkDir() err: %v", err)
}
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh1, testDirName, FileName1, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh2, testDirName, fileName2, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh3, testDirName, fileName3, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh1, testBaseDirName, FileName1, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh2, testBaseDirName, fileName2, "", t)
CloseFileAndValidateContentFromGCS(ctx, storageClient, fh3, testBaseDirName, fileName3, "", t)
}
Original file line number Diff line number Diff line change
@@ -526,8 +526,12 @@ func TestInfiniteKernelListCacheTest(t *testing.T) {
}

// Define flag set to run the tests.
// Note: metadata cache is disabled to avoid cache consistency issue between
// gcsfuse cache and kernel cache. As gcsfuse cache might hold the entry which
// already became stale due to delete operation.
// TODO: Replace metadata-cache-ttl-secs with something better
flagsSet := [][]string{
{"--kernel-list-cache-ttl-secs=-1"},
{"--kernel-list-cache-ttl-secs=-1", "--metadata-cache-ttl-secs=0"},
}

// Run tests.
10 changes: 10 additions & 0 deletions tools/integration_tests/util/operations/string_operations.go
Original file line number Diff line number Diff line change
@@ -18,6 +18,8 @@ package operations
import (
"strings"
"testing"

"github.com/google/uuid"
)

func VerifyExpectedSubstrings(t *testing.T, input string, expectedSubstrings []string) {
@@ -35,3 +37,11 @@ func VerifyUnexpectedSubstrings(t *testing.T, input string, unexpectedSubstrings
}
}
}

func GetRandomName(t *testing.T) string {
id, err := uuid.NewRandom()
if err != nil {
t.Errorf("Error while generating random string, err: %v", err)
}
return id.String()
}
12 changes: 12 additions & 0 deletions tools/integration_tests/util/setup/setup.go
Original file line number Diff line number Diff line change
@@ -382,6 +382,18 @@ func SetupTestDirectory(testDirName string) string {
return testDirPath
}

// SetupTestDirectoryRecursive recursively creates a testDirectory in the mounted directory and cleans up
// any content present in it.
func SetupTestDirectoryRecursive(testDirName string) string {
testDirPath := path.Join(MntDir(), testDirName)
err := os.MkdirAll(testDirPath, DirPermission_0755)
if err != nil && !strings.Contains(err.Error(), "file exists") {
log.Printf("Error while setting up directory %s for testing: %v", testDirPath, err)
}
CleanUpDir(testDirPath)
return testDirPath
}

// CleanupDirectoryOnGCS cleans up the object/directory path passed in parameter.
func CleanupDirectoryOnGCS(ctx context.Context, client *storage.Client, directoryPathOnGCS string) {
bucket, dirPath := GetBucketAndObjectBasedOnTypeOfMount(directoryPathOnGCS)

0 comments on commit 6ae0e39

Please sign in to comment.