From 3a4338e4cee66c2787544793a1257dc7ea95549b Mon Sep 17 00:00:00 2001 From: Luis Sergio Carneiro Date: Fri, 9 Aug 2024 12:10:37 -0300 Subject: [PATCH] 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 } } }