Skip to content

Fixes schedule update reset active #4209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 13, 2025
Merged

Fixes schedule update reset active #4209

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdebb58
Reset active status when setting a new value for cluster scheduled
0xavi0 Oct 8, 2025
b96fe35
Covers issue in unit tests
0xavi0 Oct 9, 2025
237c907
Code review 1 changes
0xavi0 Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions internal/cmd/controller/reconciler/schedule_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// when this function is called is either because we're updating a
// This function is called either because we're updating a

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed as suggested

// 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)
}
Expand Down Expand Up @@ -413,10 +416,8 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still up-to-date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've deleted the comment because now the code is obvious.

for _, cluster := range clustersNew {
if !slices.Contains(clustersOld, cluster) {
if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil {
return err
}
if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil {
return err
}
}

Expand Down
43 changes: 32 additions & 11 deletions internal/cmd/controller/reconciler/schedule_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
}

Expand Down Expand Up @@ -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{}
Expand All @@ -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)
})
})
})
Expand All @@ -339,7 +359,7 @@ func checkClusterIsScheduled(
scheduler quartz.Scheduler,
k8sclient client.Client,
cluster, namespace string,
scheduledExpected, flaggedAsScheduledExpected bool) {
scheduledExpected, flaggedAsScheduledExpected, activeScheduleExpected bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think creating a struct containing these 3 booleans would make the calls above a bit easier to read.
Example:

type expected struct{
    job bool
    statusScheduled bool
    statusActiveSchedule bool
}

... and, since these fields would be visible on the calling line, this function could be renamed to e.g. checkState

// before
checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", true, true, false)
// after
checkState(scheduler, k8sclient, "test-cluster4", "default", expected{job:true, statusScheduled: true, statusActiveSchedule: true})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed as suggested

isScheduled, err := isClusterScheduled(scheduler, cluster, namespace)
Expect(err).NotTo(HaveOccurred())
Expect(isScheduled).To(Equal(scheduledExpected))
Expand All @@ -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))
}