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

File locking module data store #3139

Merged
merged 45 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
c4cad75
Add test for concurrent cache reads + writes
doriable Jul 2, 2024
d4eb36a
Implement file locking on module.yaml in for ModuleDataStore
doriable Jul 3, 2024
a9dffdd
Skip test
doriable Jul 8, 2024
584b8f1
Fix lint
doriable Jul 8, 2024
f1953ef
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 8, 2024
c40e2fa
Updates from conversations + comments
doriable Jul 8, 2024
e9589be
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 8, 2024
89a076d
Close moduleDir before other delete operations
doriable Jul 9, 2024
0e49421
Refactor test away from errgroup
doriable Jul 9, 2024
812da0c
Use a lock file instead of locking on module.yaml
doriable Jul 9, 2024
caaad59
Check module.yaml during PutModules to avoid writing extra data
doriable Jul 9, 2024
1ac3afd
Fix delete invalid to use storage APIs
doriable Jul 9, 2024
ad470e5
Add locker options for filelock.Locker
doriable Jul 9, 2024
05ea4bb
Address comments + fix delete in delete invalid files
doriable Jul 10, 2024
7332a7d
Remove extra comment
doriable Jul 10, 2024
e7bf3cd
Only avoid calling deleteInvalidModuleData if module.yaml was not found
doriable Jul 10, 2024
2b73dc4
Address small comments
doriable Jul 11, 2024
cbf7021
Change `filelocker` as a required arg and adjust tests.
doriable Jul 11, 2024
427e0b5
Remove deleteInvalidModuleData
doriable Jul 11, 2024
f146b60
Add storageos test to module_data_store_test.go
doriable Jul 11, 2024
d162eec
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 11, 2024
8829be0
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 11, 2024
1f6c0d2
Small clean-up
doriable Jul 11, 2024
7dfa33e
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 15, 2024
c44c7ee
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 17, 2024
3916950
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 18, 2024
e98467d
Separate dir for file locking
doriable Jul 18, 2024
6a9416a
Apply suggestions from code review
doriable Jul 25, 2024
b52c371
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Jul 25, 2024
4266bd0
Delete lock files
doriable Jul 25, 2024
d4340a7
Address comments
doriable Jul 25, 2024
baa4f31
Fix lint
doriable Jul 25, 2024
1939643
Remove nil checks for filelocker and use NopLocker when passed nil in…
doriable Jul 25, 2024
ede95e9
Merge branch 'main' into file-locking-module-data-store
bufdev Aug 1, 2024
d5d9178
Address some comments
doriable Aug 1, 2024
5c3baa2
Fix tests
doriable Aug 1, 2024
44f3966
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Aug 8, 2024
2e46398
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Aug 13, 2024
7fa96d1
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Aug 21, 2024
a6c476a
Address comments
doriable Aug 21, 2024
c520685
Unexpand dockerignore glob
doriable Aug 21, 2024
723e85c
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Aug 21, 2024
61a38bb
Add in-line comments to document module read/write processes.
doriable Aug 21, 2024
fbd040f
Merge remote-tracking branch 'origin/main' into file-locking-module-d…
doriable Aug 21, 2024
4598d8c
Add comment explaining module.yaml as the source of validity at the m…
doriable Aug 21, 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
16 changes: 16 additions & 0 deletions private/buf/bufcli/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulestore"
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/filelock"
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
)
Expand Down Expand Up @@ -96,6 +97,12 @@ var (
//
// Normalized.
v3CacheWKTRelDirPath = normalpath.Join("v3", "wellknowntypes")
// v3CacheModuleLockRelDirPath is the relative path to the lock files directory for module data. This path
// is relative to the files cache directory for module data and is used to store lock files for
// synchronizing reading and writing module data from the cache.
//
// Noramlized.
doriable marked this conversation as resolved.
Show resolved Hide resolved
v3CacheModuleLockRelDirPath = "lock"
doriable marked this conversation as resolved.
Show resolved Hide resolved
doriable marked this conversation as resolved.
Show resolved Hide resolved
)

