feat: add StormService progress deadline - #2604
Conversation
Signed-off-by: Zupeng Wang <71580390+wangzupeng12061@users.noreply.github.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 a progress deadline mechanism for StormService rollouts, adding a new ProgressDeadlineSeconds field to the spec and implementing progress tracking and timeout logic. The review feedback highlights several areas to better align the implementation with standard Kubernetes behaviors and API conventions. Specifically, the reviewer recommends: ensuring that pausing a timed-out rollout transitions its progressing condition to DeploymentPaused to allow recovery; setting the status to ConditionTrue and updating the transition time when resuming; adding a minimum validation marker to prevent negative deadline values; updating the conditions slice in-place rather than overwriting it entirely; and updating the unit tests to reflect these corrected behaviors.
| case stormService.Spec.Paused: | ||
| if currentCondition != nil && currentCondition.Reason == ProgressDeadlineExceededReason { | ||
| condition = *currentCondition | ||
| } else if currentCondition != nil && currentCondition.Reason == PausedReason { | ||
| condition = *currentCondition | ||
| } else { | ||
| condition = newProgressingCondition( | ||
| corev1.ConditionUnknown, | ||
| PausedReason, | ||
| fmt.Sprintf("StormService %q is paused.", stormService.Name), | ||
| now, | ||
| ) | ||
| } |
There was a problem hiding this comment.
When stormService.Spec.Paused is true, the current implementation preserves the ProgressDeadlineExceededReason condition if it was already set. This deviates from standard Kubernetes behavior and prevents a timed-out rollout from being recovered via a pause/resume cycle.\n\nIn Kubernetes, pausing a timed-out deployment transitions its progressing condition to DeploymentPaused (ConditionUnknown). When resumed, it transitions to DeploymentResumed (ConditionTrue), which resets the progress deadline and allows the rollout to attempt recovery.\n\nWe should transition to PausedReason when paused, regardless of whether the previous state was timed out.
case stormService.Spec.Paused:
if currentCondition != nil && currentCondition.Reason == PausedReason {
condition = *currentCondition
} else {
condition = newProgressingCondition(
corev1.ConditionUnknown,
PausedReason,
fmt.Sprintf("StormService %q is paused.", stormService.Name),
now,
)
}There was a problem hiding this comment.
Thanks for asking me to verify this as well. I checked Kubernetes v1.31.8 checkPausedConditions directly. It intentionally returns without replacing an existing ProgressDeadlineExceeded condition when a timed-out Deployment is paused. For a non-timed-out rollout, both DeploymentPaused and DeploymentResumed use ConditionUnknown; resume refreshes LastUpdateTime, while LastTransitionTime remains unchanged because the status stays Unknown.
The current StormService implementation follows those semantics, including excluding paused time from the deadline, so I do not recommend changing this path. Kubernetes reference: https://github.com/kubernetes/kubernetes/blob/v1.31.8/pkg/controller/deployment/sync.go#L75-L94
| case currentCondition != nil && currentCondition.Reason == PausedReason: | ||
| condition = newProgressingCondition( | ||
| corev1.ConditionUnknown, | ||
| ResumedReason, | ||
| fmt.Sprintf("StormService %q is resumed.", stormService.Name), | ||
| now, | ||
| ) | ||
| condition.LastTransitionTime = currentCondition.LastTransitionTime |
There was a problem hiding this comment.
When transitioning from PausedReason to ResumedReason, the condition status is set to ConditionUnknown and LastTransitionTime is overwritten with currentCondition.LastTransitionTime (the time when the service was paused).\n\nThis is incorrect for two reasons:\n1. Resuming a rollout means it is actively attempting to progress, so the status should be ConditionTrue (matching Kubernetes DeploymentResumed behavior).\n2. The transition to Resumed occurs at now (the resume time), so LastTransitionTime must be updated to now instead of keeping the old pause timestamp.
| case currentCondition != nil && currentCondition.Reason == PausedReason: | |
| condition = newProgressingCondition( | |
| corev1.ConditionUnknown, | |
| ResumedReason, | |
| fmt.Sprintf("StormService %q is resumed.", stormService.Name), | |
| now, | |
| ) | |
| condition.LastTransitionTime = currentCondition.LastTransitionTime | |
| case currentCondition != nil && currentCondition.Reason == PausedReason: | |
| condition = newProgressingCondition( | |
| corev1.ConditionTrue, | |
| ResumedReason, | |
| fmt.Sprintf("StormService %q is resumed.", stormService.Name), | |
| now, | |
| ) |
| // +kubebuilder:default=600 | ||
| // +optional | ||
| ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty" protobuf:"varint,9,opt,name=progressDeadlineSeconds"` |
There was a problem hiding this comment.
ProgressDeadlineSeconds currently lacks a minimum validation marker. A negative value would cause the controller to immediately time out the rollout. We should add +kubebuilder:validation:Minimum=0 to prevent invalid negative values.
| // +kubebuilder:default=600 | |
| // +optional | |
| ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty" protobuf:"varint,9,opt,name=progressDeadlineSeconds"` | |
| // +kubebuilder:default=600 | |
| // +kubebuilder:validation:Minimum=0 | |
| // +optional | |
| ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty" protobuf:"varint,9,opt,name=progressDeadlineSeconds"` |
| condition = *currentCondition | ||
| } | ||
|
|
||
| stormService.Status.Conditions = orchestrationv1alpha1.Conditions{condition} |
There was a problem hiding this comment.
The controller currently overwrites the entire stormService.Status.Conditions slice with a single condition. This violates Kubernetes API conventions where multiple conditions (such as Ready and Progressing) should coexist to provide a complete view of the resource's status.\n\nWe should update or append the progressing condition in-place within the slice instead of overwriting it entirely.
| stormService.Status.Conditions = orchestrationv1alpha1.Conditions{condition} | |
| found := false | |
| for i, cond := range stormService.Status.Conditions { | |
| if cond.Type == orchestrationv1alpha1.StormServiceProgressing { | |
| stormService.Status.Conditions[i] = condition | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| stormService.Status.Conditions = append(stormService.Status.Conditions, condition) | |
| } |
| func TestSyncStormServiceProgressingConditionExcludesPausedTime(t *testing.T) { | ||
| started := time.Date(2026, 8, 23, 1, 2, 3, 0, time.UTC) | ||
| oldStatus := orchestrationv1alpha1.StormServiceStatus{ | ||
| Conditions: orchestrationv1alpha1.Conditions{ | ||
| progressingCondition(corev1.ConditionTrue, ProgressingReason, started, started), | ||
| }, | ||
| } | ||
| stormService := &orchestrationv1alpha1.StormService{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "paused"}, | ||
| Spec: orchestrationv1alpha1.StormServiceSpec{ | ||
| Paused: true, | ||
| ProgressDeadlineSeconds: ptr.To[int32](10), | ||
| }, | ||
| Status: *oldStatus.DeepCopy(), | ||
| } | ||
| pausedAt := started.Add(time.Hour) | ||
|
|
||
| syncStormServiceProgressingCondition(stormService, &oldStatus, pausedAt) | ||
| pausedCondition := utils.GetCondition(stormService.Status.Conditions, orchestrationv1alpha1.StormServiceProgressing) | ||
| require.NotNil(t, pausedCondition) | ||
| assert.Equal(t, corev1.ConditionUnknown, pausedCondition.Status) | ||
| assert.Equal(t, PausedReason, pausedCondition.Reason) | ||
|
|
||
| pausedStatus := stormService.Status.DeepCopy() | ||
| stormService.Spec.Paused = false | ||
| resumedAt := pausedAt.Add(time.Hour) | ||
| syncStormServiceProgressingCondition(stormService, pausedStatus, resumedAt) | ||
| resumedCondition := utils.GetCondition(stormService.Status.Conditions, orchestrationv1alpha1.StormServiceProgressing) | ||
| require.NotNil(t, resumedCondition) | ||
| assert.Equal(t, corev1.ConditionUnknown, resumedCondition.Status) | ||
| assert.Equal(t, ResumedReason, resumedCondition.Reason) | ||
| assert.Equal(t, resumedAt, resumedCondition.LastUpdateTime.Time) | ||
| assert.Equal(t, pausedAt, resumedCondition.LastTransitionTime.Time) | ||
| } |
There was a problem hiding this comment.
We should update the unit test to assert the corrected behavior where ResumedReason has status ConditionTrue and its LastTransitionTime is updated to the resume time (resumedAt). We should also add a test case to verify that a timed-out rollout transitions to PausedReason when paused.
func TestSyncStormServiceProgressingConditionExcludesPausedTime(t *testing.T) {
started := time.Date(2026, 8, 23, 1, 2, 3, 0, time.UTC)
oldStatus := orchestrationv1alpha1.StormServiceStatus{
Conditions: orchestrationv1alpha1.Conditions{
progressingCondition(corev1.ConditionTrue, ProgressingReason, started, started),
},
}
stormService := &orchestrationv1alpha1.StormService{
ObjectMeta: metav1.ObjectMeta{Name: "paused"},
Spec: orchestrationv1alpha1.StormServiceSpec{
Paused: true,
ProgressDeadlineSeconds: ptr.To[int32](10),
},
Status: *oldStatus.DeepCopy(),
}
pausedAt := started.Add(time.Hour)
syncStormServiceProgressingCondition(stormService, &oldStatus, pausedAt)
pausedCondition := utils.GetCondition(stormService.Status.Conditions, orchestrationv1alpha1.StormServiceProgressing)
require.NotNil(t, pausedCondition)
assert.Equal(t, corev1.ConditionUnknown, pausedCondition.Status)
assert.Equal(t, PausedReason, pausedCondition.Reason)
pausedStatus := stormService.Status.DeepCopy()
stormService.Spec.Paused = false
resumedAt := pausedAt.Add(time.Hour)
syncStormServiceProgressingCondition(stormService, pausedStatus, resumedAt)
resumedCondition := utils.GetCondition(stormService.Status.Conditions, orchestrationv1alpha1.StormServiceProgressing)
require.NotNil(t, resumedCondition)
assert.Equal(t, corev1.ConditionTrue, resumedCondition.Status)
assert.Equal(t, ResumedReason, resumedCondition.Reason)
assert.Equal(t, resumedAt, resumedCondition.LastUpdateTime.Time)
assert.Equal(t, resumedAt, resumedCondition.LastTransitionTime.Time)
// Test that a timed-out rollout also transitions to Paused when paused
timedOutStatus := orchestrationv1alpha1.StormServiceStatus{
Conditions: orchestrationv1alpha1.Conditions{
progressingCondition(corev1.ConditionFalse, ProgressDeadlineExceededReason, started, started),
},
}
stormService.Status = *timedOutStatus.DeepCopy()
stormService.Spec.Paused = true
syncStormServiceProgressingCondition(stormService, &timedOutStatus, pausedAt)
pausedCondition = utils.GetCondition(stormService.Status.Conditions, orchestrationv1alpha1.StormServiceProgressing)
require.NotNil(t, pausedCondition)
assert.Equal(t, corev1.ConditionUnknown, pausedCondition.Status)
assert.Equal(t, PausedReason, pausedCondition.Reason)
}
Signed-off-by: Zupeng Wang <71580390+wangzupeng12061@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds rollout progress-deadline semantics to StormService so stalled updates can be detected via status conditions (without requiring manual refresh), while continuing reconciliation to allow later recovery.
Changes:
- Introduces
spec.progressDeadlineSecondsforStormService(default 600s) and wires it into controller requeue + progressing condition updates. - Implements progress detection, pause/resume handling, timeout surfacing (
Progressing=False,ProgressDeadlineExceeded), and recovery-on-progress logic. - Regenerates API artifacts (deepcopy, apply-config, CRDs) and adds unit + integration coverage for defaulting, pause, timeout, and recovery behavior.
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/webhook/stormservice_webhook_test.go | Updates defaulting expectations to include progressDeadlineSeconds=600. |
| test/integration/controller/stormservice_test.go | Adds integration tests for deadline defaulting, refresh-on-progress, pause exclusion, timeout + recovery without RoleSet replacement. |
| pkg/controller/stormservice/sync.go | Switches requeue behavior to respect progress-deadline timing when not ready; uses new progressing-condition sync. |
| pkg/controller/stormservice/progress.go | Implements progress detection, progressing condition state machine, and deadline-driven requeue calculation. |
| pkg/controller/stormservice/progress_test.go | Adds unit tests for timeout, refresh-on-progress, pause exclusion, recovery, and requeue timing. |
| pkg/client/applyconfiguration/orchestration/v1alpha1/stormservicespec.go | Adds apply-config builder support for progressDeadlineSeconds. |
| dist/chart/crds/orchestration.aibrix.ai_stormservices.yaml | Adds CRD schema for progressDeadlineSeconds with default 600. |
| config/crd/orchestration/orchestration.aibrix.ai_stormservices.yaml | Adds CRD schema for progressDeadlineSeconds with default 600. |
| api/orchestration/v1alpha1/zz_generated.deepcopy.go | Regenerates deepcopy logic to include ProgressDeadlineSeconds. |
| api/orchestration/v1alpha1/stormservice_types.go | Adds the ProgressDeadlineSeconds field with kubebuilder default and doc comment. |
Files not reviewed (2)
- api/orchestration/v1alpha1/zz_generated.deepcopy.go: Generated file
- pkg/client/applyconfiguration/orchestration/v1alpha1/stormservicespec.go: Generated file
Suppressed comments (1)
pkg/controller/stormservice/progress.go:118
- The timeout message "has timed out progressing" is awkward/unclear. Consider phrasing it as exceeding the progress deadline to match the
ProgressDeadlineExceededreason.
condition = newProgressingCondition(
corev1.ConditionFalse,
ProgressDeadlineExceededReason,
fmt.Sprintf("StormService %q has timed out progressing.", stormService.Name),
now,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ProgressingReason = "Processing" | ||
| ProgressDeadlineExceededReason = "ProgressDeadlineExceeded" | ||
| PausedReason = "DeploymentPaused" | ||
| ResumedReason = "DeploymentResumed" |
| condition = newProgressingCondition( | ||
| corev1.ConditionUnknown, | ||
| ResumedReason, | ||
| fmt.Sprintf("StormService %q is resumed.", stormService.Name), | ||
| now, | ||
| ) |
| // is considered to be failed. The controller will continue to process failed | ||
| // StormServices and surface a condition with a ProgressDeadlineExceeded reason. | ||
| // Progress is not estimated while a StormService is paused. Defaults to 600s. | ||
| // +kubebuilder:default=600 |
There was a problem hiding this comment.
progressDeadlineSeconds should be validated as a positive value. Right now the CRD/API only sets default: 600, but does not add a minimum, so users can create a StormService with progressDeadlineSeconds: 0 or a negative value. can we valid in webhook ?
There was a problem hiding this comment.
Thanks — addressed in 3b3b5f7. I added structural CRD validation with +kubebuilder:validation:Minimum=1, so both zero and negative values are rejected by the API server on create and update. This is enforced independently of the custom webhook availability or its failure policy. I also regenerated the module and Helm CRDs and added integration cases verifying that 0 and -1 return Kubernetes Invalid errors.
googs1025
left a comment
googs1025
left a comment
There was a problem hiding this comment.
Could you clarify the intended behavior after ProgressDeadlineExceeded?
From the current implementation, it looks like the StormService only surfaces Progressing=False with reason ProgressDeadlineExceeded, while the controller continues reconciling and does not roll back or replace the existing RoleSet. Is that the expected behavior, i.e. the rollout stays where it is until later progress/ready state clears it?
Could you clarify the intended pause/resume semantics for the progress deadline?
The API comment says progress is not estimated while a StormService is paused. In the current implementation, when a StormService resumes from DeploymentPaused, the condition is updated to DeploymentResumed with a fresh LastUpdateTime, which appears to restart the progress deadline from resume time rather than preserving elapsed pre-pause time. Is that intentional?
Signed-off-by: Zupeng Wang <71580390+wangzupeng12061@users.noreply.github.com>
|
@googs1025 Thanks for checking these semantics. Yes, both behaviors are intentional and modeled after Kubernetes Deployment progress-deadline handling:
This matches Kubernetes v1.31.8 |
| return time.Duration(seconds) * time.Second | ||
| } | ||
|
|
||
| func syncStormServiceProgressingCondition(stormService *orchestrationv1alpha1.StormService, oldStatus *orchestrationv1alpha1.StormServiceStatus, now time.Time) { |
There was a problem hiding this comment.
Could you help double-check whether replacing the whole Status.Conditions slice here can cause any issue?
In syncStormServiceProgressingCondition, we assign stormService.Status.Conditions = orchestrationv1alpha1.Conditions{condition}. I noticed StormService defines multiple
condition types (Ready, Progressing, ReplicaFailure), and there is also a SetStormServiceCondition helper that preserves other condition types while updating one condition.
Could you verify whether the current StormService status model intentionally keeps only one active condition at a time, or whether this path might accidentally drop another condition that should be preserved?
There was a problem hiding this comment.
Thanks for flagging this. I traced every current StormService Status.Conditions writer. Although the Ready, Progressing, and ReplicaFailure paths currently happen to replace the slice, the API defines multiple condition types and does not document a single-active-condition contract, so replacing the whole slice here can discard an unrelated condition.
I reproduced that risk on the previous head: a focused regression test failed because updating Progressing removed an existing Ready condition. Commit 92687e7 fixes this by filtering out only existing Progressing entries and appending the newly computed Progressing condition, preserving all other condition types unchanged and collapsing malformed duplicate Progressing entries.
I did not call SetStormServiceCondition directly because its same-status/same-reason early return would suppress the LastUpdateTime refresh required when rollout progress is observed. After merging current main, the focused regression, StormService unit and race tests, controller envtest (47/47), webhook envtest (66/66), lint, codegen, manifests, and CRD sync all pass at ae4a360.
Signed-off-by: Zupeng Wang <71580390+wangzupeng12061@users.noreply.github.com>
Signed-off-by: Zupeng Wang <71580390+wangzupeng12061@users.noreply.github.com> # Conflicts: # pkg/client/applyconfiguration/orchestration/v1alpha1/stormservicespec.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 10 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- api/orchestration/v1alpha1/zz_generated.deepcopy.go: Generated file
- pkg/client/applyconfiguration/orchestration/v1alpha1/stormservicespec.go: Generated file
| want := tc.wantStormService() | ||
| want.Spec.ProgressDeadlineSeconds = ptr.To(int32(600)) | ||
| gomega.Expect(model).To(gomega.BeComparableTo(want, |
| func syncStormServiceProgressingCondition(stormService *orchestrationv1alpha1.StormService, oldStatus *orchestrationv1alpha1.StormServiceStatus, now time.Time) { | ||
| currentCondition := utils.GetCondition(oldStatus.Conditions, orchestrationv1alpha1.StormServiceProgressing) | ||
| var condition orchestrationv1alpha1.Condition | ||
|
|
googs1025
left a comment
googs1025
left a comment
There was a problem hiding this comment.
I'am OK, for this pr. LGTM.
cc @varungup90 @Jeffwan to final review
Summary
spec.progressDeadlineSecondstoStormService, defaulting to 600 secondsProgressDeadlineExceeded, and continue reconciling so later progress can recoverRelated to #2534.
Semantics
The implementation follows the existing RayClusterFleet/Kubernetes-style progress contract:
DeploymentPaused; resuming resets the deadline withDeploymentResumedProgressing=False, reasonProgressDeadlineExceeded)This is a Draft PR so maintainers can confirm the desired semantics and pause/resume reason names before it is marked ready.
Validation
go test -race ./pkg/controller/stormservice -count=1make testmake lint-allgo vet ./pkg/controller/stormservicebash hack/verify-codegen.shhack/verify-crd-sync.shThe integration workload uses the existing vLLM Pod shape but does not start the container; this change is confined to Kubernetes control-plane rollout status and requeue behavior.