Fix for limited parallelism when the target is the same but has diffe…

…rent args (#3406)

Fixes #2377

The previous `VisitedCollection` implementation attempts to wait for
ongoing targets with the same name to complete before comparing the
inputs of the target. This has the advantage of being more precise with
which ARGs actually influence the outcome of the targets, ignoring
overriding ARGs that end up being unused within those targets. But it
also has the disadvantage that all targets with the same name (but
different overriding args) execute sequentially.

This new implementation simplifies this greatly, by computing the target
input hash upfront based on all overriding args, without knowing if
there are any args that will end up being unused. The result is that we
are able to run all targets with the same name but different args in
parallel. But it also has the disadvantage that in some cases we would
create duplicated LLBs for some targets when certain overriding args are
different but they are unused. Buildkit will generally de-duplicate the
LLB in these cases, although it's possible that there might be edge
cases if the LLB construction is not consistent.

---------

Co-authored-by: nacho <[email protected]>
Co-authored-by: Vlad A. Ionescu <[email protected]>

.earthly_version_flag_overrides

@@ -1 +1 @@
-referenced-save-only,use-copy-include-patterns,earthly-version-arg,use-cache-command,use-host-command,check-duplicate-images,use-copy-link,parallel-load,shell-out-anywhere,new-platform,no-tar-build-output,wait-block
+referenced-save-only,use-copy-include-patterns,earthly-version-arg,use-cache-command,use-host-command,check-duplicate-images,use-copy-link,parallel-load,shell-out-anywhere,new-platform,no-tar-build-output,wait-block,use-visited-upfront-hash-collection

Earthfile

@@ -466,7 +466,7 @@ earthly-docker:
 # Otherwise, it will attempt to login to the docker hub mirror using the provided username and password
 earthly-integration-test-base:
     FROM +earthly-docker
-    RUN apk update && apk add pcre-tools curl python3 bash perl findutils expect yq
+    RUN apk update && apk add pcre-tools curl python3 bash perl findutils expect yq && apk add --upgrade sed
     COPY scripts/acbtest/acbtest scripts/acbtest/acbgrep /bin/
     ENV NO_DOCKER=1
     ENV NETWORK_MODE=host # Note that this breaks access to embedded registry in WITH DOCKER.

earthfile2llb/converter.go

@@ -124,7 +124,6 @@ func NewConverter(ctx context.Context, target domain.Target, bc *buildcontext.Da
 		Final:   sts,
 		Visited: opt.Visited,
 	}
-	sts.AddOverridingVarsAsBuildArgInputs(opt.OverridingVars)
 	newCollOpt := variables.NewCollectionOpt{
 		Console:          opt.Console,
 		Target:           target,

earthfile2llb/earthfile2llb.go

@@ -48,7 +48,7 @@ type ConvertOpt struct {
 	CleanCollection *cleanup.Collection
 	// Visited is a collection of target states which have been converted to LLB.
 	// This is used for deduplication and infinite cycle detection.
-	Visited *states.VisitedCollection
+	Visited states.VisitedCollection
 	// PlatformResolver is a platform resolver, which keeps track of
 	// the current platform, the native platform, the user platform, and
 	// the default platform.
@@ -132,6 +132,10 @@ type ConvertOpt struct {
 	// parentDepSub is a channel informing of any new dependencies from the parent.
 	parentDepSub chan string // chan of sts IDs.
 
+	// TargetInputHashStackSet is a set of target input hashes that are currently in the call stack.
+	// This is used to detect infinite cycles.
+	TargetInputHashStackSet map[string]bool
+
 	// ContainerFrontend is the currently used container frontend, as detected by Earthly at app start. It provides info
 	// and access to commands to manipulate the current container frontend.
 	ContainerFrontend containerutil.ContainerFrontend
@@ -189,12 +193,19 @@ func Earthfile2LLB(ctx context.Context, target domain.Target, opt ConvertOpt, in
 	if opt.SolveCache == nil {
 		opt.SolveCache = states.NewSolveCache()
 	}
-	if opt.Visited == nil {
-		opt.Visited = states.NewVisitedCollection()
-	}
 	if opt.MetaResolver == nil {
 		opt.MetaResolver = NewCachedMetaResolver(opt.GwClient)
 	}
+	if opt.TargetInputHashStackSet == nil {
+		opt.TargetInputHashStackSet = make(map[string]bool)
+	} else {
+		// We are in a recursive call. Copy the stack set.
+		newMap := make(map[string]bool)
+		for k, v := range opt.TargetInputHashStackSet {
+			newMap[k] = v
+		}
+		opt.TargetInputHashStackSet = newMap
+	}
 	egWait := false
 	if opt.ErrorGroup == nil {
 		opt.ErrorGroup, ctx = serrgroup.WithContext(ctx)
@@ -225,6 +236,14 @@ func Earthfile2LLB(ctx context.Context, target domain.Target, opt ConvertOpt, in
 		return nil, errors.Wrapf(err, "resolve build context for target %s", target.String())
 	}
 
+	if opt.Visited == nil {
+		if bc.Features.UseVisitedUpfrontHashCollection {
+			opt.Visited = states.NewVisitedUpfrontHashCollection()
+		} else {
+			opt.Visited = states.NewLegacyVisitedCollection()
+		}
+	}
+
 	opt.Features = bc.Features
 	if initialCall && !bc.Features.ReferencedSaveOnly {
 		opt.DoSaves = !target.IsRemote() // legacy mode only saves artifacts that are locally referenced
@@ -241,17 +260,21 @@ func Earthfile2LLB(ctx context.Context, target domain.Target, opt ConvertOpt, in
 	if err != nil {
 		return nil, err
 	}
-	if opt.MainTargetDetailsFunc != nil {
-		err := opt.MainTargetDetailsFunc(TargetDetails{
-			EarthlyOrgName:     bc.EarthlyOrgName,
-			EarthlyProjectName: bc.EarthlyProjectName,
-		})
-		if err != nil {
-			return nil, errors.Wrapf(err, "target details handler error: %v", err)
-		}
-		opt.MainTargetDetailsFunc = nil
+	tiHash, err := sts.TargetInput().Hash()
+	if err != nil {
+		return nil, err
 	}
 	if found {
+		if opt.TargetInputHashStackSet[tiHash] {
+			return nil, errors.Errorf("infinite cycle detected for target %s", target.String())
+		}
+		// Wait for the existing sts to complete first.
+		select {
+		case <-ctx.Done():
+			return nil, ctx.Err()
+		case <-sts.Done():
+		}
+
 		// The found target may have initially been created by a FROM or a COPY;
 		// however, if it is referenced a second time by a BUILD, it may contain items that
 		// require a save (export to the local host) or a push
@@ -275,6 +298,17 @@ func Earthfile2LLB(ctx context.Context, target domain.Target, opt ConvertOpt, in
 			Visited: opt.Visited,
 		}, nil
 	}
+	opt.TargetInputHashStackSet[tiHash] = true
+	if opt.MainTargetDetailsFunc != nil {
+		err := opt.MainTargetDetailsFunc(TargetDetails{
+			EarthlyOrgName:     bc.EarthlyOrgName,
+			EarthlyProjectName: bc.EarthlyProjectName,
+		})
+		if err != nil {
+			return nil, errors.Wrapf(err, "target details handler error: %v", err)
+		}
+		opt.MainTargetDetailsFunc = nil
+	}
 	converter, err := NewConverter(ctx, targetWithMetadata, bc, sts, opt)
 	if err != nil {
 		return nil, err

features/features.go

@@ -53,15 +53,16 @@ type Features struct {
 	WaitBlock                bool `long:"wait-block" description:"enable WITH/END feature, also allows RUN --push mixed with non-push commands"`
 
 	// unreleased
-	NoUseRegistryForWithDocker bool `long:"no-use-registry-for-with-docker" description:"disable use-registry-for-with-docker"`
-	TryFinally                 bool `long:"try" description:"allow the use of the TRY/FINALLY commands"`
-	NoNetwork                  bool `long:"no-network" description:"allow the use of RUN --network=none commands"`
-	ArgScopeSet                bool `long:"arg-scope-and-set" description:"enable SET to reassign ARGs and prevent ARGs from being redeclared in the same scope"`
-	EarthlyCIRunnerArg         bool `long:"earthly-ci-runner-arg" description:"includes EARTHLY_CI_RUNNER ARG"`
-	UseDockerIgnore            bool `long:"use-docker-ignore" description:"fallback to .dockerignore incase .earthlyignore or .earthignore do not exist in a local \"FROM DOCKERFILE\" target"`
-	PassArgs                   bool `long:"pass-args" description:"Allow the use of the --pass-arg flag in FROM, BUILD, COPY, WITH DOCKER, and DO commands"`
-	GlobalCache                bool `long:"global-cache" description:"enable global caches (shared across different Earthfiles), for cache mounts and CACHEs having an ID"`
-	GitRefs                    bool `long:"git-refs" description:"includes EARTHLY_GIT_REFS ARG"`
+	NoUseRegistryForWithDocker      bool `long:"no-use-registry-for-with-docker" description:"disable use-registry-for-with-docker"`
+	TryFinally                      bool `long:"try" description:"allow the use of the TRY/FINALLY commands"`
+	NoNetwork                       bool `long:"no-network" description:"allow the use of RUN --network=none commands"`
+	ArgScopeSet                     bool `long:"arg-scope-and-set" description:"enable SET to reassign ARGs and prevent ARGs from being redeclared in the same scope"`
+	EarthlyCIRunnerArg              bool `long:"earthly-ci-runner-arg" description:"includes EARTHLY_CI_RUNNER ARG"`
+	UseDockerIgnore                 bool `long:"use-docker-ignore" description:"fallback to .dockerignore incase .earthlyignore or .earthignore do not exist in a local \"FROM DOCKERFILE\" target"`
+	PassArgs                        bool `long:"pass-args" description:"Allow the use of the --pass-arg flag in FROM, BUILD, COPY, WITH DOCKER, and DO commands"`
+	GlobalCache                     bool `long:"global-cache" description:"enable global caches (shared across different Earthfiles), for cache mounts and CACHEs having an ID"`
+	GitRefs                         bool `long:"git-refs" description:"includes EARTHLY_GIT_REFS ARG"`
+	UseVisitedUpfrontHashCollection bool `long:"use-visited-upfront-hash-collection" description:"Uses a new target visitor implementation that computes upfront the hash of the visited targets and adds support for running all targets with the same name but different args in parallel"`
 
 	Major int
 	Minor int

states/dedup/targetinput.go

@@ -51,28 +51,6 @@ func (ti TargetInput) WithFilterBuildArgs(buildArgNames map[string]bool) TargetI
 	return tiClone
 }
 
-// Equals compares to another TargetInput for equality.
-func (ti TargetInput) Equals(other TargetInput) bool {
-	if ti.TargetCanonical != other.TargetCanonical {
-		return false
-	}
-	if ti.Platform != other.Platform {
-		return false
-	}
-	if ti.AllowPrivileged != other.AllowPrivileged {
-		return false
-	}
-	if len(ti.BuildArgs) != len(other.BuildArgs) {
-		return false
-	}
-	for index := range ti.BuildArgs {
-		if !ti.BuildArgs[index].Equals(other.BuildArgs[index]) {
-			return false
-		}
-	}
-	return true
-}
-
 func (ti TargetInput) clone() TargetInput {
 	tiCopy := TargetInput{
 		TargetCanonical: ti.TargetCanonical,
@@ -139,7 +117,8 @@ type BuildArgInput struct {
 	// ConstantValue is the constant value of this build arg.
 	ConstantValue string `json:"constantValue"`
 	// DefaultValue represents the default value of the build arg.
-	DefaultValue string `json:"defaultConstant"`
+	// Not used as part of the hashing.
+	DefaultValue string `json:"-"`
 }
 
 // IsDefaultValue returns whether the value of the BuildArgInput
@@ -159,9 +138,8 @@ func (bai BuildArgInput) Equals(other BuildArgInput) bool {
 	if bai.ConstantValue != other.ConstantValue {
 		return false
 	}
-	if bai.DefaultValue != other.DefaultValue {
-		return false
-	}
+	// Ignoring the default value comparison - it is not used for
+	// deduplication.
 	return true
 }
 

states/states.go

@@ -20,7 +20,7 @@ type MultiTarget struct {
 	// Visited represents the previously visited states, grouped by target
 	// name. Duplicate targets are possible if same target is called with different
 	// build args.
-	Visited *VisitedCollection
+	Visited VisitedCollection
 	// Final is the main target to be built.
 	Final *SingleTarget
 }
@@ -108,6 +108,11 @@ func newSingleTarget(ctx context.Context, target domain.Target, platr *platutil.
 		doneCh:                   make(chan struct{}),
 		incomingNewSubscriptions: make(chan string, 1024),
 	}
+	sts.addOverridingVarsAsBuildArgInputs(overridingVars)
+	if parentDepSub == nil {
+		// New simplified algorithm.
+		return sts, nil
+	}
 	// Consume all items from the parent subscription before returning control.
 OuterLoop:
 	for {
@@ -221,17 +226,6 @@ func (sts *SingleTarget) AddBuildArgInput(bai dedup.BuildArgInput) {
 	sts.targetInput = sts.targetInput.WithBuildArgInput(bai)
 }
 
-// AddOverridingVarsAsBuildArgInputs adds some vars to the sts's target input.
-func (sts *SingleTarget) AddOverridingVarsAsBuildArgInputs(overridingVars *variables.Scope) {
-	sts.tiMu.Lock()
-	defer sts.tiMu.Unlock()
-	for _, key := range overridingVars.Sorted() {
-		ovVar, _ := overridingVars.Get(key)
-		sts.targetInput = sts.targetInput.WithBuildArgInput(
-			dedup.BuildArgInput{ConstantValue: ovVar, Name: key})
-	}
-}
-
 // LastSaveImage returns the last save image available (if any).
 func (sts *SingleTarget) LastSaveImage() SaveImage {
 	if len(sts.SaveImages) == 0 {
@@ -297,6 +291,16 @@ func (sts *SingleTarget) Done() chan struct{} {
 	return sts.doneCh
 }
 
+func (sts *SingleTarget) addOverridingVarsAsBuildArgInputs(overridingVars *variables.Scope) {
+	sts.tiMu.Lock()
+	defer sts.tiMu.Unlock()
+	for _, key := range overridingVars.Sorted() {
+		ovVar, _ := overridingVars.Get(key)
+		sts.targetInput = sts.targetInput.WithBuildArgInput(
+			dedup.BuildArgInput{ConstantValue: ovVar, Name: key})
+	}
+}
+
 // SaveLocal is an artifact path to be saved to local disk.
 type SaveLocal struct {
 	// DestPath is the local dest path to copy the artifact to.