// NewModuleDataProvider returns a new ModuleDataProvider while creating the
Expand Down Expand Up @@ -166,12 +173,21 @@ func newModuleDataProvider(
if err != nil {
return nil, err
}
// Create filelocker directory and filelocker
doriable marked this conversation as resolved.
Show resolved Hide resolved
if err := createCacheDir(container.CacheDirPath(), normalpath.Join(v3CacheModuleRelDirPath, v3CacheModuleLockRelDirPath)); err != nil {
doriable marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
filelocker, err := filelock.NewLocker(normalpath.Join(fullCacheDirPath, v3CacheModuleLockRelDirPath))
doriable marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
return bufmodulecache.NewModuleDataProvider(
container.Logger(),
delegateModuleDataProvider,
bufmodulestore.NewModuleDataStore(
container.Logger(),
cacheBucket,
filelocker,
),
), nil
}
Expand Down
doriable marked this conversation as resolved.
Show resolved Hide resolved
Empty file.
72 changes: 71 additions & 1 deletion private/bufpkg/bufmodule/bufmodulecache/bufmodulecache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,23 @@ package bufmodulecache

import (
"context"
"fmt"
"os"
"path/filepath"
"testing"
"time"

"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulestore"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduletesting"
"github.com/bufbuild/buf/private/pkg/filelock"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/storage/storagemem"
"github.com/bufbuild/buf/private/pkg/storage/storageos"
"github.com/bufbuild/buf/private/pkg/thread"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
)

func TestCommitProviderForModuleKeyBasic(t *testing.T) {
Expand Down Expand Up @@ -165,6 +173,7 @@ func TestModuleDataProviderBasic(t *testing.T) {
bufmodulestore.NewModuleDataStore(
zap.NewNop(),
storagemem.NewReadWriteBucket(),
filelock.NewNopLocker(),
),
)

Expand Down Expand Up @@ -214,6 +223,64 @@ func TestModuleDataProviderBasic(t *testing.T) {
)
}

func TestConcurrentCacheReadWrite(t *testing.T) {
t.Parallel()

bsrProvider, moduleKeys := testGetBSRProviderAndModuleKeys(t, context.Background())
tempDir := t.TempDir()
cacheDir := filepath.Join(tempDir, "cache")
logger := zaptest.NewLogger(t, zaptest.Level(zap.InfoLevel))

for i := 0; i < 20; i++ {
require.NoError(t, os.MkdirAll(cacheDir, 0755))
jobs, err := slicesext.MapError(
[]int{0, 1, 2, 3, 4},
func(i int) (func(ctx context.Context) error, error) {
logger := logger.Named(fmt.Sprintf("job-%d", i))
bucket, err := storageos.NewProvider().NewReadWriteBucket(cacheDir)
if err != nil {
return nil, err
}
filelocker, err := filelock.NewLocker(
cacheDir,
filelock.LockerWithLockRetryDelay(10*time.Millisecond), // Drops test time from ~16s to ~1s
)
if err != nil {
return nil, err
}
cacheProvider := newModuleDataProvider(
logger,
bsrProvider,
bufmodulestore.NewModuleDataStore(
logger,
bucket,
filelocker,
),
)
return func(ctx context.Context) error {
moduleDatas, err := cacheProvider.GetModuleDatasForModuleKeys(
ctx,
moduleKeys,
)
if err != nil {
return err
}
for _, moduleData := range moduleDatas {
// Calling moduleData.Bucket() checks the digest
if _, err := moduleData.Bucket(); err != nil {
return err
}
}
return nil
}, nil
},
)
require.NoError(t, err)
require.NoError(t, thread.Parallelize(context.Background(), jobs))
require.NoError(t, os.RemoveAll(cacheDir))
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to do this, and it's somewhat unsafe - temporary directories are automatically cleaned up.

Copy link
Member

Choose a reason for hiding this comment

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

This is a subdirectory of the temp dir created by the test and we want to clear it after each attempt (to start fresh). This exposed race conditions that wouldn't be hit once we managed to successfully cache everything during an attempt.

}
}

func testGetBSRProviderAndModuleKeys(t *testing.T, ctx context.Context) (bufmoduletesting.OmniProvider, []bufmodule.ModuleKey) {
bsrProvider, err := bufmoduletesting.NewOmniProvider(
bufmoduletesting.ModuleData{
Expand All @@ -235,7 +302,10 @@ func testGetBSRProviderAndModuleKeys(t *testing.T, ctx context.Context) (bufmodu
bufmoduletesting.ModuleData{
Name: "buf.build/foo/mod3",
PathToData: map[string][]byte{
"mod3.proto": []byte(
"mod3a.proto": []byte(
pkwarren marked this conversation as resolved.
Show resolved Hide resolved
`syntax = proto3; package mod3;`,
),
"mod3b.proto": []byte(
`syntax = proto3; package mod3;`,
),
},
Expand Down
Loading