Skip to content

Commit 0d2f603

Browse files
fernando-villalbafernando-villalba
committed
feat(status): detect crash-looping pods as Degraded
 PhaseDegraded was only reachable through the Shard pool-pod path. Cell, TopoServer, and Shard MultiOrch controllers reported Progressing indefinitely when pods crash-looped, making the phase check in MultigresCluster dead code for those resources. - Add IsCrashLooping, AnyCrashLooping, ComputeWorkloadPhase shared helpers in pkg/util/status/ - Cell and TopoServer controllers list pods and use ComputeWorkloadPhase for crash-loop detection with 30s requeue - Shard updateMultiOrchStatus returns orchDegraded flag via AnyCrashLooping; updatePoolsStatus uses shared IsCrashLooping - Reorder Shard phase switch: Degraded > Healthy > Progressing - Move Shard updateStatus before reconcileDataPlane with 30s context timeout to prevent stale observedGeneration - Aggregate TopoServer phases in MultigresCluster status - Fix integration test expectations for materialized defaults Crash-looping pods now surface as Degraded across the entire resource hierarchy instead of silently staying Progressing.
1 parent d52b185 commit 0d2f603

16 files changed

+1006
-73
lines changed

‎config/samples/no-templates.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
apiVersion: multigres.com/v1alpha1
22
kind: MultigresCluster
33
metadata:
4-
name: inline
4+
name: i
55
namespace: default
66
spec:
77
pvcDeletionPolicy:
@@ -81,7 +81,7 @@ spec:
8181
type: "readWrite"
8282
cells:
8383
- "z1"
84-
replicasPerCell: 3
84+
replicasPerCell: 5
8585
storage:
8686
# class: "standard"
8787
size: "100Gi"

‎pkg/cluster-handler/controller/multigrescluster/integration_resolution_enforcement_test.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,11 @@ func TestMultigresCluster_ResolutionLogic(t *testing.T) {
341341
WhenDeleted: multigresv1alpha1.RetainPVCRetentionPolicy,
342342
WhenScaled: multigresv1alpha1.RetainPVCRetentionPolicy,
343343
},
344+
Backup: &multigresv1alpha1.BackupConfig{
345+
Type: multigresv1alpha1.BackupTypeFilesystem,
346+
Filesystem: &multigresv1alpha1.FilesystemBackupConfig{Path: resolver.DefaultBackupPath, Storage: multigresv1alpha1.StorageSpec{Size: resolver.DefaultBackupStorageSize}},
347+
},
348+
TopologyPruning: &multigresv1alpha1.TopologyPruningConfig{Enabled: ptr.To(true)},
344349
},
345350
}
346351

@@ -618,6 +623,11 @@ func TestMultigresCluster_TemplateOverrides(t *testing.T) {
618623
WhenDeleted: multigresv1alpha1.RetainPVCRetentionPolicy,
619624
WhenScaled: multigresv1alpha1.RetainPVCRetentionPolicy,
620625
},
626+
Backup: &multigresv1alpha1.BackupConfig{
627+
Type: multigresv1alpha1.BackupTypeFilesystem,
628+
Filesystem: &multigresv1alpha1.FilesystemBackupConfig{Path: resolver.DefaultBackupPath, Storage: multigresv1alpha1.StorageSpec{Size: resolver.DefaultBackupStorageSize}},
629+
},
630+
TopologyPruning: &multigresv1alpha1.TopologyPruningConfig{Enabled: ptr.To(true)},
621631
},
622632
}
623633

