fix: ignore concurrency if not specified

GoogleContainerTools · Mar 11, 2022 · f0af476 · f0af476
1 parent aa2d9b9
commit f0af476
Show file tree

Hide file tree

Showing 12 changed files with 36 additions and 64 deletions.
diff --git a/integration/testdata/diagnose/multi-config/diagnose.tmpl b/integration/testdata/diagnose/multi-config/diagnose.tmpl
@@ -10,8 +10,7 @@ build:
       dockerfile: Dockerfile
   tagPolicy:
     gitCommit: {}
-  local:
-    concurrency: 1
+  local: {}
 deploy:
   kubectl:
     manifests:
@@ -31,8 +30,7 @@ build:
       dockerfile: Dockerfile
   tagPolicy:
     gitCommit: {}
-  local:
-    concurrency: 1
+  local: {}
 deploy:
   kubectl:
     manifests:
@@ -50,8 +48,7 @@ build:
       dockerfile: Dockerfile
   tagPolicy:
     gitCommit: {}
-  local:
-    concurrency: 1
+  local: {}
 deploy:
   kubectl:
     manifests:

diff --git a/integration/testdata/diagnose/temp-config/diagnose.tmpl b/integration/testdata/diagnose/temp-config/diagnose.tmpl
@@ -8,8 +8,7 @@ build:
       dockerfile: Dockerfile
   tagPolicy:
     gitCommit: {}
-  local:
-    concurrency: 1
+  local: {}
 deploy:
   kubectl:
     manifests:

