RayCluster updates status frequently #1211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
kevin85421
force-pushed
the
ray-cluster-status-update
branch
from
16aa211 to
8558a55
Compare
|
cc @msumitjain would you mind reviewing this PR? Thanks! |
|
cc @anshulomar would you mind reviewing this PR? Thanks! |
gvspraveen
reviewed
Jul 3, 2023
| // status update will not be triggered. | ||
| // | ||
| // TODO (kevin85421): The field `ObservedGeneration` is not being well-maintained at the moment. In the future, | ||
| // this field should be used to determine whether to update this CR or not. |
There was a problem hiding this comment.
do we have GH issue for this ObservedGeneration fix?
There was a problem hiding this comment.
Good question. I will open an issue for it.
gvspraveen
reviewed
Jul 3, 2023
| oldStatus.MinWorkerReplicas, newStatus.MinWorkerReplicas, oldStatus.MaxWorkerReplicas, newStatus.MaxWorkerReplicas)) | ||
| return true | ||
| } | ||
| if !reflect.DeepEqual(oldStatus.Endpoints, newStatus.Endpoints) || !reflect.DeepEqual(oldStatus.Head, newStatus.Head) { |
There was a problem hiding this comment.
any specific reason to not include this as part of if statement in line 296?
There was a problem hiding this comment.
We can merge L290, L296, and L305 into a single if statement; however, the resulting log message will be very long and difficult for users to read.
gvspraveen
approved these changes
Jul 3, 2023
2 tasks
lowang-bh
pushed a commit
to lowang-bh/kuberay
that referenced
this pull request
Sep 24, 2023
RayCluster updates status frequently
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
This PR is similar to #1065. Without this PR, the RayCluster reconciler will update the CR's status in every reconciliation. Refer to the source code of ReplicaSet and StatefulSet. They will check the difference between the original status and the new status to determine whether update the status or not. Hence, this PR implements a similar function
inconsistentRayClusterStatusto determine whether update status or not.Related issue number
#1182
Checks
I conducted an experiment similar to [DO NOT MERGE][Experiment] RayCluster updates status frequently #1214. In [DO NOT MERGE][Experiment] RayCluster updates status frequently #1214, the status update is called 8 times in 10 minutes. Let the number of reconciliations be N.
ctrl.Result{RequeueAfter: time.Duration(requeueAfterSeconds) * time.Second}(with a default of 300 seconds). => O(N)The CR status update is called 3 times in 10 minutes.