‎pkg/cluster-handler/controller/multigrescluster/integration_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,11 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
503503
WhenDeleted: multigresv1alpha1.RetainPVCRetentionPolicy,
504504
WhenScaled: multigresv1alpha1.RetainPVCRetentionPolicy,
505505
},
506+
Backup: &multigresv1alpha1.BackupConfig{
507+
Type: multigresv1alpha1.BackupTypeFilesystem,
508+
Filesystem: &multigresv1alpha1.FilesystemBackupConfig{Path: resolver.DefaultBackupPath, Storage: multigresv1alpha1.StorageSpec{Size: resolver.DefaultBackupStorageSize}},
509+
},
510+
TopologyPruning: &multigresv1alpha1.TopologyPruningConfig{Enabled: ptr.To(true)},
506511
},
507512
},
508513
},
@@ -798,6 +803,11 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
798803
WhenDeleted: multigresv1alpha1.RetainPVCRetentionPolicy,
799804
WhenScaled: multigresv1alpha1.RetainPVCRetentionPolicy,
800805
},
806+
Backup: &multigresv1alpha1.BackupConfig{
807+
Type: multigresv1alpha1.BackupTypeFilesystem,
808+
Filesystem: &multigresv1alpha1.FilesystemBackupConfig{Path: resolver.DefaultBackupPath, Storage: multigresv1alpha1.StorageSpec{Size: resolver.DefaultBackupStorageSize}},
809+
},
810+
TopologyPruning: &multigresv1alpha1.TopologyPruningConfig{Enabled: ptr.To(true)},
801811
},
802812
},
803813
},
@@ -1088,6 +1098,11 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
10881098
WhenDeleted: multigresv1alpha1.RetainPVCRetentionPolicy,
10891099
WhenScaled: multigresv1alpha1.RetainPVCRetentionPolicy,
10901100
},
1101+
Backup: &multigresv1alpha1.BackupConfig{
1102+
Type: multigresv1alpha1.BackupTypeFilesystem,
1103+
Filesystem: &multigresv1alpha1.FilesystemBackupConfig{Path: resolver.DefaultBackupPath, Storage: multigresv1alpha1.StorageSpec{Size: resolver.DefaultBackupStorageSize}},
1104+
},
1105+
TopologyPruning: &multigresv1alpha1.TopologyPruningConfig{Enabled: ptr.To(true)},
10911106
},
10921107
},
10931108
},

‎pkg/cluster-handler/controller/multigrescluster/status.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,30 @@ func (r *MultigresClusterReconciler) updateStatus(
117117
}
118118
}
119119

