Fix 5301: Build dependencies for sync inherited from required artifacts; cache build dependencies between devloops
#5614
Conversation
gsquared94
requested review from
chanseokoh,
loosebazooka and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5614 +/- ##
==========================================
- Coverage 70.66% 70.64% -0.03%
==========================================
Files 408 408
Lines 15559 15581 +22
==========================================
+ Hits 10995 11007 +12
- Misses 3757 3764 +7
- Partials 807 810 +3
Continue to review full report at Codecov.
|
pkg/skaffold/runner/new.go
Outdated
| @@ -62,9 +62,12 @@ func NewForConfig(runCtx *runcontext.RunContext) (*SkaffoldRunner, error) { | |||
| } | |||
|
|
|||
| store := build.NewArtifactStore() | |||
| graph := build.ToArtifactGraph(runCtx.Artifacts()) | |||
| depsResolver := build.GetDependenciesResolver(runCtx, store, graph) | |||
There was a problem hiding this comment.
The depsResolver contains the store and bCtx contains a reference to both store and depResolver
Can we make it simpler by adding the method GetArtifactStore on DependencyResolver interface vs on bCtx?
IIUC, right now
<Runner>
|
--------------------
| |
<ArtifactStore> <DependencyResolver>
| ^
--------------------|
There was a problem hiding this comment.
The methods are somewhat unrelated though: DependencyResolver is actually the source-dependency-resolver which returns the list of files to watch for any artifact. ArtifactStore returns the latest tag for any built image. They could be under a BuildContext object but there's no good place to put this object without running into circular dependencies trying to import it across the different build env and the different builder implementations.
Right now there is a single buildCtx struct that implements both these interfaces but it's local to the runner package whereas the interface methods are exposed in the Config parameter of every builder setup.
I'll take another stab at it to merge these constructs.
also cc @briandealwis for his similar comments.
There was a problem hiding this comment.
Methods added to BuildContext. Kept ArtifactStore and SourceDependencies as separate interfaces still.
There was a problem hiding this comment.
High level comments:
- I think the
DependencyResolverwould be better named to indicate that it is a cache:DependencyCache - I find myself confused by dependency-as-artifact-dependency and dependency-as-artifact-sources. I'd be inclined to rename dependency-as-artifact-dependency to requires? Or otherwise making explicitly indicating dependency-as-artifact-sources as source-dependencies
- picking up on @tejal29's comment: it would seem to make sense to have a build-context that included the dependencies cache and artifact store
pkg/skaffold/build/dependencies.go
Outdated
| var getDependenciesFunc = dependenciesForSingleArtifact | ||
|
|
||
| type DependencyResolver interface { | ||
| DependenciesForArtifact(ctx context.Context, a *latest.Artifact) ([]string, error) |
There was a problem hiding this comment.
Does *latest.Artifact need to be a pointer? Do we do anything destructive?
There was a problem hiding this comment.
The convention in our codebase seems to be to pass around pointers for all SkaffoldConfig objects so I followed the same. Otherwise there's no need for this to be a pointer.
|
@tejal29 @briandealwis I've implemented the |
|
just for info: i'm now using this PR instead of the old one on our setup and everything works fine! |
cache build dependencies between devloops
Fixes: #5301 #5522
Related: #5374
Description
Currently the build dependencies for any artifact are evaluated on every file change trigger. In #5301, we need to recursively evaluate the build dependencies for all artifacts in the transitive closure of artifacts reachable from the target via its
artifact-dependencieslist. For the worst case contrived example, there could be an exponential number of build dependency evaluations for any artifact in a single devloop. So we additionally introduce a cache that gets cleared on the start of every devloop. This ensures that for the duration of a single devloop the build dependencies won't be reevaluated.Can be tested by running against this repro for 5301
PS: We could optimize further by resetting the cache only when the FS notify trigger is of type file added, since file modification or deletion won't change the result of the build dependencies from the previous devloop. However, that wouldn't work with polling or manual trigger types.