From ca2a6d64299896876604116316cbb48cd9407c4e Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Mon, 14 Sep 2026 17:55:48 +0400 Subject: [PATCH] fix(backups): warn when a Kafka backupTimeout is rejected kafkaRunDeadline silently substituted the 30m default for a backupTimeout that time.ParseDuration rejects or that falls below the one-minute floor, with no Event, condition or log line. An operator who set an unparseable or sub-minute value, then raised it, saw every run keep dying at DeadlineExceeded with nothing naming the discarded parameter. Return the applied deadline together with a message when the configured value is rejected, and record it as a BackupTimeoutInvalid Warning Event on the BackupJob and RestoreJob (the reconcilers already hold a Recorder). An honoured or unset value stays silent. TestKafkaRunDeadline now asserts the warning fires for the rejected cases and not otherwise. Signed-off-by: Andrey Kolkov Assisted-by: LLM --- .../kafkastrategy_controller.go | 34 ++++++++++++++----- .../kafkastrategy_controller_test.go | 21 +++++++----- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/internal/backupcontroller/kafkastrategy_controller.go b/internal/backupcontroller/kafkastrategy_controller.go index 128c8ecdf..9d52de9f3 100644 --- a/internal/backupcontroller/kafkastrategy_controller.go +++ b/internal/backupcontroller/kafkastrategy_controller.go @@ -147,14 +147,24 @@ func kafkaStrategyParameters(b *backupsv1alpha1.Backup) map[string]string { // kafkaRunDeadline resolves the wall-clock bound for a run from its parameters, // falling back to kafkaDefaultBackupDeadline when unset, unparseable, or below -// the floor. -func kafkaRunDeadline(parameters map[string]string) time.Duration { - if v := parameters[kafkaBackupTimeoutParam]; v != "" { - if d, err := time.ParseDuration(v); err == nil && d >= kafkaMinBackupDeadline { - return d - } +// the floor. When a backupTimeout is set but rejected, the second return value is +// a message naming the rejected value and the deadline actually applied, so the +// silent fallback becomes a Warning Event the caller records; it is empty when the +// value is honoured or unset. +func kafkaRunDeadline(parameters map[string]string) (time.Duration, string) { + v := parameters[kafkaBackupTimeoutParam] + if v == "" { + return kafkaDefaultBackupDeadline, "" + } + d, err := time.ParseDuration(v) + switch { + case err != nil: + return kafkaDefaultBackupDeadline, fmt.Sprintf("backupTimeout %q is not a valid Go duration; applying the default %s", v, kafkaDefaultBackupDeadline) + case d < kafkaMinBackupDeadline: + return kafkaDefaultBackupDeadline, fmt.Sprintf("backupTimeout %q is below the %s floor; applying the default %s", v, kafkaMinBackupDeadline, kafkaDefaultBackupDeadline) + default: + return d, "" } - return kafkaDefaultBackupDeadline } // kafkaRenderContext builds the template context. Unlike the Job strategy it @@ -263,7 +273,10 @@ func (r *BackupJobReconciler) reconcileKafka(ctx context.Context, j *backupsv1al return r.markBackupJobFailed(ctx, j, err.Error()) } - deadline := kafkaRunDeadline(resolved.Parameters) + deadline, deadlineWarn := kafkaRunDeadline(resolved.Parameters) + if deadlineWarn != "" && r.Recorder != nil { + r.Recorder.Event(j, corev1.EventTypeWarning, "BackupTimeoutInvalid", deadlineWarn) + } // First-reconcile bookkeeping (refetch guards against a stale informer // sliding StartedAt forward). Mirrors reconcileJob. @@ -545,7 +558,10 @@ func (r *RestoreJobReconciler) reconcileKafkaRestore(ctx context.Context, restor // The deadline round-trips from backup time via the Backup's parameters, so a // restore is bounded by the same value the operator set for the backup. - deadline := kafkaRunDeadline(kafkaStrategyParameters(backup)) + deadline, deadlineWarn := kafkaRunDeadline(kafkaStrategyParameters(backup)) + if deadlineWarn != "" && r.Recorder != nil { + r.Recorder.Event(restoreJob, corev1.EventTypeWarning, "BackupTimeoutInvalid", deadlineWarn) + } // A Backup with no recorded object can never restore; fail fast, before the // readiness gate, so it does not wait out the whole deadline and then report a diff --git a/internal/backupcontroller/kafkastrategy_controller_test.go b/internal/backupcontroller/kafkastrategy_controller_test.go index 8e7f4202a..7da37ba4b 100644 --- a/internal/backupcontroller/kafkastrategy_controller_test.go +++ b/internal/backupcontroller/kafkastrategy_controller_test.go @@ -551,20 +551,25 @@ func getKafkaJob(t *testing.T, c client.Client, namespace, name string) *batchv1 func TestKafkaRunDeadline(t *testing.T) { cases := []struct { - name string - params map[string]string - want time.Duration + name string + params map[string]string + want time.Duration + wantWarn bool // a rejected value must surface a non-empty warning }{ - {"unset falls back to default", nil, kafkaDefaultBackupDeadline}, - {"valid override honoured", map[string]string{kafkaBackupTimeoutParam: "2h"}, 2 * time.Hour}, - {"below floor falls back", map[string]string{kafkaBackupTimeoutParam: "10s"}, kafkaDefaultBackupDeadline}, - {"unparseable falls back", map[string]string{kafkaBackupTimeoutParam: "nope"}, kafkaDefaultBackupDeadline}, + {"unset falls back to default", nil, kafkaDefaultBackupDeadline, false}, + {"valid override honoured", map[string]string{kafkaBackupTimeoutParam: "2h"}, 2 * time.Hour, false}, + {"below floor falls back and warns", map[string]string{kafkaBackupTimeoutParam: "10s"}, kafkaDefaultBackupDeadline, true}, + {"unparseable falls back and warns", map[string]string{kafkaBackupTimeoutParam: "nope"}, kafkaDefaultBackupDeadline, true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := kafkaRunDeadline(tc.params); got != tc.want { + got, warn := kafkaRunDeadline(tc.params) + if got != tc.want { t.Fatalf("kafkaRunDeadline(%v) = %s, want %s", tc.params, got, tc.want) } + if (warn != "") != tc.wantWarn { + t.Fatalf("kafkaRunDeadline(%v) warn = %q, wantWarn %v", tc.params, warn, tc.wantWarn) + } }) } }