Skip to content

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Jul 30, 2025

This reverts commit f6b4f17.

Why are these changes needed?

I have some concerns about the behavior of #3836. For example, we currently add the concept of "cron" into RayJob. This may have some issues:

  1. How does it work with schedulers like Kueue?
  2. The CRD status loses its observability benefits after the next job is submitted.
  3. This pattern doesn't follow "Separation of Concerns".
  4. The cleanup mechanism may become very complex.
  5. If users update the RayJob CR after it’s created, a lot of logic becomes quite complex.

We should consider either (1) introducing a new CRD and controller for RayCronJob if it’s worth it, or (2) implementing a lightweight periodic submission mechanism in the KubeRay API server.

Related issue number

Checks

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

@andrewsykim
Copy link
Member

andrewsykim commented Jul 30, 2025

We should consider either (1) introducing a new CRD and controller for RayCronJob if it’s worth it, or (2) implementing a lightweight periodic submission mechanism in the KubeRay API server.

We investigated whether adding a new CRD would be worth doing and decided against it as the cron scheduling logic is not that complex to add into the existing RayJob controller. Regarding 2), I don't think any new features in kuberay should require using KubeRay API Server

@andrewsykim
Copy link
Member

andrewsykim commented Jul 30, 2025

@kevin85421 let's setup a meeting with @DW-Han and discuss before we revert, there were a lot of considerations made in the initial PR that is worth sharing. There were a lot of discussions in the original PR and issue that I suggest you review for more context

@andrewsykim
Copy link
Member

Closing this PR for now to avoid accidental merge. Kai-Hsun and I have a 1:1 later today to discuss next steps and will revisit this later today.

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.

4 participants