diff --git a/cmd/main.go b/cmd/main.go index 5d8acf1e7..00b2b4a0a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -487,25 +487,7 @@ func main() { monitor := reservations.NewMonitor(multiclusterClient) metrics.Registry.MustRegister(&monitor) commitmentsConfig := conf.GetConfigOrDie[commitments.Config]() - commitmentsDefaults := commitments.DefaultConfig() - if commitmentsConfig.RequeueIntervalActive == 0 { - commitmentsConfig.RequeueIntervalActive = commitmentsDefaults.RequeueIntervalActive - } - if commitmentsConfig.RequeueIntervalRetry == 0 { - commitmentsConfig.RequeueIntervalRetry = commitmentsDefaults.RequeueIntervalRetry - } - if commitmentsConfig.PipelineDefault == "" { - commitmentsConfig.PipelineDefault = commitmentsDefaults.PipelineDefault - } - if commitmentsConfig.SchedulerURL == "" { - commitmentsConfig.SchedulerURL = commitmentsDefaults.SchedulerURL - } - if commitmentsConfig.ChangeAPIWatchReservationsTimeout == 0 { - commitmentsConfig.ChangeAPIWatchReservationsTimeout = commitmentsDefaults.ChangeAPIWatchReservationsTimeout - } - if commitmentsConfig.ChangeAPIWatchReservationsPollInterval == 0 { - commitmentsConfig.ChangeAPIWatchReservationsPollInterval = commitmentsDefaults.ChangeAPIWatchReservationsPollInterval - } + commitmentsConfig.ApplyDefaults() if err := (&commitments.CommitmentReservationController{ Client: multiclusterClient, @@ -673,10 +655,7 @@ func main() { must.Succeed(metrics.Registry.Register(syncerMonitor)) syncer := commitments.NewSyncer(multiclusterClient, syncerMonitor) syncerConfig := conf.GetConfigOrDie[commitments.SyncerConfig]() - syncerDefaults := commitments.DefaultSyncerConfig() - if syncerConfig.SyncInterval == 0 { - syncerConfig.SyncInterval = syncerDefaults.SyncInterval - } + syncerConfig.ApplyDefaults() if err := (&task.Runner{ Client: multiclusterClient, Interval: syncerConfig.SyncInterval, diff --git a/internal/scheduling/reservations/commitments/config.go b/internal/scheduling/reservations/commitments/config.go index 98a5d59ae..7a6c9005f 100644 --- a/internal/scheduling/reservations/commitments/config.go +++ b/internal/scheduling/reservations/commitments/config.go @@ -59,6 +59,31 @@ type Config struct { EnableReportCapacityAPI bool `json:"committedResourceEnableReportCapacityAPI"` } +// ApplyDefaults fills in any unset values with defaults. +func (c *Config) ApplyDefaults() { + defaults := DefaultConfig() + if c.RequeueIntervalActive == 0 { + c.RequeueIntervalActive = defaults.RequeueIntervalActive + } + if c.RequeueIntervalRetry == 0 { + c.RequeueIntervalRetry = defaults.RequeueIntervalRetry + } + if c.PipelineDefault == "" { + c.PipelineDefault = defaults.PipelineDefault + } + if c.SchedulerURL == "" { + c.SchedulerURL = defaults.SchedulerURL + } + if c.ChangeAPIWatchReservationsTimeout == 0 { + c.ChangeAPIWatchReservationsTimeout = defaults.ChangeAPIWatchReservationsTimeout + } + if c.ChangeAPIWatchReservationsPollInterval == 0 { + c.ChangeAPIWatchReservationsPollInterval = defaults.ChangeAPIWatchReservationsPollInterval + } + // Note: EnableChangeCommitmentsAPI, EnableReportUsageAPI, EnableReportCapacityAPI + // are booleans where false is a valid value, so we don't apply defaults for them +} + func DefaultConfig() Config { return Config{ RequeueIntervalActive: 5 * time.Minute, diff --git a/internal/scheduling/reservations/commitments/syncer.go b/internal/scheduling/reservations/commitments/syncer.go index cb3d2fdb4..1949186a6 100644 --- a/internal/scheduling/reservations/commitments/syncer.go +++ b/internal/scheduling/reservations/commitments/syncer.go @@ -36,6 +36,15 @@ func DefaultSyncerConfig() SyncerConfig { } } +// ApplyDefaults fills in any unset values with defaults. +func (c *SyncerConfig) ApplyDefaults() { + defaults := DefaultSyncerConfig() + if c.SyncInterval == 0 { + c.SyncInterval = defaults.SyncInterval + } + // Note: KeystoneSecretRef and SSOSecretRef are not defaulted as they require explicit configuration +} + type Syncer struct { // Client to fetch commitments from Limes CommitmentsClient @@ -43,6 +52,8 @@ type Syncer struct { client.Client // Monitor for metrics monitor *SyncerMonitor + // SyncInterval is stored for logging purposes (actual interval managed by task.Runner) + syncInterval time.Duration } func NewSyncer(k8sClient client.Client, monitor *SyncerMonitor) *Syncer { @@ -54,6 +65,7 @@ func NewSyncer(k8sClient client.Client, monitor *SyncerMonitor) *Syncer { } func (s *Syncer) Init(ctx context.Context, config SyncerConfig) error { + s.syncInterval = config.SyncInterval if err := s.CommitmentsClient.Init(ctx, s.Client, config); err != nil { return err } @@ -191,7 +203,7 @@ func (s *Syncer) SyncReservations(ctx context.Context) error { ctx = WithNewGlobalRequestID(ctx) logger := LoggerFromContext(ctx).WithValues("component", "syncer", "runID", runID) - logger.Info("starting commitment sync with sync interval", "interval", DefaultSyncerConfig().SyncInterval) + logger.Info("starting commitment sync") // Record sync run if s.monitor != nil { diff --git a/pkg/task/runner.go b/pkg/task/runner.go index 89113699a..67e13ef16 100644 --- a/pkg/task/runner.go +++ b/pkg/task/runner.go @@ -53,12 +53,26 @@ func (r *Runner) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, return ctrl.Result{}, nil } +// MinInterval is the minimum allowed interval for task runners to prevent panics from time.NewTicker(0). +const MinInterval = 1 * time.Millisecond + // Start starts the task runner, which will send events at the specified interval. func (r *Runner) Start(ctx context.Context) error { log := log.FromContext(ctx) - log.Info("starting task runner", "name", r.Name, "interval", r.Interval) - ticker := time.NewTicker(r.Interval) + // Safety: ensure interval is at least MinInterval to prevent tight loops or panics + interval := r.Interval + if interval < MinInterval { + log.Info("task runner interval too low, using minimum", + "name", r.Name, + "configuredInterval", r.Interval, + "minInterval", MinInterval) + interval = MinInterval + } + + log.Info("starting task runner", "name", r.Name, "interval", interval) + + ticker := time.NewTicker(interval) defer ticker.Stop() defer close(r.eventCh) diff --git a/pkg/task/runner_test.go b/pkg/task/runner_test.go index 0ba3c86dc..260dc7466 100644 --- a/pkg/task/runner_test.go +++ b/pkg/task/runner_test.go @@ -251,6 +251,33 @@ func TestRunner_SetupWithManager_EventChannelCreation(t *testing.T) { } } +func TestRunner_Start_ZeroInterval_UsesMinimum(t *testing.T) { + runner := &Runner{ + Name: "test-task", + Interval: 0, // Zero interval should use MinInterval (1ms) + eventCh: make(chan event.GenericEvent, 10), + } + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + // Run Start in goroutine since it blocks until context is cancelled + go func() { + // This should NOT panic even with 0 interval + if err := runner.Start(ctx); err != nil { + t.Logf("Start returned error: %v (expected on context cancellation)", err) + } + }() + + // Wait for initial event with generous timeout to avoid flakiness on slow machines + select { + case <-runner.eventCh: + // Success - got initial event without panic + case <-time.After(1 * time.Second): + t.Error("Expected to receive initial event") + } +} + func TestRunner_EventStructure(t *testing.T) { runner := &Runner{ Name: "test-task",