[ray-operator] Update RayCluster status on validation failure#4975
[ray-operator] Update RayCluster status on validation failure#4975yoonseo-han wants to merge 2 commits into
Conversation
Signed-off-by: Yoonseo Han <yooncer00@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0f5f586c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Type: string(rayv1.RayClusterProvisioned), | ||
| Status: metav1.ConditionFalse, | ||
| Reason: rayv1.RayClusterValidationFailed, |
There was a problem hiding this comment.
When an already-provisioned RayCluster is later edited to an invalid spec, this validation path unconditionally overwrites RayClusterProvisioned=True with False/ValidationFailed. That condition is documented and tested as sticky after the cluster becomes ready for the first time, so clients watching it will see a provisioned cluster regress simply because a new generation failed validation; consider preserving an existing true RayClusterProvisioned and reporting validation failure separately.
Useful? React with 👍 / 👎.
| if features.Enabled(features.RayClusterStatusConditions) { | ||
| meta.SetStatusCondition(&newInstance.Status.Conditions, metav1.Condition{ |
There was a problem hiding this comment.
When RayClusterStatusConditions is disabled, this block adds no condition, leaving ObservedGeneration and LastUpdateTime as the only status changes. updateRayClusterStatus intentionally ignores diffs that only touch those two fields, so invalid RayClusters in that supported feature-gate configuration still return without any status update and observedGeneration remains stale.
Useful? React with 👍 / 👎.
|
|
||
| r := &RayClusterReconciler{ | ||
| Client: fakeClient, | ||
| Recorder: &record.FakeRecorder{}, |
There was a problem hiding this comment.
This new test path calls handleRayClusterValidationError, which records a Kubernetes event before updating status. A zero-value record.FakeRecorder has no Events channel, so Eventf blocks when it tries to send the event; use record.NewFakeRecorder(...) here (and in the analogous new reconcile test) so the validation tests can complete instead of hanging until the test timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 6d36d2a. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d36d2a43c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| meta.SetStatusCondition(&newInstance.Status.Conditions, metav1.Condition{ | ||
| Type: string(rayv1.RayClusterValid), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: rayv1.RayClusterSpecValid, | ||
| Message: "RayCluster metadata and spec are valid", | ||
| }) |
There was a problem hiding this comment.
If a user fixes a previously invalid RayCluster but this same reconcile later hits an operational error (for example while creating a Pod or Service), this block is skipped and the old Valid=False/ValidationFailed condition is preserved even though metadata/spec validation already passed. Consumers can continue to see the current valid generation as invalid until a later fully clean reconcile, so set the Valid condition based on validation success independently of reconcileErr.
Useful? React with 👍 / 👎.
6d36d2a to
01fad32
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01fad32ba1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Type: string(rayv1.RayClusterValid), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: rayv1.RayClusterSpecValid, | ||
| Message: "RayCluster metadata and spec are valid", |
There was a problem hiding this comment.
When a previously invalid RayCluster is corrected, this call replaces Valid=False with Valid=True but leaves the condition's ObservedGeneration at the zero value because the literal omits it. Consumers that follow condition generations cannot tell that the validation pass applies to the edited spec, unlike the validation-failure path above which sets the current generation; set it to newInstance.Generation when marking validation success.
Useful? React with 👍 / 👎.
Signed-off-by: Yoonseo Han <yooncer00@gmail.com>
01fad32 to
7e5b17e
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed feedback based on AI reveiw
|
1 participant
Why are these changes needed?
When RayCluster metadata, spec, or upgrade strategy validation fails, the operator only logs an error and emits a Kubernetes Event, then returns early without updating .status.
As a result, status.observedGeneration can stay stale, and there is no condition indicating validation failed unlike RayJob and RayService
This change updates RayCluster status immediately on user input validation failure:
-> Sets
observedGeneration,LastUpdateTime, andRayClusterProvisioned=Falsewithreason=ValidationFailed, following the RayService pattern.Related issue number
Closes #4974
Checks