From 194404971a5a8e16e12d8d4e6cef8fce2e3a702b Mon Sep 17 00:00:00 2001 From: V Prajwal Date: Fri, 14 Aug 2026 13:48:00 +0530 Subject: [PATCH] Fix schedule reconciler aliasing server-wide skipImmediately default (#10242) When a Schedule has no explicit spec.skipImmediately, the reconciler assigned &c.skipImmediately directly into the Schedule's spec pointer. The subsequent write-through-pointer (*ptr = false) mutated the reconciler's own shared field, silently disabling --schedule-skip-immediately for every schedule reconciled afterward for the life of the process. Fix: copy the value into a fresh bool before taking its address. Adds TestReconcileDoesNotCorruptReconcilerSkipImmediately, which reconciles two schedules against one reconciler instance and asserts the shared default is preserved. Signed-off-by: Prajwal --- changelogs/unreleased/10242-beep-boopp | 1 + pkg/controller/schedule_controller.go | 6 ++- pkg/controller/schedule_controller_test.go | 48 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/10242-beep-boopp diff --git a/changelogs/unreleased/10242-beep-boopp b/changelogs/unreleased/10242-beep-boopp new file mode 100644 index 000000000..42312c448 --- /dev/null +++ b/changelogs/unreleased/10242-beep-boopp @@ -0,0 +1 @@ +Fix schedule reconciler aliasing &c.skipImmediately into Schedule specs, corrupting the server-wide --schedule-skip-immediately default after the first reconcile diff --git a/pkg/controller/schedule_controller.go b/pkg/controller/schedule_controller.go index d71c86ca4..2e707bdc0 100644 --- a/pkg/controller/schedule_controller.go +++ b/pkg/controller/schedule_controller.go @@ -111,7 +111,11 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c original := schedule.DeepCopy() if schedule.Spec.SkipImmediately == nil { - schedule.Spec.SkipImmediately = &c.skipImmediately + // Copy the value rather than aliasing &c.skipImmediately: c is a long-lived + // singleton reconciler, and the block below can write through this pointer, + // which would otherwise mutate the reconciler's shared default field. + skipImmediately := c.skipImmediately + schedule.Spec.SkipImmediately = &skipImmediately } if schedule.Spec.SkipImmediately != nil && *schedule.Spec.SkipImmediately { *schedule.Spec.SkipImmediately = false diff --git a/pkg/controller/schedule_controller_test.go b/pkg/controller/schedule_controller_test.go index 85b87474a..1c134d2bc 100644 --- a/pkg/controller/schedule_controller_test.go +++ b/pkg/controller/schedule_controller_test.go @@ -246,6 +246,54 @@ func parseTime(timeString string) time.Time { return res } +// TestReconcileDoesNotCorruptReconcilerSkipImmediately guards against a regression where +// aliasing &c.skipImmediately into a Schedule's spec (when SkipImmediately is nil) let a +// subsequent write-through-pointer mutate the reconciler's own shared default field, +// silently corrupting it for every later reconcile in the process. +func TestReconcileDoesNotCorruptReconcilerSkipImmediately(t *testing.T) { + require.NoError(t, velerov1.AddToScheme(scheme.Scheme)) + + client := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() + logger := velerotest.NewLogger() + + // Server configured with schedule-skip-immediately=true. + reconciler := NewScheduleReconciler("ns", logger, client, metrics.NewServerMetrics(), true) + reconciler.clock = testclocks.NewFakeClock(time.Now()) + + makeSchedule := func(name string) *velerov1.Schedule { + return builder.ForSchedule("ns", name). + Phase(velerov1.SchedulePhaseEnabled). + CronSchedule("@every 5m"). + LastBackupTime("2000-01-01 00:00:00"). // long past due, but should be skipped + Result() // SkipImmediately left nil + } + + sched1 := makeSchedule("sched-1") + require.NoError(t, client.Create(ctx, sched1)) + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: "sched-1"}}) + require.NoError(t, err) + + // The reconciler's own default must be unchanged after processing a schedule with a nil + // SkipImmediately -- every later schedule relies on this field still being true. + assert.True(t, reconciler.skipImmediately, "reconciler's shared skipImmediately default was mutated by reconciling sched-1") + + sched2 := makeSchedule("sched-2") + require.NoError(t, client.Create(ctx, sched2)) + _, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: "sched-2"}}) + require.NoError(t, err) + + assert.True(t, reconciler.skipImmediately, "reconciler's shared skipImmediately default was mutated by reconciling sched-2") + + // Functional check: sched-2 should ALSO have been skipped (server default still true), + // proving the bug's user-visible symptom (second+ schedule silently loses the skip + // behavior) is fixed, not just the internal field. + got := &velerov1.Schedule{} + require.NoError(t, client.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "sched-2"}, got)) + require.NotNil(t, got.Status.LastSkipped, "sched-2 should have been skipped due to server-wide skipImmediately default") + require.NotNil(t, got.Status.LastBackup) + assert.Equal(t, parseTime("2000-01-01 00:00:00").Unix(), got.Status.LastBackup.Unix(), "sched-2 should not have triggered a new backup") +} + func TestGetNextRunTime(t *testing.T) { defaultSchedule := func() *velerov1.Schedule { return builder.ForSchedule("velero", "schedule-1").CronSchedule("@every 5m").Result()