diff --git a/pkg/skaffold/build/build.go b/pkg/skaffold/build/build.go
@@ -51,7 +51,7 @@ type PipelineBuilder interface {
 	PostBuild(ctx context.Context, out io.Writer) error
 
 	// Concurrency specifies the max number of builds that can run at any one time. If concurrency is 0, then all builds can run in parallel.
-	Concurrency() int
+	Concurrency() *int
 
 	// Prune removes images built in this pipeline
 	Prune(context.Context, io.Writer) error

diff --git a/pkg/skaffold/build/builder_mux.go b/pkg/skaffold/build/builder_mux.go
@@ -22,6 +22,7 @@ import (
 	"io"
 	"reflect"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/hooks"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output/log"
@@ -65,20 +66,29 @@ func NewBuilderMux(cfg Config, store ArtifactStore, builder func(p latestV1.Pipe
 
 		if cfg.BuildConcurrency() >= 0 {
 			minConcurrency = cfg.BuildConcurrency()
-		} else {
-			concurrency := b.Concurrency()
-			// set mux concurrency to be the minimum of all builders' concurrency. (concurrency = 0 means unlimited)
-			switch {
-			case minConcurrency < 0:
-				minConcurrency = concurrency
-				log.Entry(context.TODO()).Infof("build concurrency first set to %d parsed from %s[%d]", minConcurrency, reflect.TypeOf(b).String(), i)
-			case concurrency > 0 && (minConcurrency == 0 || concurrency < minConcurrency):
-				minConcurrency = concurrency
-				log.Entry(context.TODO()).Infof("build concurrency updated to %d parsed from %s[%d]", minConcurrency, reflect.TypeOf(b).String(), i)
-			default:
-				log.Entry(context.TODO()).Infof("build concurrency value %d parsed from %s[%d] is ignored since it's not less than previously set value %d", concurrency, reflect.TypeOf(b).String(), i, minConcurrency)
-			}
+			continue
 		}
+
+		if b.Concurrency() == nil {
+			continue
+		}
+		concurrency := *b.Concurrency()
+
+		// set mux concurrency to be the minimum of all builders' concurrency. (concurrency = 0 means unlimited)
+		switch {
+		case minConcurrency < 0:
+			minConcurrency = concurrency
+			log.Entry(context.TODO()).Infof("build concurrency first set to %d parsed from %s[%d]", minConcurrency, reflect.TypeOf(b).String(), i)
+		case concurrency > 0 && (minConcurrency == 0 || concurrency < minConcurrency):
+			minConcurrency = concurrency
+			log.Entry(context.TODO()).Infof("build concurrency updated to %d parsed from %s[%d]", minConcurrency, reflect.TypeOf(b).String(), i)
+		default:
+			log.Entry(context.TODO()).Infof("build concurrency value %d parsed from %s[%d] is ignored since it's not less than previously set value %d", concurrency, reflect.TypeOf(b).String(), i, minConcurrency)
+		}
+	}
+	if minConcurrency < 0 {
+		minConcurrency = constants.DefaultLocalConcurrency // set default concurrency to 1.
+		log.Entry(context.TODO()).Infof("build concurrency set to default value of %d", minConcurrency)
 	}
 	log.Entry(context.TODO()).Infof("final build concurrency value is %d", minConcurrency)
 

diff --git a/pkg/skaffold/build/builder_mux_test.go b/pkg/skaffold/build/builder_mux_test.go
@@ -121,7 +121,7 @@ func (m *mockPipelineBuilder) Build(ctx context.Context, out io.Writer, artifact
 
 func (m *mockPipelineBuilder) PostBuild(ctx context.Context, out io.Writer) error { return nil }
 
-func (m *mockPipelineBuilder) Concurrency() int { return m.concurrency }
+func (m *mockPipelineBuilder) Concurrency() *int { return util.IntPtr(m.concurrency) }
 
 func (m *mockPipelineBuilder) Prune(context.Context, io.Writer) error { return nil }
 

diff --git a/pkg/skaffold/build/cluster/cluster.go b/pkg/skaffold/build/cluster/cluster.go
@@ -70,8 +70,8 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *la
 	return build.TagWithDigest(tag, digest), nil
 }
 
-func (b *Builder) Concurrency() int {
-	return b.ClusterDetails.Concurrency
+func (b *Builder) Concurrency() *int {
+	return util.IntPtr(b.ClusterDetails.Concurrency)
 }
 
 func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latestV1.Artifact, tag string, platforms platform.Matcher) (string, error) {

diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go
@@ -44,6 +44,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/platform"
 	latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/sources"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/proto/v1"
 )
 
@@ -65,8 +66,8 @@ func (b *Builder) PostBuild(_ context.Context, _ io.Writer) error {
 	return nil
 }
 
-func (b *Builder) Concurrency() int {
-	return b.GoogleCloudBuild.Concurrency
+func (b *Builder) Concurrency() *int {
+	return util.IntPtr(b.GoogleCloudBuild.Concurrency)
 }
 
 func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, artifact *latestV1.Artifact, tag string, platform platform.Matcher) (string, error) {

diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go
@@ -58,12 +58,7 @@ func (b *Builder) PostBuild(ctx context.Context, _ io.Writer) error {
 	return nil
 }
 
-func (b *Builder) Concurrency() int {
-	if b.local.Concurrency == nil {
-		return 0
-	}
-	return *b.local.Concurrency
-}
+func (b *Builder) Concurrency() *int { return b.local.Concurrency }
 
 func (b *Builder) PushImages() bool {
 	return b.pushImages

diff --git a/pkg/skaffold/parser/config_test.go b/pkg/skaffold/parser/config_test.go
@@ -70,16 +70,11 @@ func createCfg(name string, imageName string, workspace string, requires []lates
 			Artifacts: []*latestV1.Artifact{{ImageName: imageName, ArtifactType: latestV1.ArtifactType{
 				DockerArtifact: &latestV1.DockerArtifact{DockerfilePath: "Dockerfile"}}, Workspace: workspace}}, TagPolicy: latestV1.TagPolicy{
 				GitTagger: &latestV1.GitTagger{}}, BuildType: latestV1.BuildType{
-				LocalBuild: &latestV1.LocalBuild{Concurrency: concurrency()},
+				LocalBuild: &latestV1.LocalBuild{},
 			}}, Deploy: latestV1.DeployConfig{Logs: latestV1.LogsConfig{Prefix: "container"}}},
 	}
 }
 
-func concurrency() *int {
-	c := 1
-	return &c
-}
-
 type document struct {
 	path    string
 	configs []mockCfg

diff --git a/pkg/skaffold/schema/defaults/defaults.go b/pkg/skaffold/schema/defaults/defaults.go
@@ -76,13 +76,6 @@ func Set(c *latestV1.SkaffoldConfig) error {
 		}
 	}
 
-	withLocalBuild(c, func(lb *latestV1.LocalBuild) {
-		// don't set build concurrency if there are no artifacts in the current config
-		if len(c.Build.Artifacts) > 0 {
-			setDefaultConcurrency(lb)
-		}
-	})
-
 	withCloudBuildConfig(c,
 		setDefaultCloudBuildDockerImage,
 		setDefaultCloudBuildMavenImage,
@@ -136,20 +129,6 @@ func defaultToKubectlDeploy(c *latestV1.SkaffoldConfig) {
 	c.Deploy.DeployType.KubectlDeploy = &latestV1.KubectlDeploy{}
 }
 
-func withLocalBuild(c *latestV1.SkaffoldConfig, operations ...func(*latestV1.LocalBuild)) {
-	if local := c.Build.LocalBuild; local != nil {
-		for _, operation := range operations {
-			operation(local)
-		}
-	}
-}
-
-func setDefaultConcurrency(local *latestV1.LocalBuild) {
-	if local.Concurrency == nil {
-		local.Concurrency = &constants.DefaultLocalConcurrency
-	}
-}
-
 func withCloudBuildConfig(c *latestV1.SkaffoldConfig, operations ...func(*latestV1.GoogleCloudBuild)) {
 	if gcb := c.Build.GoogleCloudBuild; gcb != nil {
 		for _, operation := range operations {

diff --git a/pkg/skaffold/schema/defaults/defaults_test.go b/pkg/skaffold/schema/defaults/defaults_test.go
@@ -319,7 +319,6 @@ func TestSetDefaultsOnLocalBuild(t *testing.T) {
 	err = Set(cfg2)
 	testutil.CheckError(t, false, err)
 	SetDefaultDeployer(cfg2)
-	testutil.CheckDeepEqual(t, 1, *cfg2.Build.LocalBuild.Concurrency)
 }
 
 func TestSetPortForwardLocalPort(t *testing.T) {

diff --git a/pkg/skaffold/schema/versions_test.go b/pkg/skaffold/schema/versions_test.go
@@ -493,9 +493,6 @@ func withLocalBuild(ops ...func(*latestV1.BuildConfig)) func(*latestV1.SkaffoldC
 		for _, op := range ops {
 			op(&b)
 		}
-		if len(b.Artifacts) > 0 {
-			b.LocalBuild.Concurrency = &constants.DefaultLocalConcurrency
-		}
 		cfg.Build = b
 	}
 }