Skip to content

Commit 851521f

Browse files
ishtmeet.singhishtoo1
authored andcommitted
feat: cascade kill active TriggerRuns on pipeline delete
Summary: Intent: - Kill active TriggerRuns before deleting them during pipeline cascade delete Changes: - Add kill-active-TriggerRuns step in handleDeletion: lists active TRs, sets KILL action on each (best-effort), requeues - Add TestCascadeDelete_ActiveTriggerRuns verifying KILL action set on running TR and requeue Test Plan: - go test ./components/pipeline/... -v -count=1 (all tests pass) Revert Plan: Revert this PR via git revert. Jira Issues:
1 parent 725de61 commit 851521f

2 files changed

Lines changed: 156 additions & 9 deletions

File tree

go/components/pipeline/controller.go

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,30 @@ func (r *Reconciler) handleDeletion(ctx context.Context, pipeline *v2pb.Pipeline
178178
return ctrl.Result{}, nil
179179
}
180180

181-
// Kill and delete steps will be added in subsequent PRs
181+
// Kill active TriggerRuns (best-effort)
182+
activeTRs, err := r.triggerRunManager.ListActiveTriggerRunsForPipeline(ctx, pipeline.Namespace, pipeline.Name)
183+
if err != nil {
184+
logger.Error("Failed to list active trigger runs for cascade delete",
185+
zap.Error(err),
186+
zap.String("operation", "list_active_trigger_runs"),
187+
zap.String("namespace", pipeline.Namespace),
188+
zap.String("name", pipeline.Name))
189+
return ctrl.Result{}, fmt.Errorf("list active trigger runs for pipeline %s/%s: %w", pipeline.Namespace, pipeline.Name, err)
190+
}
191+
if len(activeTRs) > 0 {
192+
for _, tr := range activeTRs {
193+
if killErr := r.triggerRunManager.KillTriggerRun(ctx, tr); killErr != nil {
194+
logger.Error("Failed to kill trigger run during cascade delete",
195+
zap.Error(killErr),
196+
zap.String("operation", "kill_trigger_run"),
197+
zap.String("namespace", tr.Namespace),
198+
zap.String("name", tr.Name))
199+
}
200+
}
201+
return ctrl.Result{RequeueAfter: reconcileInterval}, nil
202+
}
203+
204+
// Kill and delete steps for PipelineRuns will be added in subsequent PRs
182205
logger.Info("Children found, requeueing for cascade delete",
183206
zap.Int("triggerRuns", len(triggerRuns)),
184207
zap.Int("pipelineRuns", len(pipelineRuns)))

go/components/pipeline/controller_test.go

Lines changed: 132 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -478,9 +478,7 @@ func TestCascadeDelete_RemoveFinalizer_UpdateError(t *testing.T) {
478478
require.Contains(t, err.Error(), "remove finalizer on pipeline test-namespace/test-pipeline")
479479
}
480480

