Skip to content

Commit 1de67a8

Browse files
committed
fix(STONEINTG-1671): use IS-owned annot & finalizer 4 nudge processing
build-service's removePipelineFinalizer() unconditionally sets build.appstudio.openshift.io/component-nudge-processed="true" when skipping nudging for a NudgeConfig namespace. IS bailed out on any non-empty value of that annotation, so Renovate PipelineRuns were never created when NudgeConfig was present. Fix: - Rename NudgeProcessedAnnotation to integration.konflux-ci.dev/ component-nudge-processed so IS owns its marker independently of build-service's annotation - Add NudgePipelineRunFinalizer (integration.konflux-ci.dev/ nudge-pipelinerun) to hold the build PLR open between nudge PLR creation and the annotation write, preventing premature GC on a succeeded PLR; finalizer is added just before CreateNudgePipelineRun and removed after the annotation lands successfully; the already-annotated early-exit path cleans it up on crash-recovery Signed-off-by: Kasem Alem <kalem@redhat.com>
1 parent 51561ce commit 1de67a8

4 files changed

Lines changed: 77 additions & 3 deletions

File tree

helpers/finalizers.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,7 @@ package helpers
2020
const IntegrationPipelineRunFinalizer string = "test.appstudio.openshift.io/pipelinerun"
2121
const IntegrationTestScenarioFinalizer string = "test.appstudio.openshift.io/scenario"
2222
const ComponentFinalizer string = "test.appstudio.openshift.io/component"
23+
24+
// NudgePipelineRunFinalizer is the finalizer name added to build PipelineRuns while IS is
25+
// actively creating a nudge PipelineRun, preventing premature GC before nudging completes.
26+
const NudgePipelineRunFinalizer string = "test.appstudio.openshift.io/nudge-pipelinerun"

internal/controller/buildpipeline/buildpipeline_adapter.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,22 @@ func (a *Adapter) EnsureNudgePipelineRunsExist() (controller.OperationResult, er
161161
return controller.ContinueProcessing()
162162
}
163163

164+
// Idempotency guard: nudge already processed. Remove the nudge finalizer if it somehow
165+
// survived (e.g. IS crashed between nudge PLR creation and annotation write).
164166
if metadata.HasAnnotation(a.pipelineRun, tektonconsts.NudgeProcessedAnnotation) {
167+
if err := h.RemoveFinalizerFromPipelineRun(a.context, a.client, a.logger, a.pipelineRun, h.NudgePipelineRunFinalizer); err != nil && !errors.IsNotFound(err) {
168+
return controller.RequeueWithError(err)
169+
}
170+
return controller.ContinueProcessing()
171+
}
172+
173+
// If the PLR is being deleted, nudging can no longer be completed. Remove any stale nudge
174+
// finalizer (left by a crash between finalizer add and annotation write) so the PLR is
175+
// not stuck in Terminating.
176+
if a.pipelineRun.GetDeletionTimestamp() != nil {
177+
if err := h.RemoveFinalizerFromPipelineRun(a.context, a.client, a.logger, a.pipelineRun, h.NudgePipelineRunFinalizer); err != nil && !errors.IsNotFound(err) {
178+
return controller.RequeueWithError(err)
179+
}
165180
return controller.ContinueProcessing()
166181
}
167182

@@ -247,9 +262,16 @@ func (a *Adapter) EnsureNudgePipelineRunsExist() (controller.OperationResult, er
247262
return controller.ContinueProcessing()
248263
}
249264

265+
// Add the nudge finalizer just before creating the nudge PLR so the build PLR cannot be
266+
// GC'd between PLR creation and the annotation write that marks nudging as complete.
267+
if err := h.AddFinalizerToPipelineRun(a.context, a.client, a.logger, a.pipelineRun, h.NudgePipelineRunFinalizer); err != nil {
268+
return controller.RequeueWithError(err)
269+
}
270+
250271
err = nudging.CreateNudgePipelineRun(a.context, a.client, a.pipelineRun, targets, buildResult, simpleBranchName)
251272
if err != nil {
252273
a.logger.Error(err, "Failed to create nudge PipelineRun")
274+
// Keep the nudge finalizer so the PLR stays alive for the retry.
253275
return controller.RequeueWithError(err)
254276
}
255277

