[Feat] Consume StormService spec.mode in the controller update path and PodAutoscaler - #2617
Conversation
…odAutoscaler Follow-ups to vllm-project#2450 outlined in vllm-project#2449: - The StormService controller now derives the rollout path from a declared spec.mode (Replica -> RollingUpdate path, Pooled -> InPlaceUpdate path) through EffectiveUpdateStrategyType. When spec.mode is unset the legacy updateStrategy.type selection is kept so existing manifests behave as before. - The webhook rejects a declared mode: Replica combined with a declared updateStrategy.type: InPlaceUpdate. The reverse combination (Pooled with RollingUpdate) is not rejected because the CRD defaults spec.updateStrategy.type to RollingUpdate whenever the updateStrategy block is present, making a user-written RollingUpdate indistinguishable from the default; the controller resolves it in favor of the declared mode and logs the override. - PodAutoscaler role-level scaling reads StormService.spec.mode as the source of truth. The autoscaling.aibrix.ai/storm-service-mode annotation is deprecated and only honored when the target StormService does not declare spec.mode. Samples and docs updated accordingly. Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an explicit spec.mode field (Replica or Pooled) for StormService to determine the update path and route role-level autoscaling, deprecating the legacy autoscaling.aibrix.ai/storm-service-mode annotation. It updates the controller, webhook, and autoscaler logic, and adds corresponding tests. The review feedback highlights a critical nil pointer dereference panic in sync.go when spec.replicas is omitted, as well as an issue in workload_scale.go where returning 0 instead of 1 for a nil replica count could disrupt autoscaling calculations.
| case orchestrationv1alpha1.InPlaceUpdateStormServiceStrategyType: | ||
| // The update path follows the declared spec.mode when it is set and falls back to | ||
| // the legacy updateStrategy.type selection otherwise, see EffectiveUpdateStrategyType. | ||
| strategyType, err := EffectiveUpdateStrategyType(stormService) |
There was a problem hiding this comment.
While reviewing the update path changes, we noticed a critical issue regarding how spec.replicas is handled when it is nil (omitted). Since spec.replicas is an optional field (*int32) and has no defaulting webhook or CRD default value, it can be nil in the cluster.
In updateStatus (which is called at line 64 of sync.go during reconciliation), the controller directly dereferences stormService.Spec.Replicas at lines 399 and 400:
stormService.Status.UpdatedReplicas == *stormService.Spec.Replicas
stormService.Status.Replicas == *stormService.Spec.ReplicasThis will cause a critical nil pointer dereference panic and crash the controller whenever a StormService is created with replicas omitted.
Additionally, in scaling (line 148) and rollout (line 268), expectReplica defaults to 0 when Replicas is nil, which contradicts the documented default of 1 and will prevent the controller from creating any RoleSets or performing rollouts.
Please ensure that Replicas is safely checked for nil and defaulted to 1 throughout the controller reconcile loop.
| if ss.Spec.Replicas == nil { | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
Since spec.replicas defaults to 1 when omitted (as documented in stormservice_types.go), returning 0 here when ss.Spec.Replicas is nil can lead to incorrect autoscaling calculations (e.g., division by zero or incorrect scaling ratios). It should return 1 instead to reflect the default replica count.
| if ss.Spec.Replicas == nil { | |
| return 0, nil | |
| } | |
| if ss.Spec.Replicas == nil { | |
| return 1, nil | |
| } |
There was a problem hiding this comment.
Pull request overview
Updates StormService reconciliation and PodAutoscaler behavior to treat StormService.spec.mode as the primary signal for rollout path selection and role-level scaling routing, while keeping legacy behavior when spec.mode is unset and retaining the old PodAutoscaler annotation as a deprecated fallback.
Changes:
- StormService controller rollout path now resolves an effective update strategy via
EffectiveUpdateStrategyType, preferring declaredspec.modeoverspec.updateStrategy.typewhen set. - Webhook validation now rejects the contradictory combination
mode: Replica+updateStrategy.type: InPlaceUpdate. - PodAutoscaler mode routing now prefers
StormService.spec.mode, falling back to the deprecatedautoscaling.aibrix.ai/storm-service-modeannotation only whenspec.modeis unset; docs/samples/tests updated accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/webhook/stormservice_webhook_test.go | Adds integration webhook cases for the new mode/strategy validation combinations. |
| samples/autoscaling/stormservice-replica.yaml | Removes deprecated PodAutoscaler mode annotation; sample StormService declares mode: Replica. |
| samples/autoscaling/stormservice-pool.yaml | Removes deprecated PodAutoscaler mode annotation; sample StormService declares mode: Pooled. |
| pkg/webhook/stormservice_webhook.go | Extends validation to reject Replica + InPlaceUpdate while preserving legacy behavior when mode is unset. |
| pkg/webhook/stormservice_webhook_test.go | Adds unit tests covering the new mode/strategy validation behavior on create/update. |
| pkg/controller/stormservice/utils.go | Introduces EffectiveUpdateStrategyType to derive rollout path from declared mode (with override logging). |
| pkg/controller/stormservice/utils_test.go | Adds unit coverage for EffectiveUpdateStrategyType behavior and precedence rules. |
| pkg/controller/stormservice/sync.go | Switches rollout path selection to use EffectiveUpdateStrategyType. |
| pkg/controller/podautoscaler/workload_scale.go | Adds mode resolution helper preferring spec.mode and updates get/set role scaling paths to use it. |
| pkg/controller/podautoscaler/workload_scale_test.go | Adds tests for mode precedence and mode-driven get/set scaling behavior. |
| docs/source/features/autoscaling/metric-based-autoscaling.rst | Updates autoscaling docs to point to StormService.spec.mode and deprecate the annotation. |
| docs/source/designs/aibrix-stormservice.rst | Updates design doc to state that declared spec.mode drives rollout path and autoscaler routing. |
Suppressed comments (1)
pkg/controller/stormservice/utils.go:126
- Same as above: this override log is expected to be hit repeatedly for pooled StormServices that have updateStrategy.type stored as RollingUpdate (including CRD-defaulted values). Prefer a lower verbosity level to avoid steady-state log spam.
case orchestrationv1alpha1.StormServicePooledMode:
if declaredType == orchestrationv1alpha1.RollingUpdateStormServiceStrategyType {
klog.Infof("stormservice %s/%s declares mode %s, overriding updateStrategy.type %s with %s",
stormService.Namespace, stormService.Name, mode, declaredType, orchestrationv1alpha1.InPlaceUpdateStormServiceStrategyType)
}
return orchestrationv1alpha1.InPlaceUpdateStormServiceStrategyType, nil
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Replica mode scales the whole StormService, so report spec.replicas directly. | ||
| if stormServiceScalingMode(pa, ss) == orchestrationv1alpha1.StormServiceReplicaMode { | ||
| if ss.Spec.Replicas == nil { | ||
| return 0, nil | ||
| } | ||
| return *ss.Spec.Replicas, nil | ||
| } |
| case orchestrationv1alpha1.StormServiceReplicaMode: | ||
| if declaredType == orchestrationv1alpha1.InPlaceUpdateStormServiceStrategyType { | ||
| klog.Infof("stormservice %s/%s declares mode %s, overriding updateStrategy.type %s with %s", | ||
| stormService.Namespace, stormService.Name, mode, declaredType, orchestrationv1alpha1.RollingUpdateStormServiceStrategyType) | ||
| } | ||
| return orchestrationv1alpha1.RollingUpdateStormServiceStrategyType, nil |
Code Review Suggestions1.
|
…oled mode never surges Review follow-up for vllm-project#2617: IsRollingUpdate() still read spec.updateStrategy.type directly, so MaxSurge, MaxUnavailable and MinAvailable kept their RollingUpdate budgets for mode: Pooled objects whose type is the CRD-defaulted RollingUpdate. scaling() consumes those budgets on every reconcile, and with maxSurge > 0 it could create a replacement RoleSet while the old one was still terminating, violating the pooled single-RoleSet contract. IsRollingUpdate now resolves the strategy through EffectiveUpdateStrategyType, which makes all three helpers evaluate to 0 in Pooled mode (and on an unresolvable mode, which rollout() already rejects). Also lower the mode-override logs in EffectiveUpdateStrategyType to klog.V(4): the Pooled+RollingUpdate override is steady state (the CRD defaults the type) and fired every reconcile, and the Replica+InPlaceUpdate override is unreachable past the webhook guard. Tests: TestScalingPooledModeNeverCreatesSecondRoleSet exercises scaling() with mode: Pooled, defaulted RollingUpdate and maxSurge: 2 and asserts a second RoleSet is never created (it fails against the previous helper); TestPooledModeZeroesSurgeAndUnavailable and extended TestIsRollingUpdate cover the helper contract. Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review. Both addressed in d419b4b. 1. Good catch on the scaling path. Added 2. Steady-state override logging Both override logs in
|
| case orchestrationv1alpha1.InPlaceUpdateStormServiceStrategyType: | ||
| // The update path follows the declared spec.mode when it is set and falls back to | ||
| // the legacy updateStrategy.type selection otherwise, see EffectiveUpdateStrategyType. | ||
| strategyType, err := EffectiveUpdateStrategyType(stormService) |
There was a problem hiding this comment.
Could you please double-check the spec.replicas == nil handling around the reconcile path? spec.replicas is optional, and the generated CRD does not seem to set a real default: 1. If omitted replicas can reach reconcile, scaling() / rollout() appear to derive expectReplica as 0, while updateStatus() still compares against *stormService.Spec.Replicas. Could you verify whether replicas is actually defaulted before reconcile in this codebase, or whether these paths should resolve nil to the documented default of 1?
| if stormServiceScalingMode(pa, ss) == orchestrationv1alpha1.StormServiceReplicaMode { | ||
| if ss.Spec.Replicas == nil { | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
Could you please double-check this nil branch? Since spec.replicas is optional and documented as defaulting to 1, returning 0 here may make the autoscaler interpret an omitted replica count as zero replicas. Could you verify whether this field is defaulted before the autoscaler reads it, or whether this should resolve nil to the documented default of 1?
Summary
Implements the follow-ups googs1025 outlined in #2449 after #2450 merged:
spec.mode/ResolvedMode()for the update path instead of readingupdateStrategy.typedirectly, with webhook validation for conflicting declared combinations.autoscaling.aibrix.ai/storm-service-modeannotation toStormService.spec.mode, with the annotation kept as a deprecated compatibility fallback during migration.Changes
Controller (
pkg/controller/stormservice):rollout()now selects the path through a newEffectiveUpdateStrategyTypehelper. A declaredmode: Replicatakes the rolling path and a declaredmode: Pooledtakes the in-place path. Whenspec.modeis unset, the legacyupdateStrategy.typeswitch is kept unchanged, so manifests withoutspec.modebehave exactly as before (ResolvedMode()'s replicas-based inference is intentionally not used there because it does not always match the declared strategy, e.g.replicas: 1withRollingUpdate).Webhook (
pkg/webhook):validateStormServiceModeadditionally rejects a declaredmode: Replicacombined with a declaredupdateStrategy.type: InPlaceUpdate, on create and update. The reverse combination (Pooled+RollingUpdate) is deliberately not rejected: the CRD defaultsspec.updateStrategy.typetoRollingUpdatewhenever theupdateStrategyblock is present, so at admission a user-writtenRollingUpdateis indistinguishable from the default and rejecting it would break plainmode: Pooledmanifests. The controller resolves that case in favor of the declared mode and logs the override.PodAutoscaler (
pkg/controller/podautoscaler): newstormServiceScalingModeresolves the mode as declaredspec.modefirst, then thestorm-service-modeannotation ("replica"), then the legacy pooled default. Both the role-level read (getCurrentReplicasForRole) and write (setDesiredReplicasForRole) paths use it. The annotation constant is marked deprecated. A nilspec.replicasis no longer dereferenced on the replica-mode read path.Docs and samples: design doc notes that a declared
spec.modedrives the update path and autoscaler routing; the metric-based autoscaling doc points tospec.modewith the annotation as deprecated fallback; the sample PodAutoscalers drop the annotation since the StormServices in those samples already declarespec.modeafter #2450.Tests: unit tables for
EffectiveUpdateStrategyType, the new mode/strategy webhook validation,stormServiceScalingModeprecedence (including a declared mode winning over a stale annotation), and mode-driven get/set scaling paths; integration webhook cases for the new rejection plus the accepted combinations.Backward compatibility:
spec.mode: no behavior change in controller path selection, validation, or autoscaler routing.mode: Pooledwith a defaulted or declaredRollingUpdatenow takes the in-place path; declaredmode: ReplicawithInPlaceUpdateis now rejected at admission. Both combinations can only exist on objects that declaredspec.mode, which shipped in [API] Add explicit StormService deployment mode field #2450 (merged 2026-08-25, unreleased).Validation
golangci-lint run(v1.57.2, whole repo): exit 0, no findings.go build ./...: exit 0.Related Issues
Part of #2449. The
/scalesubresource enforcement formode: Pooledfrom the same follow-up list is left as a separate change.🤖 Generated with Claude Code