Skip to content

Commit d64c427

Browse files
elenagermanelenagerman
authored
Merge pull request konflux-ci#876 from elenagerman/release-1370-race-condition
fix(RELEASE-1370): limit the taskRuns race condition with the update
2 parents d183c5f + 4560b67 commit d64c427

5 files changed

Lines changed: 230 additions & 13 deletions

File tree

‎controllers/release/controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func (c *Controller) Register(mgr ctrl.Manager, log *logr.Logger, _ cluster.Clus
114114
Kind: "Release",
115115
Group: "appstudio.redhat.com",
116116
},
117-
}, builder.WithPredicates(tekton.ReleasePipelineRunSucceededPredicate())).
117+
}, builder.WithPredicates(tekton.ReleasePipelineRunLifecyclePredicate())).
118118
Complete(c)
119119
}
120120

‎tekton/predicates.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,29 @@ limitations under the License.
1717
package tekton
1818

1919
import (
20+
"github.com/konflux-ci/release-service/metadata"
21+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2022
"sigs.k8s.io/controller-runtime/pkg/event"
2123
"sigs.k8s.io/controller-runtime/pkg/predicate"
2224
)
2325

24-
// ReleasePipelineRunSucceededPredicate returns a predicate which filters out all objects except
25-
// Release PipelineRuns which have just succeeded.
26-
func ReleasePipelineRunSucceededPredicate() predicate.Predicate {
26+
// ReleasePipelineRunLifecyclePredicate is a predicate to select Release PipelineRuns during
27+
// key lifecycle events: successful completion, deletion with finalizers, or finalizer changes.
28+
func ReleasePipelineRunLifecyclePredicate() predicate.Predicate {
2729
return predicate.Funcs{
2830
CreateFunc: func(createEvent event.CreateEvent) bool {
2931
return false
3032
},
3133
DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
32-
return false
34+
return isReleasePipelineRun(deleteEvent.Object) &&
35+
controllerutil.ContainsFinalizer(deleteEvent.Object, metadata.ReleaseFinalizer)
3336
},
3437
GenericFunc: func(genericEvent event.GenericEvent) bool {
3538
return false
3639
},
3740
UpdateFunc: func(e event.UpdateEvent) bool {
38-
return isReleasePipelineRun(e.ObjectNew) && hasPipelineSucceeded(e.ObjectNew)
41+
return isReleasePipelineRun(e.ObjectNew) &&
42+
(hasPipelineSucceeded(e.ObjectNew) || hasFinalizersChanged(e.ObjectOld, e.ObjectNew))
3943
},
4044
}
4145
}

‎tekton/predicates_test.go‎

Lines changed: 115 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import (
2727
)
2828

