From 8a1b58d6d5688e9e33d6458864df49feb27b0839 Mon Sep 17 00:00:00 2001 From: Adam Kaplan Date: Tue, 1 Jul 2025 14:05:12 -0400 Subject: [PATCH] feat: Set Field Owners for Build/BuildRun Controllers Set field ownership for the Build and BuildRun controllers so that we avoid collisions when making API create/update calls (and future server-side apply calls). Assisted-by: Cursor Signed-off-by: Adam Kaplan --- pkg/reconciler/build/build.go | 2 +- pkg/reconciler/buildrun/buildrun.go | 2 +- .../buildrun_ttl_cleanup.go | 2 +- test/integration/build_to_taskruns_test.go | 14 +++++ .../integration/buildruns_to_taskruns_test.go | 53 ++++++++++++++++++- 5 files changed, 69 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/build/build.go b/pkg/reconciler/build/build.go index 0a56bad397..b0824ca015 100644 --- a/pkg/reconciler/build/build.go +++ b/pkg/reconciler/build/build.go @@ -52,7 +52,7 @@ type ReconcileBuild struct { func NewReconciler(c *config.Config, mgr manager.Manager, ownerRef setOwnerReferenceFunc) reconcile.Reconciler { return &ReconcileBuild{ config: c, - client: mgr.GetClient(), + client: client.WithFieldOwner(mgr.GetClient(), "shipwright-build-controller"), scheme: mgr.GetScheme(), setOwnerReferenceFunc: ownerRef, } diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go index 5c6609d147..410a9ccb4e 100644 --- a/pkg/reconciler/buildrun/buildrun.go +++ b/pkg/reconciler/buildrun/buildrun.go @@ -57,7 +57,7 @@ type ReconcileBuildRun struct { func NewReconciler(c *config.Config, mgr manager.Manager, ownerRef setOwnerReferenceFunc) reconcile.Reconciler { return &ReconcileBuildRun{ config: c, - client: mgr.GetClient(), + client: client.WithFieldOwner(mgr.GetClient(), "shipwright-buildrun-controller"), scheme: mgr.GetScheme(), setOwnerReferenceFunc: ownerRef, } diff --git a/pkg/reconciler/buildrunttlcleanup/buildrun_ttl_cleanup.go b/pkg/reconciler/buildrunttlcleanup/buildrun_ttl_cleanup.go index abe645b8a6..f03b5602fb 100644 --- a/pkg/reconciler/buildrunttlcleanup/buildrun_ttl_cleanup.go +++ b/pkg/reconciler/buildrunttlcleanup/buildrun_ttl_cleanup.go @@ -31,7 +31,7 @@ type ReconcileBuildRun struct { func NewReconciler(c *config.Config, mgr manager.Manager) reconcile.Reconciler { return &ReconcileBuildRun{ config: c, - client: mgr.GetClient(), + client: client.WithFieldOwner(mgr.GetClient(), "shipwright-buildrun-ttl-cleanup-controller"), } } diff --git a/test/integration/build_to_taskruns_test.go b/test/integration/build_to_taskruns_test.go index df3349fb04..796b388046 100644 --- a/test/integration/build_to_taskruns_test.go +++ b/test/integration/build_to_taskruns_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" "github.com/shipwright-io/build/pkg/apis/build/v1beta1" @@ -146,6 +147,19 @@ var _ = Describe("Integration tests Build and TaskRun", func() { Expect(*buildObject.Status.Reason).To(Equal(v1beta1.SucceedStatus)) Expect(*buildObject.Status.Message).To(Equal(v1beta1.AllValidationsSucceeded)) }) + + It("should set the buildrun controller as a field manager", func() { + Expect(tb.CreateBuild(buildObject)).To(Succeed()) + + buildObject, err = tb.GetBuildTillValidation(buildObject.Name) + Expect(err).NotTo(HaveOccurred()) + + fieldManagers := sets.NewString() + for _, field := range buildObject.GetObjectMeta().GetManagedFields() { + fieldManagers.Insert(field.Manager) + } + Expect(fieldManagers).To(HaveKey("shipwright-build-controller"), "build controller should be registered as a field manager") + }) }) Context("when creating the taskrun", func() { diff --git a/test/integration/buildruns_to_taskruns_test.go b/test/integration/buildruns_to_taskruns_test.go index 9782b6c1ca..3c36b4a8d1 100644 --- a/test/integration/buildruns_to_taskruns_test.go +++ b/test/integration/buildruns_to_taskruns_test.go @@ -6,6 +6,7 @@ package integration_test import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -21,6 +22,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" @@ -212,10 +214,32 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() { }) Context("when a buildrun is created", func() { + + var ( + buildRunObject *v1beta1.BuildRun + taskRunObject *pipelineapi.TaskRun + ) + + AfterEach(func() { + if !CurrentSpecReport().Failed() { + return + } + if buildRunObject != nil { + jsonData, err := json.MarshalIndent(buildRunObject, "", " ") + Expect(err).NotTo(HaveOccurred(), "failed to marshal buildRunObject to JSON") + GinkgoWriter.Printf("buildRunObject JSON:\n%s\n", string(jsonData)) + } + if taskRunObject != nil { + jsonData, err := json.MarshalIndent(taskRunObject, "", " ") + Expect(err).NotTo(HaveOccurred(), "failed to marshal taskRunObject to JSON") + GinkgoWriter.Printf("taskRunObject JSON:\n%s\n", string(jsonData)) + } + }) + It("should reflect a Pending and Running reason", func() { // use a custom strategy here that just sleeps 30 seconds to prevent it from completing too fast so that we do not get the Running state WithCustomClusterBuildStrategy([]byte(test.ClusterBuildStrategySleep30s), func() { - _, _, buildRunObject := setupBuildAndBuildRun([]byte(test.BuildCBSMinimal), []byte(test.MinimalBuildRun), STRATEGY+tb.Namespace+"custom") + _, _, buildRunObject = setupBuildAndBuildRun([]byte(test.BuildCBSMinimal), []byte(test.MinimalBuildRun), STRATEGY+tb.Namespace+"custom") _, err = tb.GetBRTillStartTime(buildRunObject.Name) Expect(err).To(BeNil()) @@ -242,6 +266,33 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() { Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason)) }) }) + + It("should create a taskrun that is owned by the buildrun", func() { + WithCustomClusterBuildStrategy([]byte(test.ClusterBuildStrategyNoOp), func() { + _, _, buildRunObject = setupBuildAndBuildRun([]byte(test.BuildCBSMinimal), []byte(test.MinimalBuildRun), STRATEGY+tb.Namespace+"custom") + + _, err = tb.GetBRTillStartTime(buildRunObject.Name) + Expect(err).To(BeNil()) + + taskRunObject, err = tb.GetTaskRunFromBuildRun(buildRunObject.Name) + Expect(err).To(BeNil()) + + // Check that the taskrun is owned by the buildrun + Expect(taskRunObject.OwnerReferences).To(HaveLen(1), "taskrun should have exactly one owner reference") + ownerRef := taskRunObject.OwnerReferences[0] + Expect(ownerRef.Kind).To(Equal("BuildRun"), "taskrun should have a buildrun owner reference") + Expect(ownerRef.Name).To(Equal(buildRunObject.Name), "taskrun should have a buildrun owner reference") + + // Check that the buildrun controller is registered as a field manager + fieldManagers := sets.NewString() + for _, field := range taskRunObject.GetObjectMeta().GetManagedFields() { + fieldManagers.Insert(field.Manager) + } + // Sets uses a map[string]struct{} internally, so we need to check for the key. + // We can't use ContainElement because it would check for the value, not the key. + Expect(fieldManagers).To(HaveKey("shipwright-buildrun-controller"), "buildrun controller should be registered as a field manager") + }) + }) }) Context("when a buildrun is created but fails because of a timeout", func() {