From f9453c88273b33bd35f34c62dde9207fe667d75a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Joulaud?= Date: Thu, 9 Jan 2025 13:02:03 +0100 Subject: [PATCH 1/2] fix: child image should be rebuildable The Filter function constructs a completely new graph were unmodified images are discarded. I suppose it was done to have a (very) smaller graph to improve (a little) performance. Unfortunately, we need info on all direct parents images when rebuilding, at least to properly amend the FROM line to be able to pull the right parent image. Thus this refacto broke completely rebuild of every image with at least one not-to-rebuild parent. I fix this quickly by avoiding the Filter function altogether which is too complicated for our sake and does not really improves the performances. Fixes be5509e34ab6ce172a61943ed2152ee7cd734b48 --- pkg/dib/build.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/dib/build.go b/pkg/dib/build.go index 3c197ac1..fc5d9c09 100644 --- a/pkg/dib/build.go +++ b/pkg/dib/build.go @@ -85,13 +85,13 @@ func (p *Builder) rebuildGraph( buildArgs map[string]string, ) { p.Graph. - Filter( - func(node *dag.Node) bool { - return node.Image.NeedsRebuild || node.Image.NeedsTests - }). WalkParallel( func(node *dag.Node) { img := node.Image + if !(img.NeedsRebuild || img.NeedsTests) { + img.RebuildFailed = false + return + } buildReport := report.BuildReport{Image: *img} // Return if any parent build failed From 3e56470f53715daf1e3d76b075ba1d682b0d7305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Joulaud?= Date: Fri, 10 Jan 2025 16:18:39 +0100 Subject: [PATCH 2/2] clean(dag): cleanup unused Filter function Following 7ef5705 the "Filter" function is not used anymore. After thinking I think it will not ever be used in this form as it is not generic and thus it is better to completely delete it. Indeed the function is buggy, difficult to understand (and to fix). E.g. I still have not grasped what it try to do with the orphans and their childs. I think (without being sure) the idea behind this was to be able to work on a smaller graph for performance but as the small graph is used just once, the cost of constructing it in the first place completely offsets the potential gains. --- pkg/dag/dag.go | 52 --------------------------------------------- pkg/dag/dag_test.go | 26 ----------------------- 2 files changed, 78 deletions(-) diff --git a/pkg/dag/dag.go b/pkg/dag/dag.go index c96ff31c..8cb3dae1 100644 --- a/pkg/dag/dag.go +++ b/pkg/dag/dag.go @@ -107,58 +107,6 @@ func (d *DAG) WalkParallel(visitor NodeVisitorFunc) { waitGroup.Wait() } -// Filter creates a new DAG only populated by nodes that fulfil the condition checked by filterFunc. -// All children nodes that no longer have any parents in the resulting graph will become a root node of the DAG. -func (d *DAG) Filter(filterFunc func(*Node) bool) *DAG { - filteredGraph := DAG{} - replacements := make(map[*Node]*Node) - var orphans []*Node - - d.Walk(func(node *Node) { - if !filterFunc(node) { - // The node does not fulfil the condition, skip it. - return - } - - // Create a replacement node containing the same Image. - newNode := NewNode(node.Image) - replacements[node] = newNode - // Check if the node is orphan - for _, parent := range node.Parents() { - if _, ok := replacements[parent]; ok { - // The node has at least one parent, nothing to do. - return - } - } - // The node no longer has parents, we add it to the list of orphans. - orphans = append(orphans, node) - }) - - for _, orphan := range orphans { - orphan.walkInDepth(createUniqueVisitor(func(node *Node) { - replacement, ok := replacements[node] - if !ok { - // This node on the source graph should not be in the filtered graph. - return - } - - // Add new child nodes - for _, child := range node.Children() { - childReplacement, ok := replacements[child] - if !ok { - // This child node on the source graph should not be in the filtered graph. - continue - } - replacement.AddChild(childReplacement) - } - })) - // Add the orphan node to the root nodes of the graph. - filteredGraph.AddNode(replacements[orphan]) - } - - return &filteredGraph -} - //nolint:musttag func (d *DAG) ListImage() string { imagesList := make(map[string]Image) diff --git a/pkg/dag/dag_test.go b/pkg/dag/dag_test.go index 15f93c68..2baf02b0 100644 --- a/pkg/dag/dag_test.go +++ b/pkg/dag/dag_test.go @@ -214,32 +214,6 @@ func Test_WalkParallel_RunsAllNodes(t *testing.T) { assert.Equal(t, 6, length) // Total number of nodes is 6 } -func Test_Filter(t *testing.T) { - t.Parallel() - - DAG := createDAG() - - withoutParents := DAG.Filter(func(node *dag.Node) bool { - return len(node.Parents()) > 0 // Filter out nodes having parents. - }) - assert.Len(t, withoutParents.Nodes(), 3) - - withParents := DAG.Filter(func(node *dag.Node) bool { - return len(node.Parents()) == 0 // Filter out nodes having no parent. - }) - assert.Len(t, withParents.Nodes(), 2) - - withoutChildren := DAG.Filter(func(node *dag.Node) bool { - return len(node.Children()) > 0 // Filter out nodes having children. - }) - assert.Len(t, withoutChildren.Nodes(), 2) - - withChildren := DAG.Filter(func(node *dag.Node) bool { - return len(node.Children()) == 0 // Filter out nodes having no children. - }) - assert.Len(t, withChildren.Nodes(), 3) -} - func Test_ListImage(t *testing.T) { t.Parallel()