@@ -261,6 +283,11 @@ func (a *Adapter) EnsureNudgePipelineRunsExist() (controller.OperationResult, er
261283
err = tekton.AnnotateBuildPipelineRun(a.context, a.pipelineRun, tektonconsts.NudgeProcessedAnnotation, processedValue, a.client)
262284
if err != nil {
263285
a.logger.Error(err, "Failed to annotate build PLR as nudge-processed")
286+
// Keep the nudge finalizer — the PLR must not be GC'd before the annotation lands.
287+
return controller.RequeueWithError(err)
288+
}
289+
290+
if err = h.RemoveFinalizerFromPipelineRun(a.context, a.client, a.logger, a.pipelineRun, h.NudgePipelineRunFinalizer); err != nil && !errors.IsNotFound(err) {
264291
return controller.RequeueWithError(err)
265292
}
266293

internal/controller/buildpipeline/buildpipeline_adapter_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4004,6 +4004,45 @@ var _ = Describe("Pipeline Adapter", Ordered, func() {
40044004
Expect(result.CancelRequest).To(BeFalse())
40054005
})
40064006

4007+
It("removes the stale nudge finalizer when PLR is already annotated as nudge-processed", func() {
4008+
// Simulate crash-recovery: IS crashed between nudge PLR creation and annotation
4009+
// write, leaving the finalizer on a PLR that already has the processed annotation.
4010+
// We use a unique name that is not on the server so the Patch returns NotFound,
4011+
// which the idempotency guard path handles gracefully (no requeue). The important
4012+
// invariant is that the in-memory finalizer is stripped and no error is returned.
4013+
crashPLR := makePushPLR()
4014+
crashPLR.Name = "pipelinerun-nudge-crash-nonexistent"
4015+
crashPLR.ResourceVersion = "1" // MergeFromWithOptimisticLock requires non-empty RV; server returns NotFound (handled gracefully)
4016+
crashPLR.Annotations[tektonconsts.NudgeProcessedAnnotation] = "component-b"
4017+
controllerutil.AddFinalizer(crashPLR, helpers.NudgePipelineRunFinalizer)
4018+
crashPLR.Status = buildPipelineRun.Status
4019+
4020+
adapter = NewAdapter(ctx, crashPLR, hasComp, &[]v1beta2.ComponentGroup{*hasCompGroup}, logger, loader.NewMockLoader(), k8sClient)
4021+
result, err := adapter.EnsureNudgePipelineRunsExist()
4022+
Expect(err).NotTo(HaveOccurred())
4023+
Expect(result.CancelRequest).To(BeFalse())
4024+
Expect(controllerutil.ContainsFinalizer(crashPLR, helpers.NudgePipelineRunFinalizer)).To(BeFalse())
4025+
})
4026+
4027+
It("removes stale nudge finalizer from a deleting PLR with no nudge-processed annotation", func() {
4028+
// Simulate crash-recovery: IS crashed after adding the nudge finalizer but before
4029+
// writing the annotation, and the PLR was subsequently deleted. The PLR is stuck in
4030+
// Terminating. Verify the adapter removes the stale finalizer and returns without error.
4031+
deletingPLR := makePushPLR()
4032+
deletingPLR.Name = "pipelinerun-nudge-deleting-nonexistent"
4033+
deletingPLR.ResourceVersion = "1"
4034+
now := metav1.Now()
4035+
deletingPLR.DeletionTimestamp = &now
4036+
controllerutil.AddFinalizer(deletingPLR, helpers.NudgePipelineRunFinalizer)
4037+
deletingPLR.Status = buildPipelineRun.Status
4038+
4039+
adapter = NewAdapter(ctx, deletingPLR, hasComp, &[]v1beta2.ComponentGroup{*hasCompGroup}, logger, loader.NewMockLoader(), k8sClient)
4040+
result, err := adapter.EnsureNudgePipelineRunsExist()
4041+
Expect(err).NotTo(HaveOccurred())
4042+
Expect(result.CancelRequest).To(BeFalse())
4043+
Expect(controllerutil.ContainsFinalizer(deletingPLR, helpers.NudgePipelineRunFinalizer)).To(BeFalse())
4044+
})
4045+
40074046
It("skips when NudgeConfig is not found", func() {
40084047
pushPLR := makePushPLR()
40094048
adapter = NewAdapter(ctx, pushPLR, hasComp, &[]v1beta2.ComponentGroup{*hasCompGroup}, logger, loader.NewMockLoader(), k8sClient)

tekton/consts/consts.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,15 @@ const (
145145
PipelineRunShouldReleaseResultName = "SHOULD_RELEASE"
146146

147147
/*
148-
* Nudge PipelineRun constants — build-service compatible annotation/label names
148+
* Nudge PipelineRun constants
149149
*/
150150

151-
// NudgeProcessedAnnotation marks a build PLR as having already been processed for nudging
152-
NudgeProcessedAnnotation = "build.appstudio.openshift.io/component-nudge-processed"
151+
// NudgeProcessedAnnotation marks a build PLR as having already been processed for nudging.
152+
// Renamed from build.appstudio.openshift.io/component-nudge-processed to avoid interference
153+
// with build-service, which sets the old key unconditionally. PLRs annotated with the old key
154+
// by IS before this change are not recognized by the idempotency guard, but any duplicate
155+
// nudge PLR creation is safe because Renovate is idempotent.
156+
NudgeProcessedAnnotation = "test.appstudio.openshift.io/component-nudge-processed"
153157

154158
// NudgeSimpleBranchAnnotation on a Component controls simplified branch naming for nudge PRs
155159
NudgeSimpleBranchAnnotation = "build.appstudio.openshift.io/build-nudge-simple-branch"

0 commit comments

Comments
 (0)