Skip to content

Conversation

@dleviminzi
Copy link
Contributor

@dleviminzi dleviminzi commented Apr 25, 2025

Resolve BE-2569

Summary by mrge

Switched file caching to use S3 as the source instead of FUSE, improving reliability and simplifying the caching process.

  • Refactors
    • Updated cache logic to pull files and images directly from S3.
    • Simplified related code and updated dependencies.

Requires beam-cloud/blobcache-v2#49
beam-cloud/geesefs#5

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

mrge found 7 issues across 7 files. View them in mrge.io

ForcePathStyle: c.config.ImageService.Registries.S3.Primary.ForcePathStyle,
}, blobcache.StoreContentOptions{
CreateCacheFSEntry: true,
RoutingKey: sourcePath,
Copy link
Contributor

Choose a reason for hiding this comment

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

The sourcePath construction is changed but still used as the routing key, which might cause inconsistency with previously cached items

nodeFilters:
- server:0
- volume: $PWD/.k3d/storage:k3s-storage
- volume: $HOME/.beta9-k3d/storage:k3s-storage
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably merge main into this branch. I think you already merged this change in right?

client := c.fileCacheManager.GetClient()

// Remove the leading "/" from the checkpoint path
sourcePath := checkpointPath[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think this // Remove the leading "/" from the checkpoint path is necessary anymore. But maybe leave it for now.

operation := func() error {
baseBlobFsContentPath := fmt.Sprintf("%s/%s", baseFileCachePath, sourcePath)
if _, err := os.Stat(baseBlobFsContentPath); err == nil && c.cacheClient.IsPathCachedNearby(ctx, "/"+sourcePath) {
if _, err := os.Stat(baseBlobFsContentPath); err == nil && c.cacheClient.IsPathCachedNearby(ctx, "/"+imageKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would triple check that the routing key and the image key are exactly the same. This caused an issue in prod last night because the routing key and path passed here were different. I think below you're using the sourcePath (the full s3 qualifier), and up here in the filesystem you're using the image key.

@dleviminzi dleviminzi requested a review from luke-lombardi May 1, 2025 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants