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.

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

Add negative cache entry after deleting object/folderCache entry update #2822

Merged
merged 7 commits into from
Dec 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 11 additions & 4 deletions internal/storage/caching/fast_stat_bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,18 +367,25 @@ 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)
codechanges marked this conversation as resolved.
Show resolved Hide resolved
} else {
b.addNegativeEntry(req.Name)
}
return
}

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
}

Expand Down
9 changes: 6 additions & 3 deletions internal/storage/caching/fast_stat_bucket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()).
Expand Down Expand Up @@ -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)

Expand All @@ -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")))

Expand Down
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
Expand Up @@ -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)
Expand All @@ -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) {
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Up @@ -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
codechanges marked this conversation as resolved.
Show resolved Hide resolved
flagsSet := [][]string{
{"--kernel-list-cache-ttl-secs=-1"},
{"--kernel-list-cache-ttl-secs=-1", "--metadata-cache-ttl-secs=0"},
codechanges marked this conversation as resolved.
Show resolved Hide resolved
}

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

"github.com/google/uuid"
)

func VerifyExpectedSubstrings(t *testing.T, input string, expectedSubstrings []string) {
Expand All @@ -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
Expand Up @@ -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 {
codechanges marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
Loading