Skip to content

Commit c2f628c

Browse files
ishtmeet.singhoz-agent
authored and
ishtmeet.singh
committed
feat: cascade delete all children on pipeline delete
What: - Replace placeholder with DeleteAllTriggerRuns + DeleteAllPipelineRuns calls (fatal errors cause requeue, best-effort revision cleanup follows). - Wire revision.Manager into the Pipeline Reconciler (defaults to NoOpManager) and invoke DeleteAllRevisions after children are deleted, mirroring the deployment controller's pattern. - Remove finalizer only after all children successfully deleted. - Add TestCascadeDelete_AllTerminal, TestCascadeDelete_RevisionsCleaned, and TestCascadeDelete_SkippedPRsTerminal tests. Why: - Users running `ma pipeline delete` previously had to manually kill active TriggerRuns/PipelineRuns, wait for terminal states, and delete each child CR one by one. Now the controller does this transparently when a Pipeline CR is marked for deletion. How to test: - bazel test //go/components/pipeline/... (all new regression tests pass). - Manually: `ma pipeline delete` against a pipeline with terminal children and observe that child CRs are removed before the pipeline CR disappears. Breaking changes: - `ma pipeline delete <pipeline>` now cascades to all child TriggerRuns, PipelineRuns, and Revisions belonging to the pipeline. Every child CR is deleted along with the pipeline. There is no undo. - Deletion is asynchronous (finalizer removal happens after cascade completes). If you relied on `ma pipeline delete` returning once the Pipeline CR is actually gone from etcd, you now need to poll `ma pipeline get` or similar until NotFound. - Any external tooling that previously issued its own cleanup for TRs/PRs/ Revisions should be simplified: those children are now cleaned up by the controller. Double-delete is safe (NotFound is tolerated) but unnecessary. Co-Authored-By: Oz <oz-agent@warp.dev>
1 parent cece21f commit c2f628c

3 files changed

Lines changed: 195 additions & 3 deletions

File tree