120+
topoServers := &multigresv1alpha1.TopoServerList{}
121+
if err := r.List(
122+
ctx,
123+
topoServers,
124+
client.InNamespace(cluster.Namespace),
125+
client.MatchingLabels{"multigres.com/cluster": cluster.Name},
126+
); err != nil {
127+
return fmt.Errorf("failed to list toposervers for status: %w", err)
128+
}
129+
130+
for _, ts := range topoServers.Items {
131+
switch {
132+
case ts.Status.ObservedGeneration != ts.Generation:
133+
allHealthy = false
134+
case ts.Status.Phase == multigresv1alpha1.PhaseDegraded:
135+
anyDegraded = true
136+
allHealthy = false
137+
case ts.Status.Phase == multigresv1alpha1.PhaseHealthy:
138+
// ok
139+
default:
140+
allHealthy = false
141+
}
142+
}
143+
120144
switch {
121145
case anyDegraded:
122146
cluster.Status.Phase = multigresv1alpha1.PhaseDegraded

‎pkg/resource-handler/controller/cell/cell_controller.go‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,17 @@ import (
2121

2222
multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
2323
"github.com/numtide/multigres-operator/pkg/monitoring"
24+
"github.com/numtide/multigres-operator/pkg/util/metadata"
2425
"github.com/numtide/multigres-operator/pkg/util/status"
2526
)
2627

28+
const (
29+
// statusRecheckDelay is the interval for periodic re-reconciliation when the
30+
// Cell is not Healthy. This allows detection of pod-level issues like
31+
// CrashLoopBackOff that don't trigger Deployment watch events.
32+
statusRecheckDelay = 30 * time.Second
33+
)
34+
2735
// CellReconciler reconciles a Cell object.
2836
type CellReconciler struct {
2937
client.Client
@@ -117,6 +125,10 @@ func (r *CellReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
117125

118126
logger.V(1).Info("reconcile complete", "duration", time.Since(start).String())
119127
r.Recorder.Event(cell, "Normal", "Synced", "Successfully reconciled Cell")
128+
129+
if cell.Status.Phase != multigresv1alpha1.PhaseHealthy {
130+
return ctrl.Result{RequeueAfter: statusRecheckDelay}, nil
131+
}
120132
return ctrl.Result{}, nil
121133
}
122134

@@ -217,21 +229,31 @@ func (r *CellReconciler) updateStatus(ctx context.Context, cell *multigresv1alph
217229
mgDeploy.Status.ReadyReplicas,
218230
)
219231

220-
// Update Phase
221-
cell.Status.Phase = status.ComputePhase(mgDeploy.Status.ReadyReplicas, mgDeploy.Status.Replicas)
222-
if mgDeploy.Status.ObservedGeneration != mgDeploy.Generation {
223-
cell.Status.Phase = multigresv1alpha1.PhaseProgressing
224-
cell.Status.Message = "Gateway Deployment is progressing"
225-
} else if cell.Status.Phase != multigresv1alpha1.PhaseHealthy {
226-
cell.Status.Message = fmt.Sprintf(
227-
"Gateway: %d/%d replicas ready",
228-
mgDeploy.Status.ReadyReplicas,
229-
mgDeploy.Status.Replicas,
230-
)
231-
} else {
232-
cell.Status.Message = "Ready"
232+
// List gateway pods for crash-loop detection
233+
mgLabels := metadata.BuildStandardLabels(
234+
cell.Labels[metadata.LabelMultigresCluster], MultiGatewayComponentName,
235+
)
236+
metadata.AddCellLabel(mgLabels, cell.Spec.Name)
237+
podList := &corev1.PodList{}
238+
if err := r.List(ctx, podList,
239+
client.InNamespace(cell.Namespace),
240+
client.MatchingLabels(metadata.GetSelectorLabels(mgLabels)),
241+
); err != nil {
242+
return fmt.Errorf("failed to list gateway pods for status: %w", err)
233243
}
234244

245+
// Update Phase
246+
result := status.ComputeWorkloadPhase(status.WorkloadPhaseInput{
247+
Ready: mgDeploy.Status.ReadyReplicas,
248+
Total: mgDeploy.Status.Replicas,
249+
GenerationCurrent: mgDeploy.Generation,
250+
GenerationObserved: mgDeploy.Status.ObservedGeneration,
251+
Pods: podList.Items,
252+
ComponentName: "Gateway",
253+
})
254+
cell.Status.Phase = result.Phase
255+
cell.Status.Message = result.Message
256+
235257
cell.Status.ObservedGeneration = cell.Generation
236258

237259
// 1. Construct the Patch Object

‎pkg/resource-handler/controller/cell/cell_controller_internal_test.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020

2121
multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
2222
"github.com/numtide/multigres-operator/pkg/testutil"
23+
"github.com/numtide/multigres-operator/pkg/util/metadata"
2324
)
2425

2526
// TestReconcileMultiGatewayDeployment_InvalidScheme tests the error path when BuildMultiGatewayDeployment fails.
@@ -356,3 +357,72 @@ func TestSetupWithManager(t *testing.T) {
356357
}
357358
})
358359
}
360+
361+
func TestUpdateStatus_DegradedOnCrashLoop(t *testing.T) {
362+
scheme := runtime.NewScheme()
363+
_ = multigresv1alpha1.AddToScheme(scheme)
364+
_ = appsv1.AddToScheme(scheme)
365+
_ = corev1.AddToScheme(scheme)
366+
367+
cell := &multigresv1alpha1.Cell{
368+
ObjectMeta: metav1.ObjectMeta{
369+
Name: "test-cell",
370+
Namespace: "default",
371+
Labels: map[string]string{metadata.LabelMultigresCluster: "test-cluster"},
372+
},
373+
Spec: multigresv1alpha1.CellSpec{
374+
Name: "zone1",
375+
},
376+
}
377+
378+
deployName := BuildMultiGatewayDeploymentName(cell)
379+
mgLabels := metadata.BuildStandardLabels("test-cluster", MultiGatewayComponentName)
380+
metadata.AddCellLabel(mgLabels, cell.Spec.Name)
381+
382+
deploy := &appsv1.Deployment{
383+
ObjectMeta: metav1.ObjectMeta{
384+
Name: deployName,
385+
Namespace: "default",
386+
Generation: 1,
387+
},
388+
Spec: appsv1.DeploymentSpec{Replicas: ptr.To(int32(1))},
389+
Status: appsv1.DeploymentStatus{Replicas: 1, ReadyReplicas: 0, ObservedGeneration: 1},
390+
}
391+
392+
crashPod := &corev1.Pod{
393+
ObjectMeta: metav1.ObjectMeta{
394+
Name: deployName + "-abc",
395+
Namespace: "default",
396+
Labels: metadata.GetSelectorLabels(mgLabels),
397+
},
398+
Status: corev1.PodStatus{
399+
ContainerStatuses: []corev1.ContainerStatus{
400+
{
401+
Name: "multigateway",
402+
State: corev1.ContainerState{
403+
Waiting: &corev1.ContainerStateWaiting{Reason: "CrashLoopBackOff"},
404+
},
405+
},
406+
},
407+
},
408+
}
409+
410+
fakeClient := fake.NewClientBuilder().
411+
WithScheme(scheme).
412+
WithObjects(cell, deploy, crashPod).
413+
WithStatusSubresource(&multigresv1alpha1.Cell{}).
414+
Build()
415+
416+
reconciler := &CellReconciler{
417+
Client: fakeClient,
418+
Scheme: scheme,
419+
Recorder: record.NewFakeRecorder(10),
420+
}
421+
422+
if err := reconciler.updateStatus(context.Background(), cell); err != nil {
423+
t.Fatalf("updateStatus() unexpected error: %v", err)
424+
}
425+
if cell.Status.Phase != multigresv1alpha1.PhaseDegraded {
426+
t.Errorf("expected PhaseDegraded, got %q", cell.Status.Phase)
427+
}
428+
}