2929
var _ = Describe("Predicates", Ordered, func() {
30-
When("testing ReleasePipelineRunSucceededPredicate predicate", func() {
30+
When("testing ReleasePipelineRunLifecyclePredicate predicate", func() {
3131
var err error
3232
var pipelineRun *v1.PipelineRun
3333

@@ -40,21 +40,46 @@ var _ = Describe("Predicates", Ordered, func() {
4040
contextEvent := event.CreateEvent{
4141
Object: pipelineRun,
4242
}
43-
Expect(ReleasePipelineRunSucceededPredicate().Create(contextEvent)).To(BeFalse())
43+
Expect(ReleasePipelineRunLifecyclePredicate().Create(contextEvent)).To(BeFalse())
4444
})
4545

46-
It("should ignore deleting events", func() {
46+
It("should ignore deleting events for non-release PipelineRuns", func() {
4747
contextEvent := event.DeleteEvent{
4848
Object: pipelineRun,
4949
}
50-
Expect(ReleasePipelineRunSucceededPredicate().Delete(contextEvent)).To(BeFalse())
50+
Expect(ReleasePipelineRunLifecyclePredicate().Delete(contextEvent)).To(BeFalse())
51+
})
52+
53+
It("should ignore deleting events for release PipelineRuns without our finalizer", func() {
54+
releasePipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
55+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
56+
Build()
57+
Expect(err).NotTo(HaveOccurred())
58+
59+
contextEvent := event.DeleteEvent{
60+
Object: releasePipelineRun,
61+
}
62+
Expect(ReleasePipelineRunLifecyclePredicate().Delete(contextEvent)).To(BeFalse())
63+
})
64+
65+
It("should reconcile deleting events for release PipelineRuns with our finalizer", func() {
66+
releasePipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
67+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
68+
WithFinalizer(metadata.ReleaseFinalizer).
69+
Build()
70+
Expect(err).NotTo(HaveOccurred())
71+
72+
contextEvent := event.DeleteEvent{
73+
Object: releasePipelineRun,
74+
}
75+
Expect(ReleasePipelineRunLifecyclePredicate().Delete(contextEvent)).To(BeTrue())
5176
})
5277

5378
It("should ignore generic events", func() {
5479
contextEvent := event.GenericEvent{
5580
Object: pipelineRun,
5681
}
57-
Expect(ReleasePipelineRunSucceededPredicate().Generic(contextEvent)).To(BeFalse())
82+
Expect(ReleasePipelineRunLifecyclePredicate().Generic(contextEvent)).To(BeFalse())
5883
})
5984

6085
It("should return true when an updated event is received for a succeeded managed PipelineRun", func() {
@@ -68,10 +93,93 @@ var _ = Describe("Predicates", Ordered, func() {
6893
ObjectNew: releasePipelineRun,
6994
}
7095
releasePipelineRun.Status.MarkRunning("Predicate function tests", "Set it to Unknown")
71-
Expect(ReleasePipelineRunSucceededPredicate().Update(contextEvent)).To(BeFalse())
96+
Expect(ReleasePipelineRunLifecyclePredicate().Update(contextEvent)).To(BeFalse())
7297

7398
releasePipelineRun.Status.MarkSucceeded("Predicate function tests", "Set it to Succeeded")
74-
Expect(ReleasePipelineRunSucceededPredicate().Update(contextEvent)).To(BeTrue())
99+
Expect(ReleasePipelineRunLifecyclePredicate().Update(contextEvent)).To(BeTrue())
100+
})
101+
102+
It("should trigger when finalizers change on a release PipelineRun", func() {
103+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
104+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
105+
Build()
106+
Expect(err).NotTo(HaveOccurred())
107+
108+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
109+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
110+
WithFinalizer(metadata.ReleaseFinalizer).
111+
Build()
112+
Expect(err).NotTo(HaveOccurred())
113+
114+
contextEvent := event.UpdateEvent{
115+
ObjectOld: oldPipelineRun,
116+
ObjectNew: newPipelineRun,
117+
}
118+
119+
Expect(ReleasePipelineRunLifecyclePredicate().Update(contextEvent)).To(BeTrue())
120+
})
121+
122+
It("should trigger when finalizers are removed from a release PipelineRun", func() {
123+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
124+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
125+
WithFinalizer(metadata.ReleaseFinalizer).
126+
WithFinalizer("tekton.dev/finalizer").
127+
Build()
128+
Expect(err).NotTo(HaveOccurred())
129+
130+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
131+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
132+
WithFinalizer("tekton.dev/finalizer").
133+
Build()
134+
Expect(err).NotTo(HaveOccurred())
135+
136+
contextEvent := event.UpdateEvent{
137+
ObjectOld: oldPipelineRun,
138+
ObjectNew: newPipelineRun,
139+
}
140+
141+
Expect(ReleasePipelineRunLifecyclePredicate().Update(contextEvent)).To(BeTrue())
142+
})
143+
144+
It("should not trigger when finalizers are unchanged", func() {
145+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
146+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
147+
WithFinalizer(metadata.ReleaseFinalizer).
148+
Build()
149+
Expect(err).NotTo(HaveOccurred())
150+
151+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
152+
WithLabels(map[string]string{metadata.PipelinesTypeLabel: metadata.ManagedPipelineType.String()}).
153+
WithFinalizer(metadata.ReleaseFinalizer).
154+
Build()
155+
Expect(err).NotTo(HaveOccurred())
156+
157+
newPipelineRun.Status.MarkRunning("Test", "Running")
158+
159+
contextEvent := event.UpdateEvent{
160+
ObjectOld: oldPipelineRun,
161+
ObjectNew: newPipelineRun,
162+
}
163+
164+
Expect(ReleasePipelineRunLifecyclePredicate().Update(contextEvent)).To(BeFalse())
165+
})
166+
167+
It("should not trigger when finalizers change on non-release PipelineRuns", func() {
168+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
169+
Build()
170+
Expect(err).NotTo(HaveOccurred())
171+
172+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
173+
WithFinalizer("some.other/finalizer").
174+
Build()
175+
Expect(err).NotTo(HaveOccurred())
176+
177+
contextEvent := event.UpdateEvent{
178+
ObjectOld: oldPipelineRun,
179+
ObjectNew: newPipelineRun,
180+
}
181+
182+
Expect(ReleasePipelineRunLifecyclePredicate().Update(contextEvent)).To(BeFalse())
75183
})
76184
})
77185
})

‎tekton/utils.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ limitations under the License.
1717
package tekton
1818

1919
import (
20+
"reflect"
21+
2022
"github.com/konflux-ci/release-service/metadata"
2123
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
2224
"knative.dev/pkg/apis"
@@ -48,3 +50,16 @@ func hasPipelineSucceeded(object client.Object) bool {
4850

4951
return false
5052
}
53+
54+
// hasFinalizersChanged returns true if the finalizers have changed between old and new objects.
55+
// This helps detect when other controllers (like Tekton) are modifying finalizers during deletion.
56+
func hasFinalizersChanged(oldObj, newObj client.Object) bool {
57+
if oldObj == nil || newObj == nil {
58+
return false
59+
}
60+
61+
oldFinalizers := oldObj.GetFinalizers()
62+
newFinalizers := newObj.GetFinalizers()
63+
64+
return !reflect.DeepEqual(oldFinalizers, newFinalizers)
65+
}

‎tekton/utils_test.go‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,94 @@ var _ = Describe("Utils", Ordered, func() {
8686
Expect(hasPipelineSucceeded(pipelineRun)).To(BeTrue())
8787
})
8888
})
89+
90+
When("hasFinalizersChanged is called", func() {
91+
It("should return false when both objects are nil", func() {
92+
Expect(hasFinalizersChanged(nil, nil)).To(BeFalse())
93+
})
94+
95+
It("should return false when the old object is nil", func() {
96+
pipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").Build()
97+
Expect(err).NotTo(HaveOccurred())
98+
Expect(hasFinalizersChanged(nil, pipelineRun)).To(BeFalse())
99+
})
100+
101+
It("should return false when the new object is nil", func() {
102+
pipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").Build()
103+
Expect(err).NotTo(HaveOccurred())
104+
Expect(hasFinalizersChanged(pipelineRun, nil)).To(BeFalse())
105+
})
106+
107+
It("should return false when the finalizers are identical", func() {
108+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
109+
WithFinalizer("finalizer1").
110+
WithFinalizer("finalizer2").
111+
Build()
112+
Expect(err).NotTo(HaveOccurred())
113+
114+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
115+
WithFinalizer("finalizer1").
116+
WithFinalizer("finalizer2").
117+
Build()
118+
Expect(err).NotTo(HaveOccurred())
119+
120+
Expect(hasFinalizersChanged(oldPipelineRun, newPipelineRun)).To(BeFalse())
121+
})
122+
123+
It("should return true when finalizers are added to the new object", func() {
124+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
125+
WithFinalizer("finalizer1").
126+
Build()
127+
Expect(err).NotTo(HaveOccurred())
128+
129+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
130+
WithFinalizer("finalizer1").
131+
WithFinalizer("finalizer2").
132+
Build()
133+
Expect(err).NotTo(HaveOccurred())
134+
135+
Expect(hasFinalizersChanged(oldPipelineRun, newPipelineRun)).To(BeTrue())
136+
})
137+
138+
It("should return true when finalizers are removed from the new object", func() {
139+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
140+
WithFinalizer("finalizer1").
141+
WithFinalizer("finalizer2").
142+
Build()
143+
Expect(err).NotTo(HaveOccurred())
144+
145+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
146+
WithFinalizer("finalizer1").
147+
Build()
148+
Expect(err).NotTo(HaveOccurred())
149+
150+
Expect(hasFinalizersChanged(oldPipelineRun, newPipelineRun)).To(BeTrue())
151+
})
152+
153+
It("should return true when finalizers are reordered in the new object", func() {
154+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
155+
WithFinalizer("finalizer1").
156+
WithFinalizer("finalizer2").
157+
Build()
158+
Expect(err).NotTo(HaveOccurred())
159+
160+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").
161+
WithFinalizer("finalizer2").
162+
WithFinalizer("finalizer1").
163+
Build()
164+
Expect(err).NotTo(HaveOccurred())
165+
166+
Expect(hasFinalizersChanged(oldPipelineRun, newPipelineRun)).To(BeTrue())
167+
})
168+
169+
It("should return false when both objects have no finalizers", func() {
170+
oldPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").Build()
171+
Expect(err).NotTo(HaveOccurred())
172+
173+
newPipelineRun, err := utils.NewPipelineRunBuilder("pipeline-run", "default").Build()
174+
Expect(err).NotTo(HaveOccurred())
175+
176+
Expect(hasFinalizersChanged(oldPipelineRun, newPipelineRun)).To(BeFalse())
177+
})
178+
})
89179
})

0 commit comments

Comments
 (0)