Skip to content

Commit 815063d

Browse files
authored
Skip retired/lost workers in janitor orphan list (#405)
The janitor's ListOrphanedWorkers query included workers in the retired and lost states alongside active states. When a worker's owning CP is expired, retireClaimedWorker re-persists the row with the original expired CP as owner, so the orphan query keeps re-listing the same row on every cycle. retireWorkerPod then loops on a 404 from the K8s pod delete: Retiring K8s worker. id=685 pod=duckgres-worker-...-685 Failed to delete worker pod. error="pods ... not found" The original reason for including retired/lost was to let a successor janitor finish a K8s pod delete that the dying CP didn't complete. That case is now handled authoritatively by the K8s label-based startup scan in cleanupOrphanedWorkerPods (4e96d11), so the inclusion is redundant and harmful. Excludes WorkerStateRetired and WorkerStateLost from the cleanup states. First-time orphans still get processed: their state is reserved/idle/ spawning/etc. when the janitor first sees them, transitions to retired inside retireClaimedWorker, then the next cycle skips the row. Updates TestListOrphanedAndStuckWorkersPostgres to assert the new behavior with both retired and lost orphan rows present.
1 parent 9164d8e commit 815063d

2 files changed

Lines changed: 23 additions & 10 deletions

File tree

‎controlplane/configstore/store.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -784,9 +784,11 @@ func (cs *ConfigStore) CreateNeutralWarmWorkerSlot(ownerCPInstanceID, podNamePre
784784

785785
// ListOrphanedWorkers returns workers whose owning control-plane instance has
786786
// already been marked expired long enough ago to pass the orphan grace cutoff.
787-
// Retired/lost rows are included so a replacement janitor can finish deleting
788-
// worker pods when the original control plane died after persisting retirement
789-
// but before the Kubernetes delete completed.
787+
// Retired/lost rows are deliberately excluded: their pods are either already
788+
// gone (in which case re-listing the row would loop the janitor on a 404 from
789+
// the K8s delete forever) or were leaked when the previous CP died mid-delete,
790+
// and that leak case is handled authoritatively by the K8s label-based startup
791+
// scan in K8sWorkerPool.cleanupOrphanedWorkerPods.
790792
func (cs *ConfigStore) ListOrphanedWorkers(before time.Time) ([]WorkerRecord, error) {
791793
var workers []WorkerRecord
792794
cleanupStates := []WorkerState{
@@ -797,8 +799,6 @@ func (cs *ConfigStore) ListOrphanedWorkers(before time.Time) ([]WorkerRecord, er
797799
WorkerStateHot,
798800
WorkerStateHotIdle,
799801
WorkerStateDraining,
800-
WorkerStateRetired,
801-
WorkerStateLost,
802802
}
803803
workerTable := cs.runtimeTable((&WorkerRecord{}).TableName())
804804
cpTable := cs.runtimeTable((&ControlPlaneInstance{}).TableName())

‎tests/configstore/runtime_store_postgres_test.go‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,10 @@ func TestListOrphanedAndStuckWorkersPostgres(t *testing.T) {
476476
}); err != nil {
477477
t.Fatalf("UpsertWorkerRecord(orphaned): %v", err)
478478
}
479+
// Retired and lost rows whose owning CP is expired must NOT be returned:
480+
// their pods are already deleted (or will be cleaned up by the K8s label
481+
// scan on the next CP startup), so re-listing them would loop the janitor
482+
// on a 404 from the K8s pod delete forever.
479483
if err := store.UpsertWorkerRecord(&configstore.WorkerRecord{
480484
WorkerID: 63,
481485
PodName: "duckgres-worker-63",
@@ -488,6 +492,18 @@ func TestListOrphanedAndStuckWorkersPostgres(t *testing.T) {
488492
}); err != nil {
489493
t.Fatalf("UpsertWorkerRecord(retired orphan): %v", err)
490494
}
495+
if err := store.UpsertWorkerRecord(&configstore.WorkerRecord{
496+
WorkerID: 64,
497+
PodName: "duckgres-worker-64",
498+
State: configstore.WorkerStateLost,
499+
OrgID: "analytics",
500+
OwnerCPInstanceID: "cp-expired:boot-a",
501+
OwnerEpoch: 4,
502+
LastHeartbeatAt: now.Add(-time.Minute),
503+
RetireReason: "crash",
504+
}); err != nil {
505+
t.Fatalf("UpsertWorkerRecord(lost orphan): %v", err)
506+
}
491507
if err := store.UpsertWorkerRecord(&configstore.WorkerRecord{
492508
WorkerID: 62,
493509
PodName: "duckgres-worker-62",
@@ -509,11 +525,8 @@ func TestListOrphanedAndStuckWorkersPostgres(t *testing.T) {
509525
if err != nil {
510526
t.Fatalf("ListOrphanedWorkers: %v", err)
511527
}
512-
if len(orphaned) != 2 {
513-
t.Fatalf("expected orphaned worker 61, got %#v", orphaned)
514-
}
515-
if orphaned[0].WorkerID != 61 || orphaned[1].WorkerID != 63 {
516-
t.Fatalf("expected orphaned workers 61 and 63, got %#v", orphaned)
528+
if len(orphaned) != 1 || orphaned[0].WorkerID != 61 {
529+
t.Fatalf("expected only active orphaned worker 61, got %#v", orphaned)
517530
}
518531

519532
stuck, err := store.ListStuckWorkers(now.Add(-2*time.Minute), now.Add(-2*time.Minute))

0 commit comments

Comments
 (0)