From aa0aeac297aa19c7ab2aefdde48430c172721bc1 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Tue, 28 Nov 2023 10:10:50 +0000 Subject: [PATCH 1/4] build: move solve opt out of duplicate map This was more error prone, as opposed to the approach used prior to 616fb3e55cbc85647026f6e409af17e1011a85c4. Signed-off-by: Justin Chadwell --- build/build.go | 22 +++++++++------------- build/driver.go | 2 ++ 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/build/build.go b/build/build.go index 194b1897..74fe0571 100644 --- a/build/build.go +++ b/build/build.go @@ -509,10 +509,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() { @@ -532,7 +528,7 @@ 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] { + for _, np := range drivers[k] { if np.Node().Driver.IsMobyDriver() { hasMobyDriver = true } @@ -552,7 +548,7 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s so.FrontendAttrs[k] = v } defers = append(defers, release) - driversSolveOpts[k][i] = so + np.SolveOpt = so } for _, at := range opt.Session { if s, ok := at.(interface { @@ -566,8 +562,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 drivers[k] { + for _, e := range np.SolveOpt.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") @@ -582,7 +578,7 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s for name := range opt { dps := drivers[name] for i, dp := range dps { - so := driversSolveOpts[name][i] + so := drivers[name][i].SolveOpt for k, v := range so.FrontendAttrs { if strings.HasPrefix(k, "context:") && strings.HasPrefix(v, "target:") { k2 := strings.TrimPrefix(v, "target:") @@ -610,7 +606,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(drivers, opt) for k, opt := range opt { err := func(k string) error { @@ -634,7 +630,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 := drivers[k][i].SolveOpt if multiDriver { for i, e := range so.Exports { switch e.Type { @@ -1303,12 +1299,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(drivers map[string][]*resolvedNode, opt map[string]Options) map[string][]string { out := make(map[string][]string) for name := range opt { dps := drivers[name] for i, dp := range dps { - so := driversSolveOpts[name][i] + so := drivers[name][i].SolveOpt for k, v := range so.FrontendAttrs { if strings.HasPrefix(k, "context:") && strings.HasPrefix(v, "target:") { target := resultKey(dp.driverIndex, strings.TrimPrefix(v, "target:")) diff --git a/build/driver.go b/build/driver.go index 9e89b440..c2e01cbb 100644 --- a/build/driver.go +++ b/build/driver.go @@ -19,6 +19,8 @@ import ( ) type resolvedNode struct { + SolveOpt *client.SolveOpt + resolver *nodeResolver driverIndex int platforms []specs.Platform From aac7a47469b32680472168669e046af34287e462 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Tue, 28 Nov 2023 10:11:51 +0000 Subject: [PATCH 2/4] build: fix incorrect solve opt platform from being set This regression was introduced in 616fb3e55cbc85647026f6e409af17e1011a85c4, with the node resolution refactor. The core issue here is just that we would unconditionally set the solve opt's platform to the default current platform, which was incorrect. We can prevent this easily by having a special case for the default case, like we had before, and then not setting the platforms field on this (which keeping the resolution behavior which was introduced). Signed-off-by: Justin Chadwell --- build/driver.go | 26 +++++++++++++++++--------- build/driver_test.go | 2 ++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/build/driver.go b/build/driver.go index c2e01cbb..0096459e 100644 --- a/build/driver.go +++ b/build/driver.go @@ -164,28 +164,36 @@ 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 { - idx := r.get(p, matcher, additional) + if len(ps) == 0 { + idx := r.get(platforms.DefaultSpec(), matcher, additional) if idx == -1 { idx = 0 perfect = false } nodeIdxs = append(nodeIdxs, idx) + } else { + for _, p := range ps { + idx := r.get(p, matcher, additional) + if idx == -1 { + idx = 0 + perfect = false + } + nodeIdxs = append(nodeIdxs, idx) + } } var nodes []*resolvedNode for i, idx := range nodeIdxs { - nodes = append(nodes, &resolvedNode{ + node := &resolvedNode{ resolver: r, driverIndex: idx, - platforms: []specs.Platform{ps[i]}, - }) + } + 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 { diff --git a/build/driver_test.go b/build/driver_test.go index 068ec7ee..63c79728 100644 --- a/build/driver_test.go +++ b/build/driver_test.go @@ -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) { @@ -227,6 +228,7 @@ func TestSelectNodeCurrentPlatform(t *testing.T) { require.True(t, perfect) require.Len(t, res, 1) require.Equal(t, "bbb", res[0].Node().Builder) + require.Empty(t, res[0].platforms) } func TestSelectNodeAdditionalPlatforms(t *testing.T) { From 54032316f90672da1e86b1df878d59c865a8f756 Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Tue, 28 Nov 2023 11:55:47 +0100 Subject: [PATCH 3/4] build: infer platform from first node if none set Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> --- build/driver.go | 35 +++++++++++++++++------------------ build/driver_test.go | 4 ++-- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/build/driver.go b/build/driver.go index 0096459e..ef0a4e1e 100644 --- a/build/driver.go +++ b/build/driver.go @@ -166,35 +166,34 @@ func (r *nodeResolver) resolve(ctx context.Context, ps []specs.Platform, pw prog perfect := true nodeIdxs := make([]int, 0) - if len(ps) == 0 { - idx := r.get(platforms.DefaultSpec(), matcher, additional) + for _, p := range ps { + idx := r.get(p, matcher, additional) if idx == -1 { idx = 0 perfect = false } nodeIdxs = append(nodeIdxs, idx) - } else { - for _, p := range ps { - idx := r.get(p, matcher, additional) - if idx == -1 { - idx = 0 - perfect = false - } - nodeIdxs = append(nodeIdxs, idx) - } } var nodes []*resolvedNode - for i, idx := range nodeIdxs { - node := &resolvedNode{ + if len(nodeIdxs) == 0 { + nodes = append(nodes, &resolvedNode{ resolver: r, - driverIndex: idx, + 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) } - 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 diff --git a/build/driver_test.go b/build/driver_test.go index 63c79728..24f43411 100644 --- a/build/driver_test.go +++ b/build/driver_test.go @@ -217,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()}, @@ -227,7 +227,7 @@ 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) require.Empty(t, res[0].platforms) } From 9d8ac1ce2d2b2258d0c494b2c818f745902d462b Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Mon, 4 Dec 2023 11:27:59 +0100 Subject: [PATCH 4/4] build: move solveOpt to local struct type *client.SolveOpt in driver code is only used by build code. For a clear separation of concerns, move it to an internal struct type only accessible by BuildWithResultHandler func. Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> --- build/build.go | 31 +++++++++++++++++++++---------- build/driver.go | 2 -- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/build/build.go b/build/build.go index 74fe0571..624ceb84 100644 --- a/build/build.go +++ b/build/build.go @@ -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") @@ -519,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 { @@ -528,6 +534,7 @@ 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") } + var reqn []*reqForNode for _, np := range drivers[k] { if np.Node().Driver.IsMobyDriver() { hasMobyDriver = true @@ -548,8 +555,12 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s so.FrontendAttrs[k] = v } defers = append(defers, release) - np.SolveOpt = 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) @@ -562,8 +573,8 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s // validate for multi-node push if hasMobyDriver && multiDriver { - for _, np := range drivers[k] { - for _, e := range np.SolveOpt.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") @@ -576,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 := drivers[name][i].SolveOpt + 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:") @@ -606,7 +617,7 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s results := waitmap.New() multiTarget := len(opt) > 1 - childTargets := calculateChildTargets(drivers, opt) + childTargets := calculateChildTargets(reqForNodes, opt) for k, opt := range opt { err := func(k string) error { @@ -630,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 := drivers[k][i].SolveOpt + so := reqForNodes[k][i].so if multiDriver { for i, e := range so.Exports { switch e.Type { @@ -1299,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, 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 := drivers[name][i].SolveOpt + 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:")) diff --git a/build/driver.go b/build/driver.go index ef0a4e1e..07f4560b 100644 --- a/build/driver.go +++ b/build/driver.go @@ -19,8 +19,6 @@ import ( ) type resolvedNode struct { - SolveOpt *client.SolveOpt - resolver *nodeResolver driverIndex int platforms []specs.Platform