From bdebb58d0367dfaee14d9f5373847eebb6931a8f Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Wed, 8 Oct 2025 18:06:16 +0200 Subject: [PATCH 1/3] Reset active status when setting a new value for cluster scheduled Signed-off-by: Xavi Garcia --- .../reconciler/schedule_controller.go | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/internal/cmd/controller/reconciler/schedule_controller.go b/internal/cmd/controller/reconciler/schedule_controller.go index add0d34687..ac88459139 100644 --- a/internal/cmd/controller/reconciler/schedule_controller.go +++ b/internal/cmd/controller/reconciler/schedule_controller.go @@ -265,16 +265,19 @@ func setClusterScheduled(ctx context.Context, c client.Client, name, namespace s return fmt.Errorf("%w, getting cluster: %w", fleetutil.ErrRetryable, err) } - // if the value is already the expected one, avoid the update - if cluster.Status.Scheduled == scheduled { + // if the values are already the expected ones, avoid the update + if cluster.Status.Scheduled == scheduled && !cluster.Status.ActiveSchedule { return nil } old := cluster.DeepCopy() cluster.Status.Scheduled = scheduled - if !scheduled { - cluster.Status.ActiveSchedule = false - } + + // when this function is called is either because we're updating a + // Schedule or because we're creating it. + // In both cases ActiveSchedule should be false as a Schedule + // always begins in OffSchedule mode until the first start call is executed. + cluster.Status.ActiveSchedule = false return updateClusterStatus(ctx, c, old, cluster) } @@ -413,10 +416,11 @@ func setClustersScheduled(ctx context.Context, c client.Client, clusters []strin func updateScheduledClusters(ctx context.Context, scheduler quartz.Scheduler, c client.Client, clustersNew []string, clustersOld []string, namespace string) error { // first look for clusters that are not scheduled yet and flag them as scheduled for _, cluster := range clustersNew { - if !slices.Contains(clustersOld, cluster) { - if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil { - return err - } + // set schedule to true and also reset the activeSchedule property + // When we update a Schedule it begins from a offSchedule status until + // executeStart is called. + if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil { + return err } } From b96fe350165f8b54799cbae15c3dd322ead56dc0 Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Thu, 9 Oct 2025 11:04:16 +0200 Subject: [PATCH 2/3] Covers issue in unit tests Signed-off-by: Xavi Garcia --- .../reconciler/schedule_controller.go | 3 -- .../reconciler/schedule_controller_test.go | 43 ++++++++++++++----- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/internal/cmd/controller/reconciler/schedule_controller.go b/internal/cmd/controller/reconciler/schedule_controller.go index ac88459139..805622c7fd 100644 --- a/internal/cmd/controller/reconciler/schedule_controller.go +++ b/internal/cmd/controller/reconciler/schedule_controller.go @@ -416,9 +416,6 @@ func setClustersScheduled(ctx context.Context, c client.Client, clusters []strin func updateScheduledClusters(ctx context.Context, scheduler quartz.Scheduler, c client.Client, clustersNew []string, clustersOld []string, namespace string) error { // first look for clusters that are not scheduled yet and flag them as scheduled for _, cluster := range clustersNew { - // set schedule to true and also reset the activeSchedule property - // When we update a Schedule it begins from a offSchedule status until - // executeStart is called. if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil { return err } diff --git a/internal/cmd/controller/reconciler/schedule_controller_test.go b/internal/cmd/controller/reconciler/schedule_controller_test.go index 47b704abd0..1e54f50f00 100644 --- a/internal/cmd/controller/reconciler/schedule_controller_test.go +++ b/internal/cmd/controller/reconciler/schedule_controller_test.go @@ -271,7 +271,7 @@ var _ = Describe("ScheduleReconciler", func() { ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster2", Namespace: "default", - Labels: map[string]string{"env": "test"}, + Labels: map[string]string{"env": "test", "foo": "bar"}, }, } @@ -305,10 +305,28 @@ var _ = Describe("ScheduleReconciler", func() { // clusters 1, 2 and 3 should be scheduled in the quartz.Scheduler and also // be flagged as Scheduled - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true, false) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, false) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true, false) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false, false) + + // force the start of the schedule (so it sets .Status.ActiveSchedule=true) + jobKey := scheduleKey(schedule) + job, err := scheduler.GetScheduledJob(jobKey) + Expect(err).NotTo(HaveOccurred()) + + cronDurationJob, ok := job.JobDetail().Job().(*CronDurationJob) + Expect(ok).To(BeTrue()) + + // Manually trigger start + err = cronDurationJob.executeStart(ctx) + Expect(err).NotTo(HaveOccurred()) + + // check now that the clusters have the expected values, specially Status.ActiveSchedule + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true, true) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, true) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true, true) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false, false) // update the schedule, now it only looks for the label foo=bar scheduleUpdated := &fleet.Schedule{} @@ -326,11 +344,13 @@ var _ = Describe("ScheduleReconciler", func() { _, err = reconciler.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) - // now only 1 cluster should be flagged as scheduled and be in the scheduled jobs list - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", false, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", false, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", false, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", true, true) + // cluster 2 and 4 should be still targeted. + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", false, false, false) + // cluster 2 had Status.ActiveSchedule set to true, but because we updated the Schedule + // it should be back to false. + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, false) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", false, false, false) + checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", true, true, false) }) }) }) @@ -339,7 +359,7 @@ func checkClusterIsScheduled( scheduler quartz.Scheduler, k8sclient client.Client, cluster, namespace string, - scheduledExpected, flaggedAsScheduledExpected bool) { + scheduledExpected, flaggedAsScheduledExpected, activeScheduleExpected bool) { isScheduled, err := isClusterScheduled(scheduler, cluster, namespace) Expect(err).NotTo(HaveOccurred()) Expect(isScheduled).To(Equal(scheduledExpected)) @@ -349,4 +369,5 @@ func checkClusterIsScheduled( err = k8sclient.Get(context.Background(), key, clusterObj) Expect(err).NotTo(HaveOccurred()) Expect(clusterObj.Status.Scheduled).To(Equal(flaggedAsScheduledExpected)) + Expect(clusterObj.Status.ActiveSchedule).To(Equal(activeScheduleExpected)) } From 237c9079f651ffee8d89d34727533b5abbcf8748 Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Mon, 13 Oct 2025 14:43:42 +0200 Subject: [PATCH 3/3] Code review 1 changes Signed-off-by: Xavi Garcia --- .../reconciler/schedule_controller.go | 3 +- .../reconciler/schedule_controller_test.go | 52 +++++++++++++------ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/internal/cmd/controller/reconciler/schedule_controller.go b/internal/cmd/controller/reconciler/schedule_controller.go index 805622c7fd..84d707a6f8 100644 --- a/internal/cmd/controller/reconciler/schedule_controller.go +++ b/internal/cmd/controller/reconciler/schedule_controller.go @@ -273,7 +273,7 @@ func setClusterScheduled(ctx context.Context, c client.Client, name, namespace s old := cluster.DeepCopy() cluster.Status.Scheduled = scheduled - // when this function is called is either because we're updating a + // This function is called either because we're updating a // Schedule or because we're creating it. // In both cases ActiveSchedule should be false as a Schedule // always begins in OffSchedule mode until the first start call is executed. @@ -414,7 +414,6 @@ func setClustersScheduled(ctx context.Context, c client.Client, clusters []strin } func updateScheduledClusters(ctx context.Context, scheduler quartz.Scheduler, c client.Client, clustersNew []string, clustersOld []string, namespace string) error { - // first look for clusters that are not scheduled yet and flag them as scheduled for _, cluster := range clustersNew { if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil { return err diff --git a/internal/cmd/controller/reconciler/schedule_controller_test.go b/internal/cmd/controller/reconciler/schedule_controller_test.go index 1e54f50f00..06e72ce0a6 100644 --- a/internal/cmd/controller/reconciler/schedule_controller_test.go +++ b/internal/cmd/controller/reconciler/schedule_controller_test.go @@ -19,6 +19,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +type expected struct { + scheduledJob bool + statusScheduled bool + statusActiveSchedule bool +} + var _ = Describe("ScheduleReconciler", func() { var ( ctx context.Context @@ -305,10 +311,14 @@ var _ = Describe("ScheduleReconciler", func() { // clusters 1, 2 and 3 should be scheduled in the quartz.Scheduler and also // be flagged as Scheduled - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false, false) + checkState(scheduler, k8sclient, "test-cluster", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false}) + checkState(scheduler, k8sclient, "test-cluster2", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false}) + checkState(scheduler, k8sclient, "test-cluster3", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false}) + checkState(scheduler, k8sclient, "test-cluster4", "default", + expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false}) // force the start of the schedule (so it sets .Status.ActiveSchedule=true) jobKey := scheduleKey(schedule) @@ -323,10 +333,14 @@ var _ = Describe("ScheduleReconciler", func() { Expect(err).NotTo(HaveOccurred()) // check now that the clusters have the expected values, specially Status.ActiveSchedule - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true, true) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, true) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true, true) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false, false) + checkState(scheduler, k8sclient, "test-cluster", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: true}) + checkState(scheduler, k8sclient, "test-cluster2", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: true}) + checkState(scheduler, k8sclient, "test-cluster3", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: true}) + checkState(scheduler, k8sclient, "test-cluster4", "default", + expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false}) // update the schedule, now it only looks for the label foo=bar scheduleUpdated := &fleet.Schedule{} @@ -345,29 +359,33 @@ var _ = Describe("ScheduleReconciler", func() { Expect(err).NotTo(HaveOccurred()) // cluster 2 and 4 should be still targeted. - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", false, false, false) + checkState(scheduler, k8sclient, "test-cluster", "default", + expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false}) // cluster 2 had Status.ActiveSchedule set to true, but because we updated the Schedule // it should be back to false. - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", false, false, false) - checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", true, true, false) + checkState(scheduler, k8sclient, "test-cluster2", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false}) + checkState(scheduler, k8sclient, "test-cluster3", "default", + expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false}) + checkState(scheduler, k8sclient, "test-cluster4", "default", + expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false}) }) }) }) -func checkClusterIsScheduled( +func checkState( scheduler quartz.Scheduler, k8sclient client.Client, cluster, namespace string, - scheduledExpected, flaggedAsScheduledExpected, activeScheduleExpected bool) { + expectedState expected) { isScheduled, err := isClusterScheduled(scheduler, cluster, namespace) Expect(err).NotTo(HaveOccurred()) - Expect(isScheduled).To(Equal(scheduledExpected)) + Expect(isScheduled).To(Equal(expectedState.scheduledJob)) key := client.ObjectKey{Name: cluster, Namespace: namespace} clusterObj := &fleet.Cluster{} err = k8sclient.Get(context.Background(), key, clusterObj) Expect(err).NotTo(HaveOccurred()) - Expect(clusterObj.Status.Scheduled).To(Equal(flaggedAsScheduledExpected)) - Expect(clusterObj.Status.ActiveSchedule).To(Equal(activeScheduleExpected)) + Expect(clusterObj.Status.Scheduled).To(Equal(expectedState.statusScheduled)) + Expect(clusterObj.Status.ActiveSchedule).To(Equal(expectedState.statusActiveSchedule)) }