feat: cron-based MonoVertex autoscaling spec#3558
Conversation
Define scheduled scale bounds and reject invalid cron windows before resources are reconciled. Co-authored-by: Sarvesh Sawant <[email protected]> Signed-off-by: Shantanu Vashishtha <[email protected]> Signed-off-by: Sarvesh Sawant <[email protected]>
Apply scheduled replica bounds before daemon metrics are available while preserving gradual, metric-driven scaling within active windows. Co-authored-by: Shantanu Vashishtha <[email protected]> Signed-off-by: Sarvesh Sawant <[email protected]> Signed-off-by: Shantanu Vashishtha <[email protected]>
| Timezone string `json:"timezone" protobuf:"bytes,1,opt,name=timezone"` | ||
| Schedules []CronSchedule `json:"schedules" protobuf:"bytes,2,rep,name=schedules"` |
There was a problem hiding this comment.
nit: Could we document the cron format being used. Standard vs extended
I see it documented below
| // Start of the cron window. Linux cron format (Minute Hour Dom Month Dow). | ||
| Start string `json:"start" protobuf:"bytes,1,opt,name=start"` |
There was a problem hiding this comment.
nit: For sideinputs we're using extended format (6 fields). This might create confusion between users about which one to use.
ref: #3054
|
|
||
| // IsActiveAt reports whether time t falls within this cron window. | ||
| func (cs CronSchedule) IsActiveAt(t time.Time) bool { | ||
| parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) |
There was a problem hiding this comment.
| parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) | |
| parser := cron.NewParser(cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) |
| // CronScheduling defines cron-based autoscaling overrides. | ||
| type CronScheduling struct { | ||
| // Timezone for interpreting cron expressions. IANA Time Zone Database format. | ||
| Timezone string `json:"timezone" protobuf:"bytes,1,opt,name=timezone"` |
There was a problem hiding this comment.
nit: Can this be made optional with default being UTC?
| for i := range s.Cron.Schedules { | ||
| if s.Cron.Schedules[i].IsActiveAt(at) { | ||
| return &s.Cron.Schedules[i], true | ||
| } |
There was a problem hiding this comment.
We should document that we honor the first matching schedule in case when there exist multiple overlapping schedules.
|
Could we also please update the autoscaling doc (specifically the mvtx section) |
|
|
||
| // IsActiveAt reports whether time t falls within this cron window. | ||
| func (cs CronSchedule) IsActiveAt(t time.Time) bool { | ||
| parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) |
There was a problem hiding this comment.
parsed schedules should be pre-computed, it is computationally heavy and performance will be impacted since it is in hot code path of reconciliation.
| func (s Scale) GetEffectiveScaleBounds() (int32, int32, bool) { | ||
| minR := s.GetMinReplicas() | ||
| maxR := s.GetMaxReplicas() | ||
| sched, active := s.GetActiveCronSchedule(time.Now()) |
There was a problem hiding this comment.
this can make it tough to unit-test, can we pass time.Now() as an arg.
There was a problem hiding this comment.
227 + if v.Scale.Cron != nil {
228 + return fmt.Errorf("vertex %q: cron autoscaling is not supported for pipeline vertices", v.Name)
229 + }
we need to add this pipeline else it will be silently ignored.
| if scale.Min != nil && *scale.Min < 0 { | ||
| return fmt.Errorf("scale.min must not be negative") | ||
| } | ||
| if scale.Max != nil && *scale.Max < 0 { | ||
| return fmt.Errorf("scale.max must not be negative") | ||
| } |
There was a problem hiding this comment.
The cronMin >= parentMin and cronMax <= parentMax is concerning. This blocks the common pattern where parentMin=1 (always keep at least one replica running) but during a quiet window you want the cron to scale down to 0. If the intent of cron overrides is to override the parent bounds, this constraint should be relaxed.
| if cronActive { | ||
| if current < minReplicas { | ||
| return s.scaleUp(ctx, monoVtx, current, minReplicas, secondsSinceLastScale, scaleUpCooldown) | ||
| } | ||
| if current > maxReplicas { | ||
| return s.scaleDown(ctx, monoVtx, current, maxReplicas, secondsSinceLastScale, scaleDownCooldown) | ||
| } |
There was a problem hiding this comment.
log.Infof("Cron window active [min=%d, max=%d], current=%d is outside bounds; scaling.", minReplicas, maxReplicas, current)
Precompute cron schedules in the autoscaler and align validation with six-field expressions and UTC defaults. Reject cron configuration on unsupported Pipeline vertices and document schedule precedence. Co-authored-by: Shantanu Vashishtha <[email protected]> Signed-off-by: Sarvesh Sawant <[email protected]> Signed-off-by: Shantanu Vashishtha <[email protected]>
d527eba to
e2a0a35
Compare
|
Thanks for the thorough reviews, @vaibhavtiwari33 and @vigith! The latest commit addresses all the comments. Summary below. @vigith
@vaibhavtiwari33
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3558 +/- ##
==========================================
- Coverage 83.58% 83.57% -0.01%
==========================================
Files 311 311
Lines 83097 83220 +123
==========================================
+ Hits 69455 69550 +95
- Misses 13046 13055 +9
- Partials 596 615 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@vigith Can you approve the awaiting workflows so that we can merge the changes ? |
| targetProcessingSeconds: 20 # Optional, defaults to 20. | ||
| replicasPerScaleUp: 2 # Optional, defaults to 2. | ||
| replicasPerScaleDown: 2 # Optional, defaults to 2. | ||
| cron: # Optional; supported only for MonoVertex. |
There was a problem hiding this comment.
nit:
| cron: # Optional; supported only for MonoVertex. | |
| cron: # Optional; currently only supported for MonoVertex. |
Since, I believe there exists an issue to add this functionality for pipeline as well
|
@vigith can you re-trigger/approve this [test / E2E Tests (jetstream, e2e) (pull_request)] , don't know what is this |
it is successful now, Github Runners are not very stable |
What this PR does / why we need it
Adds cron-window-based autoscaling bounds for MonoVertex.
Users can configure recurring windows with minimum and maximum replica
counts. While a window is active:
bound.
scheduled scale-up from zero.
When cron is omitted, existing autoscaling behavior is unchanged.
Example:
Related issues
Fixes #3546
Parent issue: #3544
Testing
make testmake pre-push -Bgo vet ./pkg/apis/numaflow/v1alpha1 ./pkg/reconciler/monovertex/scaling ./pkg/reconciler/validatorgit diff --checkspec.replicasbefore metrics.replicasPerScaleUp.Special notes for reviewers
Scaleis shared, so generated Pipeline and Vertex CRDs also contain thecron schema. Pipeline runtime support is outside this sub-issue.
generated.
Contributors