engine: record seen cache volumes at resolver level #5786
Conversation
marcosnils
force-pushed
the
fix/sync_seen_cache_volumes
branch
from
e315ea4
to
3806a27
Compare
marcosnils
force-pushed
the
fix/sync_seen_cache_volumes
branch
3 times, most recently
from
4f9d1f0
to
a1e35f2
Compare
marcosnils
requested review from
vito and
aluzzardi
and removed request for
a team
| allCacheMounts[k.(string)] = struct{}{} | ||
| return true | ||
| }) | ||
|
|
||
| for _, syncedCacheMount := range syncedCacheMounts { |
There was a problem hiding this comment.
do we still want to support manually synced mounts? I guess that's for backward compat / avoiding regressions. Should probably put a fat warning that we should remove this?
There was a problem hiding this comment.
actually now that you mention about it I think we can remove this altogether without any regressions. IIUC the current code will always call the stopCacheMountSync regardless if the volume if used or not. Now that we can detect which volumes the engine ever used during its lifetime, seems more accurate to only sync those. WDYT?
core/container.go
Outdated
| @@ -560,6 +561,8 @@ func (container *Container) WithMountedFile(ctx context.Context, bk *buildkit.Cl | |||
| return container.withMounted(ctx, bk, target, file.LLB, file.File, file.Services, owner) | |||
| } | |||
|
|
|||
| var SeenCacheKeys sync.Map | |||
There was a problem hiding this comment.
nit: I believe by convention in the codebase, this should be achieved by passing something in core/schema, and having that something passed again to containerSchema (just like the leaseManager, buildCache, etc) rather than using a global variable.
/cc @vito who added the most in there recently
There was a problem hiding this comment.
I looked into this and the thing is that I'd have to pass this "something" across multiple layers so it gets all the way to the schema package. Thing is that the CacheManager defined here (https://github.com/marcosnils/dagger/blob/a1e35f20a546f00c4e4fcc13581b1ab833a1640d/cmd/engine/main.go#L760) which has the mount synchronization logic a methods gets passed as a solver.CacheManager here (https://github.com/marcosnils/dagger/blob/a1e35f20a546f00c4e4fcc13581b1ab833a1640d/cmd/engine/main.go#L802) to the buildkit controller and there's not to many things we could do from there. Additionally, the cache mount synchronization process happens in the main package here (https://github.com/marcosnils/dagger/blob/a1e35f20a546f00c4e4fcc13581b1ab833a1640d/cmd/engine/main.go#L802) which doesn't have way to access the schema or core structs to fetch the seen cache mount keys 🤷
There was a problem hiding this comment.
I think this is OK as a special-case since this is really an engine-wide concern. We'll see if anything changes once we figure out cache namespacing/etc.
marcosnils
force-pushed
the
fix/sync_seen_cache_volumes
branch
from
a1e35f2
to
4c8f803
Compare
This PR tracks the seen cache volumes at the resolver level so they can be automatically discovered for syncronization at the engine start / stop time. This solution isn't ideal since it should be getting the cache mounts from buildkit's store instead of the resolver but we couldn't find a way to make it work with @aluzzardi. We're falling back to this workaround until maybe @sipsma can shed some light. Signed-off-by: Marcos Lilljedahl <marcosnils@gmail.com>
marcosnils
force-pushed
the
fix/sync_seen_cache_volumes
branch
from
4c8f803
to
5dbb86d
Compare
Co-authored-by: Alex Suraci <suraci.alex@gmail.com> Signed-off-by: Marcos Nils <1578458+marcosnils@users.noreply.github.com>
this is a follow-up of dagger#5786 where we have introduced automatic discovery of cache mounts so it's not needed to check if the list is empty now. Signed-off-by: Marcos Lilljedahl <marcosnils@gmail.com>
this is a follow-up of #5786 where we have introduced automatic discovery of cache mounts so it's not needed to check if the list is empty now. Signed-off-by: Marcos Lilljedahl <marcosnils@gmail.com>
Fixes https://linear.app/dagger/issue/DEV-2629/automatically-create-cache-volumes-in-cloud-derived-from-pipelines
This PR tracks the seen cache volumes at the resolver level so they
can be automatically discovered for syncronization at the engine start
/ stop time. This solution isn't ideal since it should be getting the
cache mounts from buildkit's store instead of the resolver but we
couldn't find a way to make it work with @aluzzardi. We're falling back
to this workaround until maybe @sipsma can shed some light.
Signed-off-by: Marcos Lilljedahl marcosnils@gmail.com