Fix schedule reconciler to compare SkipImmediately and LastSkipped by value (#10490)

* Fix schedule reconciler to compare SkipImmediately and LastSkipped by value

The reconciler compared spec.skipImmediately (*bool) and
status.lastSkipped (*metav1.Time) between the live object and its
DeepCopy by pointer, so both checks always reported a change once the
fields were set, and every reconcile issued a redundant Patch. Compare
by value instead (ptr.Equal / equality.Semantic.DeepEqual) and add a
regression test pinning that the skip-once flip persists with exactly
one patch, later reconciles of persisted state patch zero times, and
an explicit false is a no-op.

Signed-off-by: HeonJe LEE <lhjnano@gmail.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com>

---------

Signed-off-by: HeonJe LEE <lhjnano@gmail.com>
Signed-off-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com>
Co-authored-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

changelogs/unreleased/10490-lhjnano

@@ -0,0 +1 @@
+Fix redundant schedule patching by comparing SkipImmediately and LastSkipped by value

pkg/controller/schedule_controller.go

@@ -24,10 +24,12 @@ import (
 	"github.com/cockroachdb/errors"
 	cron "github.com/netresearch/go-cron"
 	"github.com/sirupsen/logrus"
+	equality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	clocks "k8s.io/utils/clock"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	bld "sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -149,12 +151,12 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		errStringArr = append(errStringArr, fmt.Sprintf("phase to %s", schedule.Status.Phase))
 	}
 	// update spec.SkipImmediately if it's changed
-	if original.Spec.SkipImmediately != schedule.Spec.SkipImmediately {
+	if !ptr.Equal(original.Spec.SkipImmediately, schedule.Spec.SkipImmediately) {
 		scheduleNeedsPatch = true
 		errStringArr = append(errStringArr, fmt.Sprintf("spec.skipImmediately to %v", schedule.Spec.SkipImmediately))
 	}
 	// update status if it's changed
-	if original.Status.LastSkipped != schedule.Status.LastSkipped {
+	if !equality.Semantic.DeepEqual(original.Status.LastSkipped, schedule.Status.LastSkipped) {
 		scheduleNeedsPatch = true
 		errStringArr = append(errStringArr, fmt.Sprintf("last skipped to %v", schedule.Status.LastSkipped))
 	}

pkg/controller/schedule_patch_threshold_test.go

@@ -0,0 +1,171 @@
+/*
+Copyright The Velero Contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/utils/ptr"
+	ctrl "sigs.k8s.io/controller-runtime"
+	crclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/builder"
+	"github.com/vmware-tanzu/velero/pkg/metrics"
+	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+)
+
+// patchCountingClient wraps a client.Client and counts Patch calls so tests can
+// observe how many patches each Reconcile actually issues.
+type patchCountingClient struct {
+	crclient.Client
+	patches int
+}
+
+func (p *patchCountingClient) Patch(ctx context.Context, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error {
+	p.patches++
+	return p.Client.Patch(ctx, obj, patch, opts...)
+}
+
+// TestSchedulePatchThreshold pins the Patch frequency of the schedule
+// reconciler around spec.SkipImmediately handling.
+//
+// Known defect under test (schedule_controller.go, "update spec.SkipImmediately
+// if it's changed"): the comparison is a *bool POINTER comparison between the
+// fetched schedule and its DeepCopy, so it is always unequal. Every Reconcile
+// of an unchanged schedule therefore issues one Patch.
+func TestSchedulePatchThreshold(t *testing.T) {
+	require.NoError(t, velerov1api.AddToScheme(scheme.Scheme))
+
+	ctx := context.Background()
+	const (
+		testCron     = "0 0 1 1 *"
+		testLastBk   = "9999-09-06 00:00:00"
+		testSchedule = "name" // newScheduleBuilder hardcodes ns/name (see schedule_controller_test.go)
+	)
+
+	// Same helper as schedule_controller_test.go's TestReconcileOfSchedule —
+	// that one is closure-local, so this file needs its own identical copy.
+	newScheduleBuilder := func(phase velerov1api.SchedulePhase) *builder.ScheduleBuilder {
+		return builder.ForSchedule("ns", testSchedule).Phase(phase)
+	}
+
+	newReconciler := func(c crclient.Client, skip bool) *scheduleReconciler {
+		return NewScheduleReconciler("namespace", velerotest.NewLogger(), c, metrics.NewServerMetrics(), skip)
+	}
+
+	t.Run("skip-once flip persists on first reconcile only", func(t *testing.T) {
+		base := (&fake.ClientBuilder{}).Build()
+		counting := &patchCountingClient{Client: base}
+
+		schedule := newScheduleBuilder(velerov1api.SchedulePhaseEnabled).
+			CronSchedule(testCron).
+			LastBackupTime(testLastBk).
+			SkipImmediately(ptr.To(true)).
+			Result()
+		require.NoError(t, counting.Create(ctx, schedule))
+
+		reconciler := newReconciler(counting, true)
+
+		req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: testSchedule}}
+		_, err := reconciler.Reconcile(ctx, req)
+		require.NoError(t, err)
+		firstRound := counting.patches
+
+		_, err = reconciler.Reconcile(ctx, req)
+		require.NoError(t, err)
+		secondRound := counting.patches - firstRound
+
+		t.Logf("patches in reconcile #1: %d", firstRound)
+		t.Logf("additional patches in reconcile #2: %d", secondRound)
+		// Reconcile #1 legitimately persists the skip-once flip (spec.SkipImmediately
+		// true -> false plus status.LastSkipped). Everything else (phase, cron,
+		// LastBackup) is stable, so it must patch exactly once.
+		assert.Equal(t, 1, firstRound, "reconcile #1 should persist the skip-once flip with exactly one patch")
+		// Reconcile #2 runs against the already-persisted state: comparing the
+		// *bool SkipImmediately and the *metav1.Time LastSkipped by pointer would
+		// always report a change after DeepCopy and force a patch every cycle.
+		assert.Equal(t, 0, secondRound, "reconcile #2 of a fully-persisted schedule should issue no patch")
+
+		stored := &velerov1api.Schedule{}
+		require.NoError(t, counting.Get(ctx, req.NamespacedName, stored))
+		require.NotNil(t, stored.Spec.SkipImmediately)
+		assert.False(t, *stored.Spec.SkipImmediately, "skip-once flag should stay false after the flip is persisted")
+	})
+
+	t.Run("explicit false is a no-op", func(t *testing.T) {
+		base := (&fake.ClientBuilder{}).Build()
+		counting := &patchCountingClient{Client: base}
+
+		schedule := newScheduleBuilder(velerov1api.SchedulePhaseEnabled).
+			CronSchedule(testCron).
+			LastBackupTime(testLastBk).
+			SkipImmediately(ptr.To(false)).
+			Result()
+		require.NoError(t, counting.Create(ctx, schedule))
+
+		reconciler := newReconciler(counting, true)
+
+		req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: testSchedule}}
+		_, err := reconciler.Reconcile(ctx, req)
+		require.NoError(t, err)
+
+		// The reconciler only nil-fills and flips true->false; an explicit false
+		// with a stable phase/LastSkipped changes nothing, so no patch is needed.
+		// (A pointer comparison would patch here on every reconcile.)
+		assert.Equal(t, 0, counting.patches, "an explicit spec.skipImmediately=false should not be patched when nothing changes")
+
+		stored := &velerov1api.Schedule{}
+		require.NoError(t, counting.Get(ctx, req.NamespacedName, stored))
+		require.NotNil(t, stored.Spec.SkipImmediately)
+		assert.False(t, *stored.Spec.SkipImmediately, "SkipImmediately=false survives reconcile unchanged")
+	})
+
+	t.Run("nil to set transitions once", func(t *testing.T) {
+		base := (&fake.ClientBuilder{}).Build()
+		counting := &patchCountingClient{Client: base}
+
+		// SkipImmediately left nil (builder default).
+		schedule := newScheduleBuilder(velerov1api.SchedulePhaseEnabled).
+			CronSchedule(testCron).
+			LastBackupTime(testLastBk).
+			Result()
+		require.NoError(t, counting.Create(ctx, schedule))
+
+		reconciler := newReconciler(counting, true)
+
+		req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: testSchedule}}
+		_, err := reconciler.Reconcile(ctx, req)
+		require.NoError(t, err)
+
+		assert.Equal(t, 1, counting.patches, "the nil->set transition should be covered by exactly one patch")
+
+		stored := &velerov1api.Schedule{}
+		require.NoError(t, counting.Get(ctx, req.NamespacedName, stored))
+		// Current implementation contract: nil is filled from the reconciler
+		// default (true) and then immediately flipped to false by the
+		// skip-immediately block, so the persisted value is false, not true.
+		require.NotNil(t, stored.Spec.SkipImmediately)
+		assert.False(t, *stored.Spec.SkipImmediately, "nil SkipImmediately is filled then flipped to false in one reconcile")
+	})
+}