[RayService] Skip update events without change

[sihanwang41]

Signed-off-by: Sihan Wang [email protected]

Why are these changes needed?

Ignore K8s reconcile when there is no config/label/annotation change

Related issue number

#28652

Checks

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

Local testing, all reconciles are under 2 seconds interval

[kevin85421]

It looks like we should add a RateLimiter rather than an event filter.

https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller#Options

[DmitriGekhtman]

Skipping metadata and status changes seems fine as well -- but are you sure you want to skip metadata changes?
See

kuberay/ray-operator/controllers/ray/raycluster_controller.go

Line 799 in 16fd58b

WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}, predicate.AnnotationChangedPredicate{})).

for what the RayCluster controller currently does, which is check for spec, labels, or annotations changes.

[DmitriGekhtman]

It looks like we should add a RateLimiter rather than an event filter.

https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller#Options

For later, I think we should consider this too.

[brucez-anyscale]

Skipping metadata and status changes seems fine as well -- but are you sure you want to skip metadata changes? See

kuberay/ray-operator/controllers/ray/raycluster_controller.go

Line 799 in 16fd58b

WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}, predicate.AnnotationChangedPredicate{})).

for what the RayCluster controller currently does, which is check for spec, labels, or annotations changes.

RayService is a self driven reconciler. It reconciles every 2 seconds.

[DmitriGekhtman]

RayService is a self driven reconciler. It reconciles every 2 seconds.

Right, reconciliation happens periodically by default anyways, but reconcilers should try to respond ASAP to relevant changes. I think we do use annotations in the KubeRay operator, so it makes sense to respond to changes to those. We might use labels in the future.

[DmitriGekhtman]

Anyways, my complaint is a nit for the reason that @brucez-anyscale points out -- reconciliation of metadata will happen either way, though it potentially could be delayed by a couple of seconds.
Also we don't expect users to update metadata very often if at all.
I'd support keeping the event filter as is -- up to you.

[kevin85421]

If RayService controller will reconcile every two seconds, this solution makes sense. As mentioned above, the downside is the long latency. One quick question, where do we specify the 2 seconds period (SyncPeriod?)? cc @brucez-anyscale

[brucez-anyscale]

If RayService controller will reconcile every two seconds, this solution makes sense. As mentioned above, the downside is the long latency. One quick question, where do we specify the 2 seconds period (SyncPeriod?)? cc @brucez-anyscale

@sihanwang41 updates the pr. So there should not be long latency issue.

2 second is here
ray-operator/controllers/ray/rayservice_controller.go:39
ServiceDefaultRequeueDuration = 2 * time.Second

[kevin85421]

@sihanwang41 updates the pr. So there should not be long latency issue.

It looks like the latency is still the same, but 2 seconds is okay for me. Resource events with the same generation are necessary for KubeRay because KubeRay is not an ideal operator (stateless & idempotent).

2 second is here ray-operator/controllers/ray/rayservice_controller.go:39 ServiceDefaultRequeueDuration = 2 * time.Second

Thank you for your reply!

[DmitriGekhtman]

Resource events with the same generation are necessary for KubeRay because KubeRay is not an ideal operator (stateless & idempotent).

I agree that the KubeRay operator's reconciliation is not 100% correct. We identified some of the gaps in the GitHub issues.

Why does that imply that that the reconciling multiple times with the same resource version is necessary?

[DmitriGekhtman]

I think we can merge this PR, since it solves the immediate issue in a reasonable-enough way.
We can revisit triggers for reconciliation, rate limiters, etc. later.

[kevin85421]

Resource events with the same generation are necessary for KubeRay because KubeRay is not an ideal operator (stateless & idempotent).

Why does that imply that reconciling multiple times with the same resource version is necessary?

In some cases, KubeRay needs various reconciliation iterations with same CR spec to reach the goal state.

Example1: Take #704 as an example, the extra Pod will be killed in the next reconciliation. However, there is no generation update between these two reconciliations.

Example2: Take RayJob as an example, if the first iteration creates RayCluster successfully but fails in a job launch, the second reconciliation needs to use HTTP client to create the job again.

We can think about this question from a high-level perspective. Replicated update resource events can be considered the same event only when the reconciliation logic is stateless. In other words, the operator will help Kubernetes reach the same goal state no matter the previous cluster state.

For example, we cannot discard (2) and (3) although they are the same function. On the other hand, we can ignore (5) and (6) because they are idempotent. We can imagine that KubeRay is a big non-idempotent function.

event 1. x += 1
event 2. x += 1
event 3. x += 1

--- 
event 4. x = 1
event 5. x = 1
event 6. x = 1

[DmitriGekhtman]

Example1: Take #704 as an example, the extra Pod will be killed in the next reconciliation. However, there is no generation update between these two reconciliations.

The reconciler will still respond to pod creation and deletion -- the event filter old affects response to RayCluster resource changes.

[DmitriGekhtman]

For the pod reconciliation issue, I think it's better if the operator reconciles less frequently since the problem is a stale cache. Reconciling less frequently would minimize the chances of read-after-write inconsistency.

Of course, the ideal solution is to actually fix the pod reconciliation logic.