-
Notifications
You must be signed in to change notification settings - Fork 359
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
[Refactor][RayCluster] Unify status update to single place #2249
[Refactor][RayCluster] Unify status update to single place #2249
Conversation
157ea19
to
fad4424
Compare
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.
Looks good! Would you mind adding a test in raycluster_controller_test.go
to check the following:
- RayCluster becomes ready.
- RayCluster becomes failed.
- RayCluster returns to ready again.
// reconcileServeService | ||
func(ctx context.Context, instance *rayv1.RayCluster) error { | ||
// Only reconcile the K8s service for Ray Serve when the "ray.io/enable-serve-service" annotation is set to true. | ||
if enableServeServiceValue, exist := instance.Annotations[utils.EnableServeServiceKey]; exist && enableServeServiceValue == utils.EnableServeServiceTrue { |
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.
How about checking this inside reconcileServeService
?
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.
Done
} | ||
return nil | ||
}, | ||
// reconcilePods |
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.
How about using r.reconcilePods
directly? We can remove the event for now.
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.
Done
|
||
for _, fn := range reconcileFuncs { | ||
if reconcileErr = fn(ctx, instance); reconcileErr != nil { | ||
break |
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.
Add a log for reconcileErr
?
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.
Done
Resolves: ray-project#2235 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Resolves: ray-project#2235 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Resolves: ray-project#2235 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
fad4424
to
0719b3a
Compare
Should these tests be added in this PR? This PR is for refactoring purpose. It might be better to add these tests in a separate PR. |
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.
Why are these changes needed?
See the description in the corresponding issue for details.
Implementation details of this PR:
updateClusterState
andupdateClusterReason
functions are integrated into a single functionupdateRayClusterStatus
.inconsistentRayClusterStatus
checking is moved intoupdateRayClusterStatus
, so that the status will only be updated if it is inconsistent.calculateStatus
function takes one more argument calledreconcileErr
. If it is a non-nil value, then theStatus.State
will be set toFailed
andStatus.Reason
will be set to the string representation of the error.reconcileErr
variable for processing later.Related issue number
Resolves: #2235
Checks