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

build: infer platform from first node if none set #2131

Merged
merged 4 commits into from
Dec 12, 2023
Merged

build: infer platform from first node if none set #2131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa0aeac
build: move solve opt out of duplicate map
jedevc Nov 28, 2023
aac7a47
build: fix incorrect solve opt platform from being set
jedevc Nov 28, 2023
5403231
build: infer platform from first node if none set
crazy-max Nov 28, 2023
9d8ac1c
build: move solveOpt to local struct type
crazy-max Dec 4, 2023
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
37 changes: 22 additions & 15 deletions build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ type NamedContext struct {
State *llb.State
}

type reqForNode struct {
*resolvedNode
so *client.SolveOpt
}

func filterAvailableNodes(nodes []builder.Node) ([]builder.Node, error) {
out := make([]builder.Node, 0, len(nodes))
err := errors.Errorf("no drivers found")
Expand Down Expand Up @@ -509,10 +514,6 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
if err != nil {
return nil, err
}
driversSolveOpts := make(map[string][]*client.SolveOpt, len(drivers))
for k, dps := range drivers {
driversSolveOpts[k] = make([]*client.SolveOpt, len(dps))
}

defers := make([]func(), 0, 2)
defer func() {
Expand All @@ -523,6 +524,7 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
}
}()

reqForNodes := make(map[string][]*reqForNode)
eg, ctx := errgroup.WithContext(ctx)

