From 9752b77ba0308924b2a325961974fc32f684d9d9 Mon Sep 17 00:00:00 2001 From: Luis Sergio Carneiro Date: Fri, 9 Aug 2024 12:10:37 -0300 Subject: [PATCH 1/3] bugfix(#5755): avoiding deadlock between builds with dependencies build order strategy when the builds share enough dependencies --- pkg/apis/camel/v1/build_type_support_test.go | 301 +++++++++++++++++++ pkg/apis/camel/v1/build_types_support.go | 46 ++- 2 files changed, 341 insertions(+), 6 deletions(-) diff --git a/pkg/apis/camel/v1/build_type_support_test.go b/pkg/apis/camel/v1/build_type_support_test.go index 7fba1de191..01a635310e 100644 --- a/pkg/apis/camel/v1/build_type_support_test.go +++ b/pkg/apis/camel/v1/build_type_support_test.go @@ -19,6 +19,7 @@ package v1 import ( "testing" + "time" "github.com/stretchr/testify/assert" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -37,6 +38,9 @@ func TestMatchingBuildsPending(t *testing.T) { "camel:timer", "camel:log", }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, }, }, }, @@ -58,6 +62,9 @@ func TestMatchingBuildsPending(t *testing.T) { "camel:log", "camel:bean", }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, }, }, }, @@ -80,6 +87,9 @@ func TestMatchingBuildsPending(t *testing.T) { "camel:bean", "camel:zipfile", }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, }, }, }, @@ -101,6 +111,9 @@ func TestMatchingBuildsPending(t *testing.T) { "camel:component-a", "camel:component-b", }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, }, }, }, @@ -126,3 +139,291 @@ func TestMatchingBuildsPending(t *testing.T) { assert.False(t, matches) assert.Nil(t, buildMatch) } + +func TestMatchingBuildsSchedulingSharedDependencies(t *testing.T) { + timestamp, _ := time.Parse("2006-01-02T15:04:05-0700", "2024-08-09T10:00:00Z") + creationTimestamp := v1.Time{Time: timestamp} + buildA := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildA", + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:core", + "camel:rest", + "mvn:org.apache.camel.k:camel-k-runtime", + "mvn:org.apache.camel.quarkus:camel-quarkus-yaml-dsl", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + buildB := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildB", + CreationTimestamp: creationTimestamp, + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "mvn:org.apache.camel.k:camel-k-cron", + "mvn:org.apache.camel.k:camel-k-runtime", + "mvn:org.apache.camel.quarkus:camel-quarkus-yaml-dsl", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }}, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + + buildList := BuildList{ + Items: []Build{buildA, buildB}, + } + + // both builds share dependencies and have the same creationTimestamp + // buildA should be prioritized so there should be not matching build for it + + matches, buildMatch := buildList.HasMatchingBuild(&buildA) + assert.False(t, matches) + assert.Nil(t, buildMatch) + matches, buildMatch = buildList.HasMatchingBuild(&buildB) + assert.True(t, matches) + assert.True(t, buildMatch.Name == buildA.Name) +} + +func TestMatchingBuildsSchedulingSameDependenciesDIfferentRuntimes(t *testing.T) { + timestamp, _ := time.Parse("2006-01-02T15:04:05-0700", "2024-08-09T10:00:00Z") + creationTimestamp := v1.Time{Time: timestamp} + buildA := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildA", + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "mvn:org.apache.camel.k:camel-k-cron", + "mvn:org.apache.camel.k:camel-k-runtime", + "mvn:org.apache.camel.quarkus:camel-quarkus-yaml-dsl", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + buildB := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildB", + CreationTimestamp: creationTimestamp, + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "mvn:org.apache.camel.k:camel-k-cron", + "mvn:org.apache.camel.k:camel-k-runtime", + "mvn:org.apache.camel.quarkus:camel-quarkus-yaml-dsl", + }, + Runtime: RuntimeSpec{ + Version: "3.2.3", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + + buildList := BuildList{ + Items: []Build{buildA, buildB}, + } + + // each build uses a different runtime, so they should not match + + matches, buildMatch := buildList.HasMatchingBuild(&buildA) + assert.False(t, matches) + assert.Nil(t, buildMatch) + matches, buildMatch = buildList.HasMatchingBuild(&buildB) + assert.False(t, matches) + assert.Nil(t, buildMatch) +} + +func TestMatchingBuildsSchedulingSameDependenciesSameRuntime(t *testing.T) { + timestamp, _ := time.Parse("2006-01-02T15:04:05-0700", "2024-08-09T10:00:00Z") + creationTimestamp := v1.Time{Time: timestamp} + buildA := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildA", + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "mvn:org.apache.camel.k:camel-k-cron", + "mvn:org.apache.camel.k:camel-k-runtime", + "mvn:org.apache.camel.quarkus:camel-quarkus-yaml-dsl", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + buildB := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildB", + CreationTimestamp: creationTimestamp, + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "mvn:org.apache.camel.k:camel-k-cron", + "mvn:org.apache.camel.k:camel-k-runtime", + "mvn:org.apache.camel.quarkus:camel-quarkus-yaml-dsl", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + + buildList := BuildList{ + Items: []Build{buildA, buildB}, + } + + // ebuilds have the same dependencies, runtime and creation timestamp + + matches, buildMatch := buildList.HasMatchingBuild(&buildA) + assert.False(t, matches) + assert.Nil(t, buildMatch) + matches, buildMatch = buildList.HasMatchingBuild(&buildB) + assert.True(t, matches) + assert.True(t, buildMatch.Name == buildA.Name) +} + +func TestMatchingBuildsSchedulingFewCommonDependencies(t *testing.T) { + timestamp, _ := time.Parse("2006-01-02T15:04:05-0700", "2024-08-09T10:00:00Z") + creationTimestamp := v1.Time{Time: timestamp} + buildA := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildA", + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "camel:componenta1", + "camel:componentb1", + "camel:componentc1", + "camel:componentd1", + "camel:componente1", + "camel:componentf1", + "camel:componentg1", + "camel:componenth1", + "camel:componenti1", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + buildB := Build{ + ObjectMeta: v1.ObjectMeta{ + Name: "buildB", + CreationTimestamp: creationTimestamp, + }, + Spec: BuildSpec{ + Tasks: []Task{ + { + Builder: &BuilderTask{ + Dependencies: []string{ + "camel:quartz", + "camel:componenta2", + "camel:componentb2", + "camel:componentc2", + "camel:componentd2", + "camel:componente2", + "camel:componentf2", + "camel:componentg2", + "camel:componenth2", + "camel:componenti2", + }, + Runtime: RuntimeSpec{ + Version: "3.8.1", + }, + }, + }, + }, + }, + Status: BuildStatus{ + Phase: BuildPhaseScheduling, + }, + } + + buildList := BuildList{ + Items: []Build{buildA, buildB}, + } + + // builds have only 1 out of 10 shared dependencies. they should not match + + matches, buildMatch := buildList.HasMatchingBuild(&buildA) + assert.False(t, matches) + assert.Nil(t, buildMatch) + matches, buildMatch = buildList.HasMatchingBuild(&buildB) + assert.False(t, matches) + assert.Nil(t, buildMatch) +} diff --git a/pkg/apis/camel/v1/build_types_support.go b/pkg/apis/camel/v1/build_types_support.go index a264e5168c..978c941f32 100644 --- a/pkg/apis/camel/v1/build_types_support.go +++ b/pkg/apis/camel/v1/build_types_support.go @@ -18,6 +18,8 @@ limitations under the License. package v1 import ( + "strings" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -73,6 +75,14 @@ func (build *Build) BuilderDependencies() []string { return []string{} } +func (build *Build) RuntimeVersion() *string { + if builder, ok := FindBuilderTask(build.Spec.Tasks); ok { + return &builder.Runtime.Version + } + + return nil +} + // FindBuilderTask returns the 1st builder task from the task list. func FindBuilderTask(tasks []Task) (*BuilderTask, bool) { for _, t := range tasks { @@ -272,11 +282,17 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { if len(required) == 0 { return false, nil } + runtimeVersion := build.RuntimeVersion() for _, b := range bl.Items { if b.Name == build.Name || b.Status.IsFinished() { continue } + bRuntimeVersion := b.RuntimeVersion() + + if *runtimeVersion != *bRuntimeVersion { + continue + } dependencies := b.BuilderDependencies() dependencyMap := make(map[string]int, len(dependencies)) @@ -286,16 +302,17 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { allMatching := true missing := 0 + commonDependencies := 0 for _, item := range required { if _, ok := dependencyMap[item]; !ok { allMatching = false missing++ + } else { + commonDependencies++ } } - // Heuristic approach: if there are too many unrelated libraries then this image is - // not suitable to be used as base image - if !allMatching && missing > len(required)/2 { + if commonDependencies < len(required)/2 { continue } @@ -311,10 +328,27 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { // additionally check for the creation timestamp if b.CreationTimestamp.Before(&build.CreationTimestamp) { return true, &b + } else if b.CreationTimestamp.Equal(&build.CreationTimestamp) { + if strings.Compare(b.Name, build.Name) < 0 { + return true, &b + } + } + + } else if !allMatching && commonDependencies > 0 { + // there are common dependencies. let's compare the total number of dependencies + // in each build. whichever build has less dependencies should run first + if len(dependencies) < len(required) { + return true, &b + } else if len(dependencies) == len(required) { + // same number of total dependencies. + if b.CreationTimestamp.Before(&build.CreationTimestamp) { + return true, &b + } else if b.CreationTimestamp.Equal(&build.CreationTimestamp) && + strings.Compare(b.Name, build.Name) < 0 { + return true, &b + } } - } else if missing > 0 { - // found another suitable scheduled build with fewer dependencies that should build first in order to reuse the produced image - return true, &b + continue } } } From 35b89dc306e7d6d172ad4ce6d874af099b22b6de Mon Sep 17 00:00:00 2001 From: Luis Sergio Carneiro Date: Mon, 12 Aug 2024 08:52:24 -0300 Subject: [PATCH 2/3] Creating compareBuilds function --- pkg/apis/camel/v1/build_types_support.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/apis/camel/v1/build_types_support.go b/pkg/apis/camel/v1/build_types_support.go index 978c941f32..da08022074 100644 --- a/pkg/apis/camel/v1/build_types_support.go +++ b/pkg/apis/camel/v1/build_types_support.go @@ -326,14 +326,9 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { if allMatching && len(required) == len(dependencies) { // seems like both builds require exactly the same list of dependencies // additionally check for the creation timestamp - if b.CreationTimestamp.Before(&build.CreationTimestamp) { + if compareBuilds(&b, build) < 0 { return true, &b - } else if b.CreationTimestamp.Equal(&build.CreationTimestamp) { - if strings.Compare(b.Name, build.Name) < 0 { - return true, &b - } } - } else if !allMatching && commonDependencies > 0 { // there are common dependencies. let's compare the total number of dependencies // in each build. whichever build has less dependencies should run first @@ -341,10 +336,7 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { return true, &b } else if len(dependencies) == len(required) { // same number of total dependencies. - if b.CreationTimestamp.Before(&build.CreationTimestamp) { - return true, &b - } else if b.CreationTimestamp.Equal(&build.CreationTimestamp) && - strings.Compare(b.Name, build.Name) < 0 { + if compareBuilds(&b, build) < 0 { return true, &b } } @@ -355,3 +347,13 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { return false, nil } + +func compareBuilds(b1 *Build, b2 *Build) int { + if b1.CreationTimestamp.Before(&b2.CreationTimestamp) { + return -1 + } else if b2.CreationTimestamp.Before(&b1.CreationTimestamp) { + return 1 + } else { + return strings.Compare(b1.Name, b2.Name) + } +} From d5a9b80c315b6e849aa07ab1aab964f2d3658ab5 Mon Sep 17 00:00:00 2001 From: Luis Sergio Carneiro Date: Tue, 13 Aug 2024 17:30:20 -0300 Subject: [PATCH 3/3] lint: reducing code complexity --- pkg/apis/camel/v1/build_types_support.go | 25 ++++++++++-------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/pkg/apis/camel/v1/build_types_support.go b/pkg/apis/camel/v1/build_types_support.go index da08022074..c66930f72b 100644 --- a/pkg/apis/camel/v1/build_types_support.go +++ b/pkg/apis/camel/v1/build_types_support.go @@ -316,13 +316,12 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { continue } - // handle suitable build that has started already - if b.Status.Phase == BuildPhasePending || b.Status.Phase == BuildPhaseRunning { + switch b.Status.Phase { + case BuildPhasePending, BuildPhaseRunning: + // handle suitable build that has started already return true, &b - } - - // handle suitable scheduled build - if b.Status.Phase == BuildPhaseInitialization || b.Status.Phase == BuildPhaseScheduling { + case BuildPhaseInitialization, BuildPhaseScheduling: + // handle suitable scheduled build if allMatching && len(required) == len(dependencies) { // seems like both builds require exactly the same list of dependencies // additionally check for the creation timestamp @@ -332,13 +331,9 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { } else if !allMatching && commonDependencies > 0 { // there are common dependencies. let's compare the total number of dependencies // in each build. whichever build has less dependencies should run first - if len(dependencies) < len(required) { + if len(dependencies) < len(required) || + len(dependencies) == len(required) && compareBuilds(&b, build) < 0 { return true, &b - } else if len(dependencies) == len(required) { - // same number of total dependencies. - if compareBuilds(&b, build) < 0 { - return true, &b - } } continue } @@ -351,9 +346,9 @@ func (bl BuildList) HasMatchingBuild(build *Build) (bool, *Build) { func compareBuilds(b1 *Build, b2 *Build) int { if b1.CreationTimestamp.Before(&b2.CreationTimestamp) { return -1 - } else if b2.CreationTimestamp.Before(&b1.CreationTimestamp) { + } + if b2.CreationTimestamp.Before(&b1.CreationTimestamp) { return 1 - } else { - return strings.Compare(b1.Name, b2.Name) } + return strings.Compare(b1.Name, b2.Name) }