Skip to content
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

[Feature][Hotfix] Add observedGeneration to the status of CRDs #979

Merged
merged 6 commits into from
Mar 24, 2023

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Mar 22, 2023

Why are these changes needed?

observedGeneration is a common pattern in Kubernetes to determine whether the current status is out-of-date or not.

  • Good article: Kubernetes operator best practices: Implementing observedGeneration

  • StatefulSet uses ObservedGeneration to determine whether update status or not.

    // inconsistentStatus returns true if the ObservedGeneration of status is greater than set's
    // Generation or if any of the status's fields do not match those of set's status.
    func inconsistentStatus(set *apps.StatefulSet, status *apps.StatefulSetStatus) bool {
        return status.ObservedGeneration > set.Status.ObservedGeneration ||
      	  status.Replicas != set.Status.Replicas ||
      	  status.CurrentReplicas != set.Status.CurrentReplicas ||
      	  status.ReadyReplicas != set.Status.ReadyReplicas ||
      	  status.UpdatedReplicas != set.Status.UpdatedReplicas ||
      	  status.CurrentRevision != set.Status.CurrentRevision ||
      	  status.AvailableReplicas != set.Status.AvailableReplicas ||
      	  status.UpdateRevision != set.Status.UpdateRevision
    }

However, this PR does not use observedGeneration to do anything because:

  • Currently, KubeRay lacks a well-defined strategy for updating the status of its resources. Furthermore, it's possible for multiple status updates to occur within a single reconciliation loop, which can lead to problematic patterns and outcomes.
  • As a result, merging a pull request that includes any changes related to status updates shortly before the release of v0.5.0 is very risky.

The main motivation for this PR is to unblock Shopify. Other works will be included in the release v0.6.0.

ObservedGeneration definition at this moment

  • ObservedGeneration in this PR only promises that ObservedGeneration <= Generation.
  • I did not promise any other behavior in this PR because there are a lot of code paths that will update the status and I do not check every path.

Test

  • We currently only have a test for RayCluster because we do not have tests with fake clients for RayJob and RayService at this moment. These should be done in the next release which we plan to focus on the stability of RayJob and RayService.

Related issue number

Hotfix for #916

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(
  • RayCluster

Screen Shot 2023-03-22 at 4 27 39 PM

  • RayJob

Screen Shot 2023-03-22 at 4 46 21 PM

  • RayService

Screen Shot 2023-03-22 at 4 36 52 PM

@kevin85421 kevin85421 changed the title [WIP] Observed generation [Feature] Add observedGeneration to the status of CRDs Mar 22, 2023
@kevin85421 kevin85421 changed the title [Feature] Add observedGeneration to the status of CRDs [Feature][Hotfix] Add observedGeneration to the status of CRDs Mar 23, 2023
@kevin85421 kevin85421 marked this pull request as ready for review March 23, 2023 00:20
@kevin85421
Copy link
Member Author

cc @gariepyalex

Copy link
Contributor

@shrekris-anyscale shrekris-anyscale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, other than a few nits, this change looks good to me!

Out of curiosity, is the ObjectMeta.Generation set by the user or is it autogenerated by the controller?

ray-operator/controllers/ray/raycluster_controller.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/rayservice_controller.go Outdated Show resolved Hide resolved
kevin85421 and others added 2 commits March 22, 2023 21:01
Co-authored-by: shrekris-anyscale <92341594+shrekris-anyscale@users.noreply.github.com>
Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
Co-authored-by: shrekris-anyscale <92341594+shrekris-anyscale@users.noreply.github.com>
Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
@kevin85421
Copy link
Member Author

Out of curiosity, is the ObjectMeta.Generation set by the user or is it autogenerated by the controller?

ObjectMeta.Generation is maintained by Kubernetes's APIServer. The field is a sequence number representing a specific generation of the desired state. Set by the system and monotonically increasing, per-resource. See this for more details.

@kevin85421 kevin85421 merged commit 5ca90b3 into ray-project:master Mar 24, 2023
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants