Second code review changes

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

Author: 0xavi0

charts/fleet/ci/debug-values.yaml

@@ -51,6 +51,7 @@ controller:
       gitrepo: "1"
       bundle: "1"
       bundledeployment: "1"
+      schedule: "1"
 
 shards:
   - id: shard0

charts/fleet/ci/nobootstrap-values.yaml

@@ -50,6 +50,7 @@ controller:
       gitrepo: "1"
       bundle: "1"
       bundledeployment: "1"
+      schedule: "1"
 
 shards:
   - id: shard0

charts/fleet/ci/nodebug-values.yaml

@@ -50,6 +50,7 @@ controller:
       gitrepo: "1"
       bundle: "1"
       bundledeployment: "1"
+      schedule: "1"
 
 shards:
   - id: shard0

charts/fleet/ci/nogitops-values.yaml

@@ -50,6 +50,7 @@ controller:
       gitrepo: "1"
       bundle: "1"
       bundledeployment: "1"
+      schedule: "1"
 
 shards:
   - id: shard0

charts/fleet/ci/nohelmops-values.yaml

@@ -50,6 +50,7 @@ controller:
       gitrepo: "1"
       bundle: "1"
       bundledeployment: "1"
+      schedule: "1"
 
 shards:
   - id: shard0

charts/fleet/templates/deployment.yaml

@@ -89,6 +89,10 @@ spec:
         - name: IMAGESCAN_RECONCILER_WORKERS
           value: {{ quote $.Values.controller.reconciler.workers.imagescan }}
         {{- end }}
+        {{- if $.Values.controller.reconciler.workers.schedule }}
+        - name: SCHEDULE_RECONCILER_WORKERS
+          value: {{ quote $.Values.controller.reconciler.workers.schedule }}
+        {{- end }}
 {{- if $.Values.extraEnv }}
 {{ toYaml $.Values.extraEnv | indent 8}}
 {{- end }}

charts/fleet/values.yaml

@@ -119,6 +119,7 @@ controller:
       cluster: "50"
       clustergroup: "50"
       imagescan: "50"
+      schedule: "50"
 
 gitjob:
   replicas: 1

internal/cmd/controller/reconciler/cronduration_job.go

@@ -214,13 +214,6 @@ func (c *CronDurationJob) executeStart(ctx context.Context) error {
 		return err
 	}
 
-	// Sets ActiveSchedule to true for all matching clusters.
-	for _, cluster := range clusters {
-		if err := setClusterActiveSchedule(context.Background(), c.client, cluster, c.Schedule.Namespace, true); err != nil {
-			return err
-		}
-	}
-
 	// Sets Scheduled to false for all clusters that previously matched but no longer do.
 	for _, cluster := range c.MatchingClusters {
 		if !slices.Contains(clusters, cluster) {
@@ -231,9 +224,11 @@ func (c *CronDurationJob) executeStart(ctx context.Context) error {
 		}
 	}
 
-	// Sets Scheduled to true to all the matching clusters
-	if err := setClustersScheduled(ctx, c.client, clusters, c.Schedule.Namespace, true); err != nil {
-		return err
+	// Sets ActiveSchedule to true for all matching clusters.
+	for _, cluster := range clusters {
+		if err := setClusterActiveSchedule(ctx, c.client, cluster, c.Schedule.Namespace, true); err != nil {
+			return err
+		}
 	}
 	c.MatchingClusters = clusters
 

internal/cmd/controller/reconciler/cronduration_job_test.go

@@ -2,6 +2,7 @@ package reconciler
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -298,14 +299,13 @@ var _ = Describe("CronDurationJob", func() {
 
 	Context("scheduleJob", func() {
 		It("should schedule a job and update status", func() {
-			// wait a bit if we are too close to second 59...
-			// this is to ensure calculations based on minutes and seconds
-			waitIfTimeIsTooCloseTo59()
-
+			// set the schedule time to now plus 10 seconds so we can check
+			// later knowning the expected value.
+			scheduleTime := time.Now().Add(10 * time.Second)
+			schedule.Spec.Schedule = fmt.Sprintf("%d %d %d * * *", scheduleTime.Second(), scheduleTime.Minute(), scheduleTime.Hour())
 			job, err := newCronDurationJob(ctx, schedule, scheduler, k8sclient)
 			Expect(err).NotTo(HaveOccurred())
 
-			timeBeforeSchedule := time.Now()
 			err = job.scheduleJob(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
@@ -319,33 +319,32 @@ var _ = Describe("CronDurationJob", func() {
 			err = k8sclient.Get(ctx, types.NamespacedName{Name: "test-schedule", Namespace: "default"}, updatedSchedule)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(updatedSchedule.Status.Active).To(BeFalse())
-			// time should be the next minute and 0 seconds
-			Expect(updatedSchedule.Status.NextStartTime.Time.Second()).To(BeZero())
-			Expect(updatedSchedule.Status.NextStartTime.Time.Minute()).To(Equal(timeBeforeSchedule.Add(time.Minute).Minute()))
-			Expect(updatedSchedule.Status.NextStartTime.Time.Hour()).To(Equal(timeBeforeSchedule.Hour()))
-			// Expect(updatedSchedule.Status.NextStartTime.Time).To(BeTemporally("~", timeBeforeSchedule.Add(time.Minute), 2*time.Second))
+
+			Expect(updatedSchedule.Status.NextStartTime.Time.Second()).To(Equal(scheduleTime.Second()))
+			Expect(updatedSchedule.Status.NextStartTime.Time.Minute()).To(Equal(scheduleTime.Minute()))
+			Expect(updatedSchedule.Status.NextStartTime.Time.Hour()).To(Equal(scheduleTime.Hour()))
 			Expect(updatedSchedule.Status.MatchingClusters).To(Equal(job.MatchingClusters))
 		})
 	})
 
 	Context("updateJob", func() {
 		It("should update an existing job", func() {
-			// wait a bit if we are too close to second 59...
-			// this is to ensure calculations based on minutes and seconds
-			waitIfTimeIsTooCloseTo59()
-
+			// set the schedule time to now plus 10 seconds so we can check
+			// later knowning the expected value.
+			scheduleTime := time.Now().Add(10 * time.Second)
+			schedule.Spec.Schedule = fmt.Sprintf("%d %d %d * * *", scheduleTime.Second(), scheduleTime.Minute(), scheduleTime.Hour())
 			job, err := newCronDurationJob(ctx, schedule, scheduler, k8sclient)
 			Expect(err).NotTo(HaveOccurred())
 
 			err = job.scheduleJob(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
 			// Modify schedule to force an update
-			schedule.Spec.Schedule = "0 */2 * * * *"
+			newScheduleTime := time.Now().Add(30 * time.Second)
+			schedule.Spec.Schedule = fmt.Sprintf("%d %d %d * * *", newScheduleTime.Second(), newScheduleTime.Minute(), newScheduleTime.Hour())
 			updatedJob, err := newCronDurationJob(ctx, schedule, scheduler, k8sclient)
 			Expect(err).NotTo(HaveOccurred())
 
-			timeBeforeUpdate := time.Now()
 			err = updatedJob.updateJob(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
@@ -361,18 +360,9 @@ var _ = Describe("CronDurationJob", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(updatedSchedule.Status.Active).To(BeFalse())
 
-			// next fire time should be the next even minute and 0 seconds
-			Expect(updatedSchedule.Status.NextStartTime.Time.Second()).To(BeZero())
-			if timeBeforeUpdate.Minute()%2 == 0 {
-				Expect(updatedSchedule.Status.NextStartTime.Time.Minute()).To(Equal(timeBeforeUpdate.Add(2 * time.Minute).Minute()))
-			} else {
-				Expect(updatedSchedule.Status.NextStartTime.Time.Minute()).To(Equal(timeBeforeUpdate.Add(1 * time.Minute).Minute()))
-			}
-			if timeBeforeUpdate.Minute() < 59 {
-				Expect(updatedSchedule.Status.NextStartTime.Time.Hour()).To(Equal(timeBeforeUpdate.Hour()))
-			} else {
-				Expect(updatedSchedule.Status.NextStartTime.Time.Hour()).To(Equal(timeBeforeUpdate.Add(1 * time.Hour).Hour()))
-			}
+			Expect(updatedSchedule.Status.NextStartTime.Time.Second()).To(Equal(newScheduleTime.Second()))
+			Expect(updatedSchedule.Status.NextStartTime.Time.Minute()).To(Equal(newScheduleTime.Minute()))
+			Expect(updatedSchedule.Status.NextStartTime.Time.Hour()).To(Equal(newScheduleTime.Hour()))
 			Expect(updatedSchedule.Status.MatchingClusters).To(Equal(job.MatchingClusters))
 		})
 	})
@@ -552,14 +542,3 @@ var _ = Describe("CronDurationJob", func() {
 		})
 	})
 })
-
-func waitIfTimeIsTooCloseTo59() {
-	for {
-		now := time.Now()
-		// if we are up to second 55... just break
-		if now.Second() < 55 {
-			break
-		}
-		time.Sleep(200 * time.Millisecond)
-	}
-}