Repository navigation
[SPARK-60057] Add suspendReason to Suspended states - #954
dongjoon-hyun wants to merge 1 commit into
Conversation
|
cc @viirya . This is the PR based on your previous review comment. |
viirya
left a comment
There was a problem hiding this comment.
Thanks for following up on the review of #952. Using a structured suspension reason removes the dependency on user-facing message wording and makes the application and cluster paths consistent.
I checked the interaction with status persistence, resource cleanup, and resume. The suspension entry points set the reason, the cluster’s stuck-pod state preserves it, and resuming creates a new Submitted state without carrying the reason forward. This also makes removing the cluster’s history scan reasonable.
Dropping the message-prefix fallback seems reasonable for the released-version upgrade path: Suspended is new in 1.1.0, and the migration guide already requires updating the CRDs before upgrading the operator. The documented behavior for a missing reason is consistent with the implementation.
I found no blocking issues. One optional integration-test suggestion below would strengthen coverage of the persisted field across operator restarts.
I attempted the five affected test classes, but dependency resolution failed because Maven Central was unreachable, and the offline cache was incomplete. I could not independently verify the test results; CI should pass before merging.
| } | ||
|
|
||
| @Test | ||
| void evictionIsToldByTheSuspendReasonRatherThanByTheMessage() { |
There was a problem hiding this comment.
Non-blocking: could we extend the existing Kueue e2e coverage to verify this across an operator restart? After an eviction reaches Suspended, assert that the API server retains suspendReason: KueueEviction, restart the operator, and verify that a deactivated Workload remains held or an unexpired requeue backoff is still honored.
The JSON round-trip and reconciliation tests cover the individual pieces well. This would additionally verify the CRD persistence boundary and recovery without the in-memory status cache, which matter now that this field determines whether keepKueueWorkload() runs.
There was a problem hiding this comment.
Thank you for the review and the suggestion. Verifying the persisted field across an operator restart in E2E makes sense. I'll add that E2E coverage separately in a follow-up, since it needs to restart the operator in the Kueue scenarios.
suspendReason to Suspended states`suspendReason to Suspended states
|
Merged to main |
What changes were proposed in this pull request?
This PR adds
status.currentState.suspendReason(SpecSuspendorKueueEviction), which is set onSuspendedstates, and uses it inAppSuspendStepandClusterSuspendStepinstead of the prefix of the state message.SuspendReasonand the nullable field inBaseState. The existing 3-arg constructor is kept.isSuspendedByEvictiononly checks the reason of the current state. The cluster's walk through the history is removed. The stuck-pods state keeps the reason of the state before it.Suspendedstate without a reason is treated asSpecSuspend.v1CRDs (v1beta1is frozen), and document the field indocs/spark_custom_resources.md.Why are the changes needed?
The message prefix became a control protocol, as pointed out in the review of #952. If the wording changes, a resource suspended by an eviction by an older version is taken for a manual suspend, so
keepKueueWorkload()is skipped and the remaining requeue backoff is ignored.No message-prefix fallback is needed.
Suspendedis not in 1.0.0, and the migration guide already requires replacing the CRDs for 1.1.0, which ships the new field.Does this PR introduce any user-facing change?
No for the released versions.
suspendReasonis a new field within the unreleased 1.1.0 changes, and the state messages are unchanged.How was this patch tested?
Pass the CIs.
gradle buildpasses locally, with new and updated tests inApplicationStatusTest,ClusterStatusTest,AppSuspendStepTest,ClusterSuspendStepTestandClusterInitStepTest.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 5.5