vllm-project / vllm-project/aibrix
Make StormService deployment mode explicit
- Dominant language
- Go
- Stars
- 5.1k
- Forks
- 697
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 104
Description
## Problem
StormService currently has two deployment modes, but the mode is implicit:
- `spec.replicas > 1` means replica mode.
- `spec.replicas == 1` means pooled mode.
This makes the API hard to understand and easy to misuse because `spec.replicas` is doing two jobs at once:
1. It is the desired number of RoleSets.
2. It is also used as the deployment mode selector.
The ambiguity is especially visible when `spec.replicas == 1`. A user may intend either:
- pooled mode: one RoleSet with independently scalable roles, or
- replica mode with a single service replica.
The autoscaling path already exposes this ambiguity. `PodAutoscaler` uses the annotation `autoscaling.aibrix.ai/storm-service-mode` to decide whether role-level scaling should update `StormService.spec.replicas` or `StormService.spec.template.spec.roles[].replicas`. The code comment also notes that it is hard to know whether `replicas=1` means pool mode or replica mode.
## Why this matters
For users, the current API is too implicit:
- There is no obvious field showing whether a StormService is in pooled mode or replica mode.
- `replicas: 1` is overloaded and cannot distinguish pooled mode from single-replica replica mode.
- Autoscaling requires a separate annotation to recover the missing mode signal.
- Documentation explains the convention, but the resource itself does not encode the intent.
This can lead to surprising scaling behavior, especially for users who start with one replica and later add autoscaling or role-level scaling.
## Current examples
Replica mode:
```yaml
spec:
replicas: 3
updateStrategy:
type: RollingUpdate
template:
spec:
roles:
- name: prefill
replicas: 1
- name: decode
replicas: 1
```
Pooled mode:
```yaml
spec:
replicas: 1
updateStrategy:
type: InPlaceUpdate
template:
spec:
roles:
- name: prefill
replicas: 4
- name: decode
replicas: 8
```
Both are valid, but the mode is inferred rather than declared.
## Recommended implementation
Add an explicit deployment mode field to `StormServiceSpec`, for example:
```go
type StormServiceMode string
const (
StormServiceReplicaMode StormServiceMode = "Replica"
StormServicePooledMode StormServiceMode = "Pooled"
)
type StormServiceSpec struct {
Mode StormServiceMode `json:"mode,omitempty"`
Replicas *int32 `json:"replicas,omitempty"`
// ...
}
```
Suggested semantics:
- `mode: Replica`
- `spec.replicas` means number of RoleSets.
- `replicas: 1` is valid and means one replica-mode RoleSet.
- Autoscaling the whole StormService updates `spec.replicas`.
- `mode: Pooled`
- `spec.replicas` should be `1` or omitted/defaulted to `1`.
- Each role is scaled through `spec.template.spec.roles[].replicas`.
- Role-level autoscaling updates the selected role replicas.
## Backward compatibility
To avoid breaking existing users:
1. Keep `mode` optional initially.
2. If `mode` is omitted, preserve the current behavior:
- `replicas > 1` => replica mode
- `replicas == 1` or omitted => pooled mode
3. Add defaulting and status reporting so users can see the resolved mode.
4. Update documentation and samples to set `spec.mode` explicitly.
5. Eventually consider warning on omitted mode once the field is broadly adopted.
## Webhook validation
The webhook can make invalid combinations explicit:
- `mode: Pooled` with `spec.replicas > 1` should be rejected or normalized, depending on compatibility preference.
- `mode: Replica` should allow `spec.replicas >= 1`.
- Role-level autoscaling should require `mode: Pooled`, unless the intended behavior is clearly documented for replica mode.
## Autoscaler changes
After `spec.mode` exists, `PodAutoscaler` should read `StormService.spec.mode` instead of relying on:
```yaml
metadata:
annotations:
autoscaling.aibrix.ai/storm-service-mode: "pool"
```
The annotation can be kept temporarily as a compatibility fallback, but `spec.mode` should become the source of truth.
## Status and observability
Consider adding the resolved mode to status:
```yaml
status:
mode: Pooled
```
This would make `kubectl get stormservice -o yaml` self-explanatory and help users/debuggers understand how the controller interpreted the resource.
## Tests
Suggested test coverage:
- API defaulting/resolution when `mode` is omitted.
- Validation for `mode: Pooled` with `replicas > 1`.
- `mode: Replica` with `replicas: 1` remains valid.
- Autoscaler updates `spec.replicas` for replica mode.
- Autoscaler updates `spec.template.spec.roles[].replicas` for pooled mode.
- Existing manifests without `mode` continue to behave as before.
Contributor guide
Research direction
Start by locating StormServiceSpec and the PodAutoscaler mode decision, then inspect the webhook validation and defaulting paths. Use the suggested API, autoscaler, compatibility, and existing-manifest tests to confirm omitted-mode behavior and explicit pooled or replica handling; update the documentation and samples so the resolved mode is visible and test-covered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design, devops
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 40/100