fix concurency issue in multi-config

GoogleContainerTools · Apr 8, 2021 · 189edbe · 189edbe
1 parent b496731
commit 189edbe
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 26 deletions.
diff --git a/pkg/skaffold/build/builder_mux.go b/pkg/skaffold/build/builder_mux.go
@@ -57,7 +57,7 @@ func NewBuilderMux(cfg Config, store ArtifactStore, builder func(p latest.Pipeli
 		concurrency := b.Concurrency()
 		if minConcurrency < 0 {
 			minConcurrency = concurrency
-		} else if concurrency > 0 && concurrency < minConcurrency {
+		} else if concurrency > 0 && (minConcurrency == 0 || concurrency < minConcurrency) {
 			// set mux concurrency to be the minimum of all builders' concurrency. (concurrency = 0 means unlimited)
 			minConcurrency = concurrency
 		}

diff --git a/pkg/skaffold/build/builder_mux_test.go b/pkg/skaffold/build/builder_mux_test.go
@@ -23,24 +23,27 @@ import (
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
 func TestNewBuilderMux(t *testing.T) {
 	tests := []struct {
-		description      string
-		pipelines        []latest.Pipeline
-		pipeBuilder      func(latest.Pipeline) (PipelineBuilder, error)
-		shouldErr        bool
-		expectedBuilders []string
+		description         string
+		pipelines           []latest.Pipeline
+		pipeBuilder         func(latest.Pipeline) (PipelineBuilder, error)
+		shouldErr           bool
+		expectedBuilders    []string
+		expectedConcurrency int
 	}{
 		{
 			description: "only local builder",
 			pipelines: []latest.Pipeline{
-				{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{}}}},
+				{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{Concurrency: util.IntPtr(1)}}}},
 			},
-			pipeBuilder:      newMockPipelineBuilder,
-			expectedBuilders: []string{"local"},
+			pipeBuilder:         newMockPipelineBuilder,
+			expectedBuilders:    []string{"local"},
+			expectedConcurrency: 1,
 		},
 		{
 			description: "only cluster builder",
@@ -59,13 +62,15 @@ func TestNewBuilderMux(t *testing.T) {
 			expectedBuilders: []string{"gcb"},
 		},
 		{
-			description: "multiple builders",
+			description: "min non-zero concurrency",
 			pipelines: []latest.Pipeline{
-				{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{}}}},
-				{Build: latest.BuildConfig{BuildType: latest.BuildType{Cluster: &latest.ClusterDetails{}}}},
+				{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{Concurrency: util.IntPtr(0)}}}},
+				{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{Concurrency: util.IntPtr(3)}}}},
+				{Build: latest.BuildConfig{BuildType: latest.BuildType{Cluster: &latest.ClusterDetails{Concurrency: 2}}}},
 			},
-			pipeBuilder:      newMockPipelineBuilder,
-			expectedBuilders: []string{"local", "cluster"},
+			pipeBuilder:         newMockPipelineBuilder,
+			expectedBuilders:    []string{"local", "local", "cluster"},
+			expectedConcurrency: 2,
 		},
 	}
 	for _, test := range tests {
@@ -81,6 +86,7 @@ func TestNewBuilderMux(t *testing.T) {
 			for i := range b.builders {
 				t.CheckDeepEqual(test.expectedBuilders[i], b.builders[i].(*mockPipelineBuilder).builderType)
 			}
+			t.CheckDeepEqual(test.expectedConcurrency, b.concurrency)
 		})
 	}
 }
@@ -113,11 +119,15 @@ func (m *mockPipelineBuilder) Prune(context.Context, io.Writer) error { return n
 func newMockPipelineBuilder(p latest.Pipeline) (PipelineBuilder, error) {
 	switch {
 	case p.Build.BuildType.LocalBuild != nil:
-		return &mockPipelineBuilder{builderType: "local"}, nil
+		c := 0
+		if p.Build.LocalBuild.Concurrency != nil {
+			c = *p.Build.LocalBuild.Concurrency
+		}
+		return &mockPipelineBuilder{builderType: "local", concurrency: c}, nil
 	case p.Build.BuildType.Cluster != nil:
-		return &mockPipelineBuilder{builderType: "cluster"}, nil
+		return &mockPipelineBuilder{builderType: "cluster", concurrency: p.Build.Cluster.Concurrency}, nil
 	case p.Build.BuildType.GoogleCloudBuild != nil:
-		return &mockPipelineBuilder{builderType: "gcb"}, nil
+		return &mockPipelineBuilder{builderType: "gcb", concurrency: p.Build.GoogleCloudBuild.Concurrency}, nil
 	default:
 		return nil, errors.New("invalid config")
 	}

diff --git a/pkg/skaffold/schema/defaults/defaults.go b/pkg/skaffold/schema/defaults/defaults.go
@@ -75,9 +75,12 @@ func Set(c *latest.SkaffoldConfig) error {
 		}
 	}
 
-	withLocalBuild(c,
-		setDefaultConcurrency,
-	)
+	withLocalBuild(c, func(lb *latest.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,

diff --git a/pkg/skaffold/schema/defaults/defaults_test.go b/pkg/skaffold/schema/defaults/defaults_test.go
@@ -309,13 +309,17 @@ func TestSetDefaultsOnCloudBuild(t *testing.T) {
 }
 
 func TestSetDefaultsOnLocalBuild(t *testing.T) {
-	cfg := &latest.SkaffoldConfig{}
-
-	err := Set(cfg)
-	SetDefaultDeployer(cfg)
+	cfg1 := &latest.SkaffoldConfig{Pipeline: latest.Pipeline{Build: latest.BuildConfig{}}}
+	cfg2 := &latest.SkaffoldConfig{Pipeline: latest.Pipeline{Build: latest.BuildConfig{Artifacts: []*latest.Artifact{{ImageName: "foo"}}}}}
 
+	err := Set(cfg1)
+	testutil.CheckError(t, false, err)
+	SetDefaultDeployer(cfg1)
+	testutil.CheckDeepEqual(t, latest.LocalBuild{}, *cfg1.Build.LocalBuild)
+	err = Set(cfg2)
 	testutil.CheckError(t, false, err)
-	testutil.CheckDeepEqual(t, 1, *cfg.Build.LocalBuild.Concurrency)
+	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
@@ -482,10 +482,13 @@ func format(t *testutil.T, configs []string, apiVersions []string) string {
 
 func withLocalBuild(ops ...func(*latest.BuildConfig)) func(*latest.SkaffoldConfig) {
 	return func(cfg *latest.SkaffoldConfig) {
-		b := latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{Concurrency: &constants.DefaultLocalConcurrency}}}
+		b := latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{}}}
 		for _, op := range ops {
 			op(&b)
 		}
+		if len(b.Artifacts) > 0 {
+			b.LocalBuild.Concurrency = &constants.DefaultLocalConcurrency
+		}
 		cfg.Build = b
 	}
 }

diff --git a/pkg/skaffold/util/util.go b/pkg/skaffold/util/util.go
@@ -142,6 +142,12 @@ func StringPtr(s string) *string {
 	return &o
 }
 
+// IntPtr returns a pointer to an int
+func IntPtr(i int) *int {
+	o := i
+	return &o
+}
+
 func IsURL(s string) bool {
 	return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://")
 }