Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix concurrency issue in multi-config #5646

Merged
merged 2 commits into from
Apr 10, 2021
Merged

fix concurrency issue in multi-config #5646

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
189edbe
fix concurency issue in multi-config
gsquared94 Apr 8, 2021
cff5987
add log info messages
gsquared94 Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/skaffold/build/builder_mux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
// set mux concurrency to be the minimum of all builders' concurrency. (concurrency = 0 means unlimited)
minConcurrency = concurrency
}
Expand Down
42 changes: 26 additions & 16 deletions pkg/skaffold/build/builder_mux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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 {
Expand All @@ -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)
})
}
}
Expand Down Expand Up @@ -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")
}
Expand Down
9 changes: 6 additions & 3 deletions pkg/skaffold/schema/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 9 additions & 5 deletions pkg/skaffold/schema/defaults/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
5 changes: 4 additions & 1 deletion pkg/skaffold/schema/versions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +489 to +491
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we instead pushed this to the local builder itself, so that it determined the default concurrency at build time?

(And isn't this duplicating what is being done in the call to withLocalBuild() in defaults.go?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a function in the test file that sets up the expected config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought about pushing this to local.Builder but it doesn't maintain the []*latest.Artifact slice and it seemed more convoluted to first set the default to 1 but later ignore it in the method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just change this method in the local builder to return 1?

skaffold/pkg/skaffold/build/local/local.go

Lines 60 to 65 in 89b8315

func (b *Builder) Concurrency() int {
if b.local.Concurrency == nil {
return 0
}
return *b.local.Concurrency
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't set it in default.Set then we'll need a reference to the []*latest.Artifact per config in local.Builder to decide if it should return 0 or 1 by default (0 if len(artifacts) == 0, otherwise 1). That plumbing seems more convoluted that this fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't clue in that this definition was in a test — I thought it was the implementation of withLocalBuilder in pkg/skaffold/schema/defaults/ 🤦

cfg.Build = b
}
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/skaffold/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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://")
}
Expand Down