‎go/components/pipeline/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ go_library(
1313
"//go/api:go_default_library",
1414
"//go/api/handler:go_default_library",
1515
"//go/base/env:go_default_library",
16+
"//go/base/revision:go_default_library",
1617
"//go/components/pipelinerun:go_default_library",
1718
"//go/components/triggerrun:go_default_library",
1819
"//proto/api:go_default_library",
@@ -37,6 +38,7 @@ go_test(
3738
"//go/api:go_default_library",
3839
"//go/api/handler:go_default_library",
3940
"//go/base/env:go_default_library",
41+
"//go/base/revision:go_default_library",
4042
"//go/components/pipelinerun:go_default_library",
4143
"//go/components/triggerrun:go_default_library",
4244
"//proto/api:go_default_library",

‎go/components/pipeline/controller.go‎

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/michelangelo-ai/michelangelo/go/api"
2222
apiHandler "github.com/michelangelo-ai/michelangelo/go/api/handler"
2323
"github.com/michelangelo-ai/michelangelo/go/base/env"
24+
"github.com/michelangelo-ai/michelangelo/go/base/revision"
2425
"github.com/michelangelo-ai/michelangelo/go/components/pipelinerun"
2526
"github.com/michelangelo-ai/michelangelo/go/components/triggerrun"
2627
apipb "github.com/michelangelo-ai/michelangelo/proto-go/api"
@@ -48,6 +49,7 @@ type Reconciler struct {
4849
apiHandlerFactory apiHandler.Factory
4950
triggerRunManager triggerrun.Manager
5051
pipelineRunManager pipelinerun.Manager
52+
revisionManager revision.Manager
5153
}
5254

5355
// Reconcile is the main reconciliation loop entry point for Pipeline resources.
@@ -224,11 +226,50 @@ func (r *Reconciler) handleDeletion(ctx context.Context, pipeline *v2pb.Pipeline
224226
return ctrl.Result{RequeueAfter: reconcileInterval}, nil
225227
}
226228

227-
// Delete steps will be added in subsequent PRs
228-
logger.Info("Children found, requeueing for cascade delete",
229+
// All children are terminal — delete them (fatal: error causes requeue)
230+
logger.Info("All children terminal, deleting",
229231
zap.Int("triggerRuns", len(triggerRuns)),
230232
zap.Int("pipelineRuns", len(pipelineRuns)))
231-
return ctrl.Result{RequeueAfter: reconcileInterval}, nil
233+
234+
if err := r.triggerRunManager.DeleteAllTriggerRuns(ctx, pipeline.Namespace, pipeline.Name); err != nil {
235+
logger.Error("Failed to delete trigger runs during cascade delete",
236+
zap.Error(err),
237+
zap.String("operation", "delete_all_trigger_runs"),
238+
zap.String("namespace", pipeline.Namespace),
239+
zap.String("name", pipeline.Name))
240+
return ctrl.Result{}, fmt.Errorf("delete trigger runs for pipeline %s/%s: %w", pipeline.Namespace, pipeline.Name, err)
241+
}
242+
if err := r.pipelineRunManager.DeleteAllPipelineRuns(ctx, pipeline.Namespace, pipeline.Name); err != nil {
243+
logger.Error("Failed to delete pipeline runs during cascade delete",
244+
zap.Error(err),
245+
zap.String("operation", "delete_all_pipeline_runs"),
246+
zap.String("namespace", pipeline.Namespace),
247+
zap.String("name", pipeline.Name))
248+
return ctrl.Result{}, fmt.Errorf("delete pipeline runs for pipeline %s/%s: %w", pipeline.Namespace, pipeline.Name, err)
249+
}
250+
251+
// Best-effort revision cleanup. Revisions are a tracking concern; failing to
252+
// clean them up should not block finalizer removal. The deployment controller
253+
// follows the same pattern in go/components/deployment/controller.go.
254+
if err := r.revisionManager.DeleteAllRevisions(ctx, pipeline.Namespace, pipeline.Name, "Pipeline"); err != nil {
255+
logger.Warn("Failed to delete revisions during cascade delete",
256+
zap.Error(err),
257+
zap.String("operation", "delete_all_revisions"),
258+
zap.String("namespace", pipeline.Namespace),
259+
zap.String("name", pipeline.Name))
260+
}
261+
262+
logger.Info("All children deleted, removing finalizer")
263+
controllerutil.RemoveFinalizer(pipeline, api.PipelineFinalizer)
264+
if err := r.Update(ctx, pipeline, &metav1.UpdateOptions{}); err != nil {
265+
logger.Error("Failed to remove finalizer after cascade delete",
266+
zap.Error(err),
267+
zap.String("operation", "remove_finalizer"),
268+
zap.String("namespace", pipeline.Namespace),
269+
zap.String("name", pipeline.Name))
270+
return ctrl.Result{}, fmt.Errorf("remove finalizer on pipeline %s/%s: %w", pipeline.Namespace, pipeline.Name, err)
271+
}
272+
return ctrl.Result{}, nil
232273
}
233274

234275
// formatRevisionName generates a standardized revision name for a pipeline.
@@ -270,6 +311,9 @@ func (r *Reconciler) Register(mgr ctrl.Manager) error {
270311
r.Handler = handler
271312
r.triggerRunManager = triggerrun.NewManager(handler, r.logger)
272313
r.pipelineRunManager = pipelinerun.NewManager(handler, r.logger)
314+
if r.revisionManager == nil {
315+
r.revisionManager = revision.NewNoOpManager()
316+
}
273317
return ctrl.NewControllerManagedBy(mgr).
274318
For(&v2pb.Pipeline{}).
275319
Complete(r)

‎go/components/pipeline/controller_test.go‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/michelangelo-ai/michelangelo/go/api"
1616
apiHandler "github.com/michelangelo-ai/michelangelo/go/api/handler"
1717
"github.com/michelangelo-ai/michelangelo/go/base/env"
18+
"github.com/michelangelo-ai/michelangelo/go/base/revision"
1819
"github.com/michelangelo-ai/michelangelo/go/components/pipelinerun"
1920
"github.com/michelangelo-ai/michelangelo/go/components/triggerrun"
2021
apipb "github.com/michelangelo-ai/michelangelo/proto-go/api"
@@ -405,7 +406,58 @@ func TestCascadeDelete_ActivePipelineRuns(t *testing.T) {
405406
require.True(t, updatedPR.Spec.Kill)
406407
}
407408

409+
func TestCascadeDelete_AllTerminal(t *testing.T) {
410+
now := metav1.Now()
411+
pipeline := &v2pb.Pipeline{
412+
ObjectMeta: metav1.ObjectMeta{
413+
Name: "test-pipeline",
414+
Namespace: "test-namespace",
415+
Finalizers: []string{api.PipelineFinalizer},
416+
DeletionTimestamp: &now,
417+
},
418+
Spec: v2pb.PipelineSpec{
419+
Commit: &v2pb.CommitInfo{GitRef: "abc123", Branch: "main"},
420+
},
421+
}
422+
killedTR := &v2pb.TriggerRun{
423+
ObjectMeta: metav1.ObjectMeta{Name: "tr-killed", Namespace: "test-namespace"},
424+
Spec: v2pb.TriggerRunSpec{
425+
Pipeline: &apipb.ResourceIdentifier{Name: "test-pipeline", Namespace: "test-namespace"},
426+
},
427+
Status: v2pb.TriggerRunStatus{State: v2pb.TRIGGER_RUN_STATE_KILLED},
428+
}
429+
succeededPR := &v2pb.PipelineRun{
430+
ObjectMeta: metav1.ObjectMeta{Name: "pr-succeeded", Namespace: "test-namespace"},
431+
Spec: v2pb.PipelineRunSpec{
432+
Pipeline: &apipb.ResourceIdentifier{Name: "test-pipeline", Namespace: "test-namespace"},
433+
},
434+
Status: v2pb.PipelineRunStatus{State: v2pb.PIPELINE_RUN_STATE_SUCCEEDED},
435+
}
436+
437+
reconciler := setUpReconciler(t, []client.Object{pipeline, killedTR, succeededPR}, env.Context{})
438+
result, err := reconciler.Reconcile(context.Background(), ctrl.Request{
439+
NamespacedName: types.NamespacedName{Name: "test-pipeline", Namespace: "test-namespace"},
440+
})
441+
require.NoError(t, err)
442+
require.Equal(t, ctrl.Result{}, result)
443+
444+
// Verify children were deleted
445+
trList := &v2pb.TriggerRunList{}
446+
require.NoError(t, reconciler.Handler.List(context.Background(), "test-namespace",
447+
&metav1.ListOptions{}, nil, trList))
448+
require.Empty(t, trList.Items)
449+
450+
prList := &v2pb.PipelineRunList{}
451+
require.NoError(t, reconciler.Handler.List(context.Background(), "test-namespace",
452+
&metav1.ListOptions{}, nil, prList))
453+
require.Empty(t, prList.Items)
454+
}
455+
408456
func setUpReconciler(t *testing.T, initialObjects []client.Object, env env.Context) *Reconciler {
457+
return setUpReconcilerWithRevisionManager(t, initialObjects, env, revision.NewNoOpManager())
458+
}
459+
460+
func setUpReconcilerWithRevisionManager(t *testing.T, initialObjects []client.Object, env env.Context, revMgr revision.Manager) *Reconciler {
409461
scheme := runtime.NewScheme()
410462
err := v2pb.AddToScheme(scheme)
411463
require.NoError(t, err)
@@ -421,6 +473,100 @@ func setUpReconciler(t *testing.T, initialObjects []client.Object, env env.Conte
421473
logger: logger,
422474
triggerRunManager: triggerrun.NewManager(handler, logger),
423475
pipelineRunManager: pipelinerun.NewManager(handler, logger),
476+
revisionManager: revMgr,
424477
}
425478
return reconciler
426479
}
480+
481+
// spyRevisionManager records DeleteAllRevisions calls for assertions.
482+
type spyRevisionManager struct {
483+
calls []spyRevisionManagerCall
484+
}
485+
486+
type spyRevisionManagerCall struct {
487+
namespace string
488+
name string
489+
resourceType string
490+
}
491+
492+
func (s *spyRevisionManager) UpsertRevision(ctx context.Context, deployment *v2pb.Deployment) error {
493+
return nil
494+
}
495+
496+
func (s *spyRevisionManager) DeleteAllRevisions(ctx context.Context, namespace, name, resourceType string) error {
497+
s.calls = append(s.calls, spyRevisionManagerCall{namespace: namespace, name: name, resourceType: resourceType})
498+
return nil
499+
}
500+
501+
func TestCascadeDelete_RevisionsCleaned(t *testing.T) {
502+
now := metav1.Now()
503+
pipeline := &v2pb.Pipeline{
504+
ObjectMeta: metav1.ObjectMeta{
505+
Name: "test-pipeline",
506+
Namespace: "test-namespace",
507+
Finalizers: []string{api.PipelineFinalizer},
508+
DeletionTimestamp: &now,
509+
},
510+
Spec: v2pb.PipelineSpec{
511+
Commit: &v2pb.CommitInfo{GitRef: "abc123", Branch: "main"},
512+
},
513+
}
514+
killedTR := &v2pb.TriggerRun{
515+
ObjectMeta: metav1.ObjectMeta{Name: "tr-killed", Namespace: "test-namespace"},
516+
Spec: v2pb.TriggerRunSpec{
517+
Pipeline: &apipb.ResourceIdentifier{Name: "test-pipeline", Namespace: "test-namespace"},
518+
},
519+
Status: v2pb.TriggerRunStatus{State: v2pb.TRIGGER_RUN_STATE_KILLED},
520+
}
521+
spy := &spyRevisionManager{}
522+
reconciler := setUpReconcilerWithRevisionManager(t, []client.Object{pipeline, killedTR}, env.Context{}, spy)
523+
524+
result, err := reconciler.Reconcile(context.Background(), ctrl.Request{
525+
NamespacedName: types.NamespacedName{Name: "test-pipeline", Namespace: "test-namespace"},
526+
})
527+
require.NoError(t, err)
528+
require.Equal(t, ctrl.Result{}, result)
529+
530+
// Revision cleanup must be invoked exactly once with the pipeline identifier
531+
// and resource type "Pipeline".
532+
require.Len(t, spy.calls, 1)
533+
require.Equal(t, spyRevisionManagerCall{namespace: "test-namespace", name: "test-pipeline", resourceType: "Pipeline"}, spy.calls[0])
534+
}
535+
536+
func TestCascadeDelete_SkippedPRsTerminal(t *testing.T) {
537+
// A SKIPPED PR must be treated as terminal so cascade delete completes.
538+
// Before B1 this would have looped forever treating SKIPPED as active.
539+
now := metav1.Now()
540+
pipeline := &v2pb.Pipeline{
541+
ObjectMeta: metav1.ObjectMeta{
542+
Name: "test-pipeline",
543+
Namespace: "test-namespace",
544+
Finalizers: []string{api.PipelineFinalizer},
545+
DeletionTimestamp: &now,
546+
},
547+
Spec: v2pb.PipelineSpec{
548+
Commit: &v2pb.CommitInfo{GitRef: "abc123", Branch: "main"},
549+
},
550+
}
551+
skippedPR := &v2pb.PipelineRun{
552+
ObjectMeta: metav1.ObjectMeta{Name: "pr-skipped", Namespace: "test-namespace"},
553+
Spec: v2pb.PipelineRunSpec{
554+
Pipeline: &apipb.ResourceIdentifier{Name: "test-pipeline", Namespace: "test-namespace"},
555+
},
556+
Status: v2pb.PipelineRunStatus{State: v2pb.PIPELINE_RUN_STATE_SKIPPED},
557+
}
558+
559+
reconciler := setUpReconciler(t, []client.Object{pipeline, skippedPR}, env.Context{})
560+
result, err := reconciler.Reconcile(context.Background(), ctrl.Request{
561+
NamespacedName: types.NamespacedName{Name: "test-pipeline", Namespace: "test-namespace"},
562+
})
563+
require.NoError(t, err)
564+
// Cascade delete must complete (no requeue) because SKIPPED is terminal.
565+
require.Equal(t, ctrl.Result{}, result)
566+
567+
// Verify the SKIPPED PR was deleted along with the cascade.
568+
prList := &v2pb.PipelineRunList{}
569+
require.NoError(t, reconciler.Handler.List(context.Background(), "test-namespace",
570+
&metav1.ListOptions{}, nil, prList))
571+
require.Empty(t, prList.Items)
572+
}

0 commit comments

Comments
 (0)