diff --git a/changelogs/unreleased/10490-lhjnano b/changelogs/unreleased/10490-lhjnano new file mode 100644 index 000000000..4b01b76e6 --- /dev/null +++ b/changelogs/unreleased/10490-lhjnano @@ -0,0 +1 @@ +Fix redundant schedule patching by comparing SkipImmediately and LastSkipped by value diff --git a/pkg/controller/schedule_controller.go b/pkg/controller/schedule_controller.go index 2e707bdc0..9500123c0 100644 --- a/pkg/controller/schedule_controller.go +++ b/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)) } diff --git a/pkg/controller/schedule_patch_threshold_test.go b/pkg/controller/schedule_patch_threshold_test.go new file mode 100644 index 000000000..39117b59e --- /dev/null +++ b/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") + }) +}