for k, opt := range opt {
Expand All @@ -532,7 +534,8 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
if err != nil {
logrus.WithError(err).Warn("current commit information was not captured by the build")
}
for i, np := range drivers[k] {
var reqn []*reqForNode
for _, np := range drivers[k] {
if np.Node().Driver.IsMobyDriver() {
hasMobyDriver = true
}
Expand All @@ -552,8 +555,12 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
so.FrontendAttrs[k] = v
}
defers = append(defers, release)
driversSolveOpts[k][i] = so
reqn = append(reqn, &reqForNode{
resolvedNode: np,
so: so,
})
}
reqForNodes[k] = reqn
for _, at := range opt.Session {
if s, ok := at.(interface {
SetLogger(progresswriter.Logger)
Expand All @@ -566,8 +573,8 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s

// validate for multi-node push
if hasMobyDriver && multiDriver {
for _, so := range driversSolveOpts[k] {
for _, e := range so.Exports {
for _, np := range reqForNodes[k] {
for _, e := range np.so.Exports {
if e.Type == "moby" {
if ok, _ := strconv.ParseBool(e.Attrs["push"]); ok {
return nil, errors.Errorf("multi-node push can't currently be performed with the docker driver, please switch to a different driver")
Expand All @@ -580,9 +587,9 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s

// validate that all links between targets use same drivers
for name := range opt {
dps := drivers[name]
dps := reqForNodes[name]
for i, dp := range dps {
so := driversSolveOpts[name][i]
so := reqForNodes[name][i].so
for k, v := range so.FrontendAttrs {
if strings.HasPrefix(k, "context:") && strings.HasPrefix(v, "target:") {
k2 := strings.TrimPrefix(v, "target:")
Expand Down Expand Up @@ -610,7 +617,7 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
results := waitmap.New()

multiTarget := len(opt) > 1
childTargets := calculateChildTargets(drivers, driversSolveOpts, opt)
childTargets := calculateChildTargets(reqForNodes, opt)

for k, opt := range opt {
err := func(k string) error {
Expand All @@ -634,7 +641,7 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
for i, dp := range dps {
i, dp := i, dp
node := dp.Node()
so := driversSolveOpts[k][i]
so := reqForNodes[k][i].so
if multiDriver {
for i, e := range so.Exports {
switch e.Type {
Expand Down Expand Up @@ -1303,12 +1310,12 @@ func resultKey(index int, name string) string {
}

// calculateChildTargets returns all the targets that depend on current target for reverse index
func calculateChildTargets(drivers map[string][]*resolvedNode, driversSolveOpts map[string][]*client.SolveOpt, opt map[string]Options) map[string][]string {
func calculateChildTargets(reqs map[string][]*reqForNode, opt map[string]Options) map[string][]string {
out := make(map[string][]string)
for name := range opt {
dps := drivers[name]
dps := reqs[name]
for i, dp := range dps {
so := driversSolveOpts[name][i]
so := reqs[name][i].so
for k, v := range so.FrontendAttrs {
if strings.HasPrefix(k, "context:") && strings.HasPrefix(v, "target:") {
target := resultKey(dp.driverIndex, strings.TrimPrefix(v, "target:"))
Expand Down
21 changes: 14 additions & 7 deletions build/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,6 @@ func (r *nodeResolver) resolve(ctx context.Context, ps []specs.Platform, pw prog
return nil, true, nil
}

if len(ps) == 0 {
ps = []specs.Platform{platforms.DefaultSpec()}
}

perfect := true
nodeIdxs := make([]int, 0)
for _, p := range ps {
Expand All @@ -178,13 +174,24 @@ func (r *nodeResolver) resolve(ctx context.Context, ps []specs.Platform, pw prog
}

var nodes []*resolvedNode
for i, idx := range nodeIdxs {
if len(nodeIdxs) == 0 {
nodes = append(nodes, &resolvedNode{
resolver: r,
driverIndex: idx,
platforms: []specs.Platform{ps[i]},
driverIndex: 0,
})
} else {
for i, idx := range nodeIdxs {
node := &resolvedNode{
resolver: r,
driverIndex: idx,
}
if len(ps) > 0 {
node.platforms = []specs.Platform{ps[i]}
}
nodes = append(nodes, node)
}
}

nodes = recombineNodes(nodes)
if _, err := r.boot(ctx, nodeIdxs, pw); err != nil {
return nil, false, err
Expand Down
6 changes: 4 additions & 2 deletions build/driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func TestFindDriverSanity(t *testing.T) {
require.Len(t, res, 1)
require.Equal(t, 0, res[0].driverIndex)
require.Equal(t, "aaa", res[0].Node().Builder)
require.Equal(t, []specs.Platform{platforms.DefaultSpec()}, res[0].platforms)
}

func TestFindDriverEmpty(t *testing.T) {
Expand Down Expand Up @@ -216,7 +217,7 @@ func TestSelectNodePreferExact(t *testing.T) {
require.Equal(t, "bbb", res[0].Node().Builder)
}

func TestSelectNodeCurrentPlatform(t *testing.T) {
func TestSelectNodeNoPlatform(t *testing.T) {
r := makeTestResolver(map[string][]specs.Platform{
"aaa": {platforms.MustParse("linux/foobar")},
"bbb": {platforms.DefaultSpec()},
Expand All @@ -226,7 +227,8 @@ func TestSelectNodeCurrentPlatform(t *testing.T) {
require.NoError(t, err)
require.True(t, perfect)
require.Len(t, res, 1)
require.Equal(t, "bbb", res[0].Node().Builder)
require.Equal(t, "aaa", res[0].Node().Builder)
Copy link
Member

Choose a reason for hiding this comment

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

The real issue is that the platform in opt gets reset when it should not. We need to figure out a test to make sure it doesn't happen. Looks like the old build code only expect "resolveNode" to have a platform if it is an override to the existing platform but maybe this should be made safer (more explicit) somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased on top of @jedevc changes in master...jedevc:buildx:fix-setting-wrong-platform-on-solve-opt to fix the solve opt case and remove the duplicated map.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced we have to change this, I think we could leave the default to be the current platform - this change isn't actually needed to fix the regression.

However, no real objections, my use case for this was to ease some UX issues with docker cloud build, which I don't really have any more.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess you talk about the TestSelectNodeNoPlatform and default spec case right? It seems related to #2119 and proposed new rules in #2119 (comment) should cover this in follow-up iiuc.

require.Empty(t, res[0].platforms)
}

func TestSelectNodeAdditionalPlatforms(t *testing.T) {
Expand Down