Skip to content

Commit e85501e

Browse files
mattia-eleuterimattia-eleuteri
committed
fix(backupstrategy-controller): bound each gate check and tolerate an absent Secret
Two defects found in review of the periodic recovery loop. checkAndLog passed the manager's root context straight into Check, which makes several sequential API calls. That context is only cancelled at shutdown, so an API server stalling on any one of them blocked the call indefinitely. The ticker loop is sequential, so one stuck call stopped every later check for the life of the pod: the recovery this gate exists to provide went silent, leaving a stale gauge and no further log line, with only a restart to bring it back. Each check now runs under a timeout of half the Period, capped at 30s, so it can never overlap the next tick. Check also propagated a NotFound on the projector's source Secret, although the very next branch treats an empty bucket name as an expected no-op. Those are the same bootstrap state seen a moment apart, so the absent Secret logged a check failure on every tick of the whole bootstrap window for a condition the doc comment frames as graceful. Both are pinned by tests that fail against the previous behaviour: one reverts to an error on the absent Secret, the other to a timeout equal to Period. Signed-off-by: Mattia Eleuteri <[email protected]>
1 parent 51ad226 commit e85501e

2 files changed

Lines changed: 86 additions & 4 deletions

File tree

‎internal/backupcontroller/default_objects_gate.go‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,32 @@ func (g *DefaultObjectsGate) Start(ctx context.Context) error {
199199
}
200200
}
201201

202+
// checkTimeout bounds a single check. It stays below Period so a slow check
203+
// cannot overlap the next tick, and is capped so a long Period does not mean a
204+
// correspondingly long stall.
205+
func (g *DefaultObjectsGate) checkTimeout() time.Duration {
206+
const maxTimeout = 30 * time.Second
207+
if g.Period <= 0 {
208+
return maxTimeout
209+
}
210+
if half := g.Period / 2; half < maxTimeout {
211+
return half
212+
}
213+
return maxTimeout
214+
}
215+
202216
func (g *DefaultObjectsGate) checkAndLog(ctx context.Context, logger logr.Logger) {
203-
missing, forced, err := g.Check(ctx)
217+
// Bound every tick. Check makes several sequential API calls (Secret get,
218+
// BackupClass get, one Get per routed strategy, HelmRelease patch) and the
219+
// manager context is only cancelled at shutdown, so an API server that
220+
// stalls on any one of them would block here indefinitely. The loop that
221+
// drives this is sequential, so that single stuck call would stop all
222+
// later checks for the life of the pod: the recovery this gate exists to
223+
// provide would go silent, with a stale gauge and no further log line, and
224+
// only a restart would bring it back.
225+
checkCtx, cancel := context.WithTimeout(ctx, g.checkTimeout())
226+
defer cancel()
227+
missing, forced, err := g.Check(checkCtx)
204228
switch {
205229
case err != nil:
206230
// Info, not Error: every branch here is either transient (the
@@ -225,12 +249,20 @@ func (g *DefaultObjectsGate) checkAndLog(ctx context.Context, logger logr.Logger
225249
// a forced upgrade was issued on this call.
226250
//
227251
// It is a no-op while the bucket name is unresolvable: forcing then would
228-
// re-render the same empty lookup. The bucket name is read from the
229-
// projector's source Secret rather than from the BucketClaim, so the gate
230-
// needs no objectstorage RBAC and works for the external-S3 path too.
252+
// re-render the same empty lookup. That covers both an absent source Secret
253+
// and one whose bucket name is still empty, which are the same bootstrap state
254+
// seen a moment apart. The bucket name is read from the projector's source
255+
// Secret rather than from the BucketClaim, so the gate needs no objectstorage
256+
// RBAC and works for the external-S3 path too.
231257
func (g *DefaultObjectsGate) Check(ctx context.Context) ([]string, bool, error) {
232258
src := &corev1.Secret{}
233259
if err := g.Client.Get(ctx, types.NamespacedName{Namespace: g.Config.SourceNamespace, Name: g.Config.SourceSecretName}, src); err != nil {
260+
if apierrors.IsNotFound(err) {
261+
// The projector has not created the Secret yet. Same no-op as an
262+
// unresolved bucket name: without it, every tick of the whole
263+
// bootstrap window logs a check failure for an expected state.
264+
return nil, false, nil
265+
}
234266
return nil, false, fmt.Errorf("get source credentials Secret %s/%s: %w", g.Config.SourceNamespace, g.Config.SourceSecretName, err)
235267
}
236268
creds, err := parseSourceSecret(src)

‎internal/backupcontroller/default_objects_gate_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,56 @@ func TestCheckSkipsWhileBucketUnresolved(t *testing.T) {
267267
}
268268
}
269269

270+
// TestCheckSkipsWhileSourceSecretAbsent pins the state one moment earlier than
271+
// TestCheckSkipsWhileBucketUnresolved: the projector has not created the Secret
272+
// at all yet. Returning an error here would log a check failure on every tick
273+
// of the entire bootstrap window, for a state the gate documents as a no-op.
274+
func TestCheckSkipsWhileSourceSecretAbsent(t *testing.T) {
275+
g, dyn := newGate(t,
276+
[]client.Object{cozyDefaultBackupClass()},
277+
helmReleaseObject(),
278+
)
279+
280+
missing, forced, err := g.Check(context.Background())
281+
if err != nil {
282+
t.Fatalf("Check returned an error for an absent source Secret, want a silent no-op: %v", err)
283+
}
284+
if len(missing) != 0 || forced {
285+
t.Fatalf("missing = %v, forced = %v, want none while the source Secret is absent", missing, forced)
286+
}
287+
if ann := forceAnnotations(t, dyn); len(ann) != 0 {
288+
t.Errorf("HelmRelease forced before the source Secret existed: %v", ann)
289+
}
290+
}
291+
292+
// TestCheckTimeoutStaysBelowPeriod pins the bound on a single check. The ticker
293+
// loop is sequential and the manager context is only cancelled at shutdown, so
294+
// an unbounded check that hangs on a stalled API call stops every later check
295+
// for the life of the pod.
296+
func TestCheckTimeoutStaysBelowPeriod(t *testing.T) {
297+
for _, tc := range []struct {
298+
name string
299+
period time.Duration
300+
want time.Duration
301+
}{
302+
{"unset falls back to the cap", 0, 30 * time.Second},
303+
{"default period halves", time.Minute, 30 * time.Second},
304+
{"short period halves", 10 * time.Second, 5 * time.Second},
305+
{"long period is capped", time.Hour, 30 * time.Second},
306+
} {
307+
t.Run(tc.name, func(t *testing.T) {
308+
g := &DefaultObjectsGate{Period: tc.period}
309+
got := g.checkTimeout()
310+
if got != tc.want {
311+
t.Fatalf("checkTimeout() = %v, want %v", got, tc.want)
312+
}
313+
if tc.period > 0 && got >= tc.period {
314+
t.Fatalf("checkTimeout() = %v, must stay below Period %v", got, tc.period)
315+
}
316+
})
317+
}
318+
}
319+
270320
// TestCheckThrottlesRepeatedForces pins the throttle. A condition a
271321
// re-render cannot fix (missing CRD, unreachable bucket) must not turn into
272322
// a hot loop of Helm upgrades against helm-controller.

0 commit comments

Comments
 (0)