‎pkg/resource-handler/controller/cell/cell_controller_test.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ func TestCellReconciler_Reconcile(t *testing.T) {
8888
},
8989
},
9090
existingObjects: []client.Object{},
91+
wantRequeue: true,
9192
assertFunc: func(t *testing.T, c client.Client, cell *multigresv1alpha1.Cell) {
9293
hashedName := buildHashedName(
9394
cell.Labels["multigres.com/cluster"],
@@ -306,6 +307,7 @@ func TestCellReconciler_Reconcile(t *testing.T) {
306307
},
307308
},
308309
},
310+
wantRequeue: true,
309311
existingObjects: []client.Object{
310312
&appsv1.Deployment{
311313
ObjectMeta: metav1.ObjectMeta{
@@ -544,8 +546,13 @@ func TestCellReconciler_Reconcile(t *testing.T) {
544546
return
545547
}
546548

547-
// NOTE: Check for requeue delay when we need to support such setup.
548-
_ = result
549+
if (result.RequeueAfter != 0) != tc.wantRequeue {
550+
t.Errorf(
551+
"Reconcile() requeue = %v, want requeue = %v",
552+
result.RequeueAfter,
553+
tc.wantRequeue,
554+
)
555+
}
549556

550557
// Run custom assertions if provided
551558
if tc.assertFunc != nil {

‎pkg/resource-handler/controller/shard/shard_controller.go‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,22 @@ func (r *ShardReconciler) Reconcile(
9191
return r.handlePendingDeletion(ctx, shard)
9292
}
9393

94+
// Update status early so observedGeneration and pod-health phase are
95+
// always current. Later steps (especially reconcileDataPlane) may block
96+
// on the topo connection for an extended period; placing updateStatus
97+
// here guarantees the status subresource is written every reconcile.
98+
{
99+
_, childSpan := monitoring.StartChildSpan(ctx, "Shard.UpdateStatus")
100+
if err := r.updateStatus(ctx, shard); err != nil {
101+
monitoring.RecordSpanError(childSpan, err)
102+
childSpan.End()
103+
logger.Error(err, "Failed to update status")
104+
r.Recorder.Eventf(shard, "Warning", "StatusError", "Failed to update status: %v", err)
105+
return ctrl.Result{}, err
106+
}
107+
childSpan.End()
108+
}
109+
94110
// Reconcile pg_hba ConfigMap first (required by all pools before starting)
95111
if err := r.reconcilePgHbaConfigMap(ctx, shard); err != nil {
96112
monitoring.RecordSpanError(span, err)
@@ -251,24 +267,16 @@ func (r *ShardReconciler) Reconcile(
251267
}
252268

253269
// Data-plane phases: open a single topo connection shared across all phases.
254-
result, err := r.reconcileDataPlane(ctx, shard)
270+
// Use a deadline so a hanging topo dial cannot block the reconcile
271+
// goroutine indefinitely, which would prevent future reconciles for
272+
// this shard and leave observedGeneration stale.
273+
dataPlaneCtx, dataPlaneCancel := context.WithTimeout(ctx, 30*time.Second)
274+
defer dataPlaneCancel()
275+
result, err := r.reconcileDataPlane(dataPlaneCtx, shard)
255276
if err != nil || result.RequeueAfter > 0 {
256277
return result, err
257278
}
258279

259-
// Update status
260-
{
261-
_, childSpan := monitoring.StartChildSpan(ctx, "Shard.UpdateStatus")
262-
if err := r.updateStatus(ctx, shard); err != nil {
263-
monitoring.RecordSpanError(childSpan, err)
264-
childSpan.End()
265-
logger.Error(err, "Failed to update status")
266-
r.Recorder.Eventf(shard, "Warning", "StatusError", "Failed to update status: %v", err)
267-
return ctrl.Result{}, err
268-
}
269-
childSpan.End()
270-
}
271-
272280
logger.V(1).Info("reconcile complete", "duration", time.Since(start).String())
273281
r.Recorder.Event(shard, "Normal", "Synced", "Successfully reconciled Shard")
274282
return ctrl.Result{}, nil

0 commit comments

Comments
 (0)