481-
func TestCascadeDelete_WithChildrenRequeues(t *testing.T) {
482-
// When children exist, handleDeletion does not remove the finalizer; it
483-
// requeues after reconcileInterval so a subsequent PR can perform kill/delete.
481+
func TestCascadeDelete_ActiveTriggerRuns(t *testing.T) {
484482
now := metav1.Now()
485483
pipeline := &v2pb.Pipeline{
486484
ObjectMeta: metav1.ObjectMeta{
@@ -493,25 +491,151 @@ func TestCascadeDelete_WithChildrenRequeues(t *testing.T) {
493491
Commit: &v2pb.CommitInfo{GitRef: "abc123", Branch: "main"},
494492
},
495493
}
496-
tr := &v2pb.TriggerRun{
494+
runningTR := &v2pb.TriggerRun{
497495
ObjectMeta: metav1.ObjectMeta{Name: "tr-running", Namespace: "test-namespace"},
498496
Spec: v2pb.TriggerRunSpec{
499497
Pipeline: &apipb.ResourceIdentifier{Name: "test-pipeline", Namespace: "test-namespace"},
500498
},
501499
Status: v2pb.TriggerRunStatus{State: v2pb.TRIGGER_RUN_STATE_RUNNING},
502500
}
503-
reconciler := setUpReconciler(t, []client.Object{pipeline, tr}, env.Context{})
501+
killedTR := &v2pb.TriggerRun{
502+
ObjectMeta: metav1.ObjectMeta{Name: "tr-killed", Namespace: "test-namespace"},
503+
Spec: v2pb.TriggerRunSpec{
504+
Pipeline: &apipb.ResourceIdentifier{Name: "test-pipeline", Namespace: "test-namespace"},
505+
},
506+
Status: v2pb.TriggerRunStatus{State: v2pb.TRIGGER_RUN_STATE_KILLED},
507+
}
504508

509+
reconciler := setUpReconciler(t, []client.Object{pipeline, runningTR, killedTR}, env.Context{})
505510
result, err := reconciler.Reconcile(context.Background(), ctrl.Request{
506511
NamespacedName: types.NamespacedName{Name: "test-pipeline", Namespace: "test-namespace"},
507512
})
508513
require.NoError(t, err)
509514
require.Equal(t, ctrl.Result{RequeueAfter: reconcileInterval}, result)
510515

516+
// Verify the running TR was marked for kill (both deprecated Spec.Kill and new Spec.Action)
517+
updated := &v2pb.TriggerRun{}
518+
require.NoError(t, reconciler.Get(context.Background(), "test-namespace", "tr-running", &metav1.GetOptions{}, updated))
519+
require.Equal(t, v2pb.TRIGGER_RUN_ACTION_KILL, updated.Spec.Action)
520+
require.True(t, updated.Spec.Kill)
521+
511522
// Finalizer should NOT have been removed yet.
512-
updated := &v2pb.Pipeline{}
513-
require.NoError(t, reconciler.Get(context.Background(), "test-namespace", "test-pipeline", &metav1.GetOptions{}, updated))
514-
require.True(t, controllerutil.ContainsFinalizer(updated, api.PipelineFinalizer))
523+
updatedPipeline := &v2pb.Pipeline{}
524+
require.NoError(t, reconciler.Get(context.Background(), "test-namespace", "test-pipeline", &metav1.GetOptions{}, updatedPipeline))
525+
require.True(t, controllerutil.ContainsFinalizer(updatedPipeline, api.PipelineFinalizer))
526+
}
527+
528+
// stubTriggerRunManager implements triggerrun.Manager with configurable behavior
529+
// so we can exercise error branches in handleDeletion that the real handler
530+
// can't deterministically trigger (e.g. ListActive after List succeeded).
531+
type stubTriggerRunManager struct {
532+
listAll []*v2pb.TriggerRun
533+
listAllErr error
534+
listActive []*v2pb.TriggerRun
535+
listActiveErr error
536+
killErrByName map[string]error
537+
killedNames []string
538+
}
539+
540+
func (s *stubTriggerRunManager) ListTriggerRunsForPipeline(ctx context.Context, namespace, pipelineName string) ([]*v2pb.TriggerRun, error) {
541+
return s.listAll, s.listAllErr
542+
}
543+
544+
func (s *stubTriggerRunManager) ListActiveTriggerRunsForPipeline(ctx context.Context, namespace, pipelineName string) ([]*v2pb.TriggerRun, error) {
545+
return s.listActive, s.listActiveErr
546+
}
547+
548+
func (s *stubTriggerRunManager) KillTriggerRun(ctx context.Context, tr *v2pb.TriggerRun) error {
549+
s.killedNames = append(s.killedNames, tr.Name)
550+
if err, ok := s.killErrByName[tr.Name]; ok {
551+
return err
552+
}
553+
return nil
554+
}
555+
556+
func (s *stubTriggerRunManager) DeleteAllTriggerRuns(ctx context.Context, namespace, pipelineName string) error {
557+
return nil
558+
}
559+
560+
func setUpReconcilerWithStubTR(t *testing.T, initialObjects []client.Object, trMgr triggerrun.Manager) *Reconciler {
561+
scheme := runtime.NewScheme()
562+
require.NoError(t, v2pb.AddToScheme(scheme))
563+
k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initialObjects...).WithStatusSubresource(initialObjects...).Build()
564+
logger := zaptest.NewLogger(t)
565+
handler := apiHandler.NewFakeAPIHandler(k8sClient)
566+
return &Reconciler{
567+
Handler: handler,
568+
logger: logger,
569+
triggerRunManager: trMgr,
570+
pipelineRunManager: pipelinerun.NewManager(handler, logger),
571+
}
572+
}
573+
574+
func TestCascadeDelete_ListActiveTriggerRunsError(t *testing.T) {
575+
now := metav1.Now()
576+
pipeline := &v2pb.Pipeline{
577+
ObjectMeta: metav1.ObjectMeta{
578+
Name: "test-pipeline",
579+
Namespace: "test-namespace",
580+
Finalizers: []string{api.PipelineFinalizer},
581+
DeletionTimestamp: &now,
582+
},
583+
Spec: v2pb.PipelineSpec{
584+
Commit: &v2pb.CommitInfo{GitRef: "abc123", Branch: "main"},
585+
},
586+
}
587+
activeErr := errors.New("list active boom")
588+
trStub := &stubTriggerRunManager{
589+
// First List (children check) returns one TR so we proceed past the empty-children branch.
590+
listAll: []*v2pb.TriggerRun{
591+
{ObjectMeta: metav1.ObjectMeta{Name: "tr-1", Namespace: "test-namespace"}},
592+
},
593+
listActiveErr: activeErr,
594+
}
595+
reconciler := setUpReconcilerWithStubTR(t, []client.Object{pipeline}, trStub)
596+
597+
_, err := reconciler.Reconcile(context.Background(), ctrl.Request{
598+
NamespacedName: types.NamespacedName{Name: "test-pipeline", Namespace: "test-namespace"},
599+
})
600+
require.Error(t, err)
601+
require.ErrorIs(t, err, activeErr)
602+
require.Contains(t, err.Error(), "list active trigger runs for pipeline test-namespace/test-pipeline")
603+
}
604+
605+
func TestCascadeDelete_KillTriggerRunError_LogsAndContinues(t *testing.T) {
606+
// KillTriggerRun errors are logged but do not cause handleDeletion to fail;
607+
// it still requeues so subsequent reconciles can retry.
608+
now := metav1.Now()
609+
pipeline := &v2pb.Pipeline{
610+
ObjectMeta: metav1.ObjectMeta{
611+
Name: "test-pipeline",
612+
Namespace: "test-namespace",
613+
Finalizers: []string{api.PipelineFinalizer},
614+
DeletionTimestamp: &now,
615+
},
616+
Spec: v2pb.PipelineSpec{
617+
Commit: &v2pb.CommitInfo{GitRef: "abc123", Branch: "main"},
618+
},
619+
}
620+
active := []*v2pb.TriggerRun{
621+
{ObjectMeta: metav1.ObjectMeta{Name: "tr-bad", Namespace: "test-namespace"}},
622+
{ObjectMeta: metav1.ObjectMeta{Name: "tr-good", Namespace: "test-namespace"}},
623+
}
624+
trStub := &stubTriggerRunManager{
625+
listAll: active,
626+
listActive: active,
627+
killErrByName: map[string]error{"tr-bad": errors.New("kill boom")},
628+
}
629+
reconciler := setUpReconcilerWithStubTR(t, []client.Object{pipeline}, trStub)
630+
631+
result, err := reconciler.Reconcile(context.Background(), ctrl.Request{
632+
NamespacedName: types.NamespacedName{Name: "test-pipeline", Namespace: "test-namespace"},
633+
})
634+
require.NoError(t, err)
635+
require.Equal(t, ctrl.Result{RequeueAfter: reconcileInterval}, result)
636+
637+
// Both TRs were attempted despite the first failing.
638+
require.ElementsMatch(t, []string{"tr-bad", "tr-good"}, trStub.killedNames)
515639
}
516640

517641
func setUpReconciler(t *testing.T, initialObjects []client.Object, env env.Context) *Reconciler {

0 commit comments

Comments
 (0)