-
Notifications
You must be signed in to change notification settings - Fork 665
feat: add StormService progress deadline #2604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
6a60abd
c55b695
3b3b5f7
92687e7
ae4a360
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -54,6 +54,15 @@ type StormServiceSpec struct { | |||||||||||||||
| // +optional | ||||||||||||||||
| Paused bool `json:"paused,omitempty" protobuf:"varint,7,opt,name=paused"` | ||||||||||||||||
|
|
||||||||||||||||
| // The maximum time in seconds for a StormService to make progress before it | ||||||||||||||||
| // 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 | ||||||||||||||||
| // +kubebuilder:validation:Minimum=1 | ||||||||||||||||
| // +optional | ||||||||||||||||
| ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty" protobuf:"varint,9,opt,name=progressDeadlineSeconds"` | ||||||||||||||||
|
Comment on lines
+69
to
+72
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| // DisruptionTolerance indicates how many roleSets can be unavailable during the preemption/eviction. | ||||||||||||||||
| // +optional | ||||||||||||||||
| DisruptionTolerance DisruptionTolerance `json:"disruptionTolerance,omitempty"` | ||||||||||||||||
|
|
||||||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,144 @@ | ||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||
| Copyright 2026 The Aibrix Team. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||||||
| you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||||||
| You may obtain a copy of the License at | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||||||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||||||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||||||||||
| See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||
| limitations under the License. | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| package stormservice | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| corev1 "k8s.io/api/core/v1" | ||||||||||||||||||||||||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| orchestrationv1alpha1 "github.com/vllm-project/aibrix/api/orchestration/v1alpha1" | ||||||||||||||||||||||||||||||||
| utils "github.com/vllm-project/aibrix/pkg/controller/util/orchestration" | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||
| defaultProgressDeadlineSeconds int32 = 600 | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| ProgressingReason = "Processing" | ||||||||||||||||||||||||||||||||
| ProgressDeadlineExceededReason = "ProgressDeadlineExceeded" | ||||||||||||||||||||||||||||||||
| PausedReason = "DeploymentPaused" | ||||||||||||||||||||||||||||||||
| ResumedReason = "DeploymentResumed" | ||||||||||||||||||||||||||||||||
|
Comment on lines
+33
to
+36
|
||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func newProgressingCondition(status corev1.ConditionStatus, reason, message string, now time.Time) orchestrationv1alpha1.Condition { | ||||||||||||||||||||||||||||||||
| timestamp := metav1.NewTime(now) | ||||||||||||||||||||||||||||||||
| return orchestrationv1alpha1.Condition{ | ||||||||||||||||||||||||||||||||
| Type: orchestrationv1alpha1.StormServiceProgressing, | ||||||||||||||||||||||||||||||||
| Status: status, | ||||||||||||||||||||||||||||||||
| LastUpdateTime: ×tamp, | ||||||||||||||||||||||||||||||||
| LastTransitionTime: ×tamp, | ||||||||||||||||||||||||||||||||
| Reason: reason, | ||||||||||||||||||||||||||||||||
| Message: message, | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func stormServiceProgressing(oldStatus, newStatus *orchestrationv1alpha1.StormServiceStatus) bool { | ||||||||||||||||||||||||||||||||
| oldReplicas := oldStatus.Replicas - oldStatus.UpdatedReplicas | ||||||||||||||||||||||||||||||||
| newReplicas := newStatus.Replicas - newStatus.UpdatedReplicas | ||||||||||||||||||||||||||||||||
| return newStatus.UpdatedReplicas > oldStatus.UpdatedReplicas || | ||||||||||||||||||||||||||||||||
| newReplicas < oldReplicas || | ||||||||||||||||||||||||||||||||
| newStatus.ReadyReplicas > oldStatus.ReadyReplicas || | ||||||||||||||||||||||||||||||||
| newStatus.UpdatedReadyReplicas > oldStatus.UpdatedReadyReplicas | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func progressDeadline(stormService *orchestrationv1alpha1.StormService) time.Duration { | ||||||||||||||||||||||||||||||||
| seconds := defaultProgressDeadlineSeconds | ||||||||||||||||||||||||||||||||
| if stormService.Spec.ProgressDeadlineSeconds != nil { | ||||||||||||||||||||||||||||||||
| seconds = *stormService.Spec.ProgressDeadlineSeconds | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return time.Duration(seconds) * time.Second | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func syncStormServiceProgressingCondition(stormService *orchestrationv1alpha1.StormService, oldStatus *orchestrationv1alpha1.StormServiceStatus, now time.Time) { | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you help double-check whether replacing the whole In 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||||||||||||
| currentCondition := utils.GetCondition(oldStatus.Conditions, orchestrationv1alpha1.StormServiceProgressing) | ||||||||||||||||||||||||||||||||
| var condition orchestrationv1alpha1.Condition | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
Comment on lines
+68
to
+71
|
||||||||||||||||||||||||||||||||
| switch { | ||||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+73
to
+85
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When 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,
)
}
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. plz check this too
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+87
to
+92
|
||||||||||||||||||||||||||||||||
| condition.LastTransitionTime = currentCondition.LastTransitionTime | ||||||||||||||||||||||||||||||||
|
Comment on lines
+86
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When transitioning from
Suggested change
|
||||||||||||||||||||||||||||||||
| case stormServiceProgressing(oldStatus, &stormService.Status): | ||||||||||||||||||||||||||||||||
| condition = newProgressingCondition( | ||||||||||||||||||||||||||||||||
| corev1.ConditionTrue, | ||||||||||||||||||||||||||||||||
| ProgressingReason, | ||||||||||||||||||||||||||||||||
| fmt.Sprintf("StormService %q is progressing.", stormService.Name), | ||||||||||||||||||||||||||||||||
| now, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| if currentCondition != nil && currentCondition.Status == corev1.ConditionTrue { | ||||||||||||||||||||||||||||||||
| condition.LastTransitionTime = currentCondition.LastTransitionTime | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| case currentCondition == nil || currentCondition.LastUpdateTime == nil: | ||||||||||||||||||||||||||||||||
| condition = newProgressingCondition( | ||||||||||||||||||||||||||||||||
| corev1.ConditionTrue, | ||||||||||||||||||||||||||||||||
| ProgressingReason, | ||||||||||||||||||||||||||||||||
| fmt.Sprintf("StormService %q is progressing.", stormService.Name), | ||||||||||||||||||||||||||||||||
| now, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| case currentCondition.Reason == ProgressDeadlineExceededReason: | ||||||||||||||||||||||||||||||||
| condition = *currentCondition | ||||||||||||||||||||||||||||||||
| case !currentCondition.LastUpdateTime.Add(progressDeadline(stormService)).After(now): | ||||||||||||||||||||||||||||||||
| condition = newProgressingCondition( | ||||||||||||||||||||||||||||||||
| corev1.ConditionFalse, | ||||||||||||||||||||||||||||||||
| ProgressDeadlineExceededReason, | ||||||||||||||||||||||||||||||||
| fmt.Sprintf("StormService %q has timed out progressing.", stormService.Name), | ||||||||||||||||||||||||||||||||
| now, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||
| condition = *currentCondition | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| stormService.Status.Conditions = orchestrationv1alpha1.Conditions{condition} | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The controller currently overwrites the entire
Suggested change
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func progressDeadlineRequeueAfter(stormService *orchestrationv1alpha1.StormService, now time.Time) time.Duration { | ||||||||||||||||||||||||||||||||
| if stormService.Spec.Paused { | ||||||||||||||||||||||||||||||||
| return DefaultRequeueAfter | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| condition := utils.GetCondition(stormService.Status.Conditions, orchestrationv1alpha1.StormServiceProgressing) | ||||||||||||||||||||||||||||||||
| if condition == nil || condition.LastUpdateTime == nil || condition.Reason == ProgressDeadlineExceededReason { | ||||||||||||||||||||||||||||||||
| return DefaultRequeueAfter | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| requeueAfter := condition.LastUpdateTime.Add(progressDeadline(stormService)).Sub(now) + time.Second | ||||||||||||||||||||||||||||||||
| if requeueAfter < time.Second { | ||||||||||||||||||||||||||||||||
| return time.Second | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if requeueAfter < DefaultRequeueAfter { | ||||||||||||||||||||||||||||||||
| return requeueAfter | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return DefaultRequeueAfter | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progressDeadlineSecondsshould be validated as a positive value. Right now the CRD/API only setsdefault: 600, but does not add a minimum, so users can create a StormService withprogressDeadlineSeconds: 0or a negative value. can we valid in webhook ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 that0and-1return KubernetesInvaliderrors.