feat: update RayCluster `.status.reason` field with pod creation error #639

davidxia · 2022-10-17T19:19:51Z

Why are these changes needed?

Makes RayCluster errors related to Pod creation easier more apparent to user.

example before if Pod can't be created due to ResourceQuota exceeded

kubectl get rayclusters dxia-test2 -o yaml
...
status:
  state: failed

kubectl describe rayclusters dxia-test2
...
Status:
  State:   failed

example after if Pod can't be created due to ResourceQuota exceeded

kubectl get rayclusters dxia-test2 -o yaml
...
status:
  reason: 'pods "dxia-test2-head-lbvdc" is forbidden: exceeded quota: quota, requested:
    limits.cpu=15,requests.cpu=15, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10'
  state: failed

kubectl describe rayclusters dxia-test2
...
Status:
  Reason:  pods "dxia-test2-head-9mdm5" is forbidden: exceeded quota: quota, requested: limits.cpu=15,requests.cpu=15, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  State:   failed

Related issue number

fixes #603

Checks

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

davidxia · 2022-10-17T20:11:34Z

This requires an update to the RayCluster CRD which I know you like to avoid. The only other way is to set this info as an annotation or label. I think this case is worth updating the CRD to have a new field. Just have to get it right the first time. :) Lmk what you think.

DmitriGekhtman · 2022-10-17T20:28:39Z

This requires an update to the RayCluster CRD which I know you like to avoid. The only other way is to set this info as an annotation or label. I think this case is worth updating the CRD to have a new field. Just have to get it right the first time. :) Lmk what you think.

Addition of optional fields is safe for backwards compatibility.

davidxia · 2022-10-18T03:25:33Z

For comparison, here's the behavior of K8s Deployments when it can't create Pods due to exhausted ResourceQuota.

Listing Deployments shows whether it's READY with actual/desired Pods.

kubectl get deployments
NAME               READY   UP-TO-DATE   AVAILABLE   AGE
nginx-deployment   0/1     0            0           11s

Getting the YAML shows a list of conditions with reason and message.

kubectl get deployments nginx-deployment -o yaml
...
status:
  conditions:
  - lastTransitionTime: "2022-10-18T03:16:51Z"
    lastUpdateTime: "2022-10-18T03:16:51Z"
    message: Created new replica set "nginx-deployment-79499496f5"
    reason: NewReplicaSetCreated
    status: "True"
    type: Progressing
  - lastTransitionTime: "2022-10-18T03:16:51Z"
    lastUpdateTime: "2022-10-18T03:16:51Z"
    message: Deployment does not have minimum availability.
    reason: MinimumReplicasUnavailable
    status: "False"
    type: Available
  - lastTransitionTime: "2022-10-18T03:16:51Z"
    lastUpdateTime: "2022-10-18T03:16:51Z"
    message: 'pods "nginx-deployment-79499496f5-ndzdk" is forbidden: exceeded quota:
      quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60,
      limited: limits.cpu=10,requests.cpu=10'
    reason: FailedCreate
    status: "True"
    type: ReplicaFailure
  observedGeneration: 1
  unavailableReplicas: 1

Similar detailed info for underlying ReplicaSet.

kubectl get rs
NAME                          DESIRED   CURRENT   READY   AGE
nginx-deployment-79499496f5   1         0         0       49s

kubectl get rs nginx-deployment-79499496f5 -o yaml
...
status:
  conditions:
  - lastTransitionTime: "2022-10-18T03:16:51Z"
    message: 'pods "nginx-deployment-79499496f5-ndzdk" is forbidden: exceeded quota:
      quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60,
      limited: limits.cpu=10,requests.cpu=10'
    reason: FailedCreate
    status: "True"
    type: ReplicaFailure
  observedGeneration: 1
  replicas: 0

Describing ReplicaSet shows events about root cause.

kubectl describe rs nginx-deployment-79499496f5
...
Conditions:
  Type             Status  Reason
  ----             ------  ------
  ReplicaFailure   True    FailedCreate
Events:
  Type     Reason        Age                From                   Message
  ----     ------        ----               ----                   -------
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-ndzdk" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-df9qh" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-bdcjs" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-rlkgq" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-qz62l" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-bch2m" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  63s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-td4ml" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  62s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-4nvkp" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  62s                replicaset-controller  Error creating: pods "nginx-deployment-79499496f5-k98jh" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10
  Warning  FailedCreate  22s (x5 over 60s)  replicaset-controller  (combined from similar events): Error creating: pods "nginx-deployment-79499496f5-z2bb8" is forbidden: exceeded quota: quota, requested: limits.cpu=11,requests.cpu=11, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10

Might be nice to add a READY column with actual/desired Pods for RayCluster and also events?

DmitriGekhtman · 2022-10-18T20:10:04Z

I think a RayCluster is most like a ReplicaSet. Maybe we could do Desired, Current, Ready with a status.conditions list?
IMO, status.reason is too course to describe individual worker failure.
Thoughts, @Jeffwan @akanso?

Jeffwan · 2022-10-18T16:57:06Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -214,6 +214,9 @@ func (r *RayClusterReconciler) rayClusterReconcile(request ctrl.Request, instanc
 		if updateErr := r.updateClusterState(instance, rayiov1alpha1.Failed); updateErr != nil {
 			r.Log.Error(updateErr, "RayCluster update state error", "cluster name", request.Name)
 		}
+		if updateErr := r.updateClusterReason(instance, err.Error()); updateErr != nil {
+			r.Log.Error(updateErr, "RayCluster update reason error", "cluster name", request.Name)


This is a good start. we may want to expose more for other state.

Jeffwan · 2022-10-20T21:36:06Z

I think a RayCluster is most like a ReplicaSet. Maybe we could do Desired, Current, Ready with a status.conditions list?
IMO, status.reason is too course to describe individual worker failure.
Thoughts, @Jeffwan @akanso?

Agree, conditions would be great to have. status can not tell the user how entire state machine but condition does.

Jeffwan · 2022-10-20T21:38:21Z

@davidxia Is this PR ready? If so, please remove the WIP status.

davidxia · 2022-10-24T13:34:53Z

@Jeffwan would you like me to implement conditions in this PR instead of status or do you want conditions to be in addition to status and implemented in a future PR?

Jeffwan · 2022-10-31T04:31:25Z

@davidxia I think future PRs sounds good. I think you mark the PR in WIP. I was not sure whether you like to implement it in same PR or not?

DmitriGekhtman · 2022-10-31T16:44:54Z

This is a good start. Before merging, it would be great to add a simple unit test.

As a follow-up for a different PR, we could emit an event with the RayCluster as subject -- I think that would get the error into the kubectl describe output.

davidxia · 2022-10-31T18:53:58Z

Thanks, I'll add some test(s) and mark as ready for review. Any tips for how to make the "Go-build-and-test / Compatibility Test - 2.0.0 (pull_request)" pass?

DmitriGekhtman · 2022-10-31T19:23:50Z

Any tips for how to make the "Go-build-and-test / Compatibility Test - 2.0.0 (pull_request)" pass?

I'd ignore it. We need to smooth out some tests of experimental features.

davidxia · 2022-11-01T15:24:13Z

Thanks, added a unit test and marked as ready for review. (follow up issue "ray-operator emit RayCluster events and .status.conditions[] for Pod creation errors")

I tested manually on my K8s cluster (deploy new operator image, apply a ResourceQuota that's already exceeded, try to create a RayCluster in that namespace) and noticed the following difference in behavior. With v0.3.0 release, the reconcile loop retried once every second and backs off pretty quickly if there are errors. With this change, for some reason the operator tries to reconcile a failed RayCluster every 100ms with no backoff.

See the before and after logs here. Any idea why this is happening and/or if it's an issue? I can't see what changes I made here that would alter this behavior.

DmitriGekhtman · 2022-11-03T22:19:58Z

With v0.3.0 release, the reconcile loop retried once every second and backs off pretty quickly if there are errors. With this change, for some reason the operator tries to reconcile a failed RayCluster every 100ms with no backoff.

Do you see the problematic behavior in current master? Is it possible another commit caused it?

DmitriGekhtman · 2022-11-04T01:31:11Z

@kevin85421 if you have some time, could you check to see if the undesirable reconciler behavior described a couple of comments up occurs in master?
cc @sihanwang41

DmitriGekhtman · 2022-11-04T17:08:19Z

I've opened an issue to track investigation of the overactive reconciliation loop:
#686

DmitriGekhtman · 2022-11-07T04:28:06Z

I've opened an issue to track investigation of the overactive reconciliation loop: #686

Seems master is fine. I hope we can help debug the PR branch so that it can be merged.

davidxia · 2022-11-10T00:31:11Z

@DmitriGekhtman after rebasing this PR on latest master, building image, and testing the new operator on my cluster, I can't repro the original tight reconcile loop. Now it behaves as we expect. 🤷‍♂️

davidxia · 2022-11-10T00:50:34Z

actually nvm, I can still repro it when I update the RayCluster CRD. Looking more...

davidxia · 2022-11-10T13:28:04Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -214,6 +214,9 @@ func (r *RayClusterReconciler) rayClusterReconcile(request ctrl.Request, instanc
 		if updateErr := r.updateClusterState(instance, rayiov1alpha1.Failed); updateErr != nil {
 			r.Log.Error(updateErr, "RayCluster update state error", "cluster name", request.Name)
 		}
+		if updateErr := r.updateClusterReason(instance, err.Error()); updateErr != nil {


@DmitriGekhtman The reconcile loop doesn't cease because the err.Error() string here is always different. It begins with pods "POD_NAME" is forbidden: exceeded quota: quota... and POD_NAME keeps changing. So updating the .status.reason queues the RayCluster for another reconciliation which updates .status.reason which queues it again, on and on.

I'll make the string here stable which should fix.

Ah, that's interesting! Outputting the same error sgtm.

@DmitriGekhtman this is ugly and brittle. Is there a way to have the reconcile loop ignore changes to .status.reason or perhaps .status altogether? I want to do something like this. I think relevant code is here and/or here? But not sure and need help.

Hm, maybe we could just set the reason to something like "Pod reconcile failed" and then display the full error by emitting an event, which would be easily accessible via kubectl describe.

Sgtm, I’ll take a look soon

Let's use Or to also watch for changes to Labels and Annotations.

CR annotations are currently taken into account by the operator.
Labels are not taken into account for now, but they might be later.

The con of ignoring status changes is that extraneous status changes are not quickly fixed. I think that's a minor issue, though.

thanks, updated. Also included emitting event since it's small and straightforward. Lmk if there's any good examples of how to test event or label/annotation update-trigger-reconciliation logic.

Tested on my cluster. Updating labels or annotations triggers reconciliation as expected. Events also show up as expected. See gist

Nice, thanks!

fixes ray-project#603 ### example before if Pod can't be created due to ResourceQuota exceeded ``` kubectl get rayclusters dxia-test2 -o yaml ... status: state: failed kubectl describe rayclusters dxia-test2 ... Status: State: failed ``` ### example after if Pod can't be created due to ResourceQuota exceeded ``` kubectl get rayclusters dxia-test2 -o yaml ... status: reason: 'pods "dxia-test2-head-lbvdc" is forbidden: exceeded quota: quota, requested: limits.cpu=15,requests.cpu=15, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10' state: failed kubectl describe rayclusters dxia-test2 ... Status: Reason: pods "dxia-test2-head-9mdm5" is forbidden: exceeded quota: quota, requested: limits.cpu=15,requests.cpu=15, used: limits.cpu=60,requests.cpu=60, limited: limits.cpu=10,requests.cpu=10 State: failed ```

…sn't change by adding `predicate.GenerationChangedPredicate` to reconciler. > This predicate will skip update events that have no change in the object's > metadata.generation field. The metadata.generation field of an object is > incremented by the API server when writes are made to the spec field of an > object. This allows a controller to ignore update events where the spec is > unchanged, and only the metadata and/or status fields are changed. https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/predicate?utm_source=godoc#GenerationChangedPredicate Without this change, the controller gets stuck in a tight loop repeatedly updating the same RayCluster when a ResourceQuota is exhausted. The controller updates the RayCluster.status, gets an event about the forbidden Pod with a different name, updates the .status, gets an event, on and on.

and emit event on RayCluster for Pod reconciliation errors.

DmitriGekhtman

Thanks, this is a great improvement!

davidxia · 2022-11-24T20:17:52Z

Lmk if this can be included in upcoming 0.4.0 release.

DmitriGekhtman · 2022-11-24T23:01:12Z

Lmk if this can be included in upcoming 0.4.0 release.

It will probably be included.
Let's wait a business day or two for more feedback, though.

davidxia · 2022-11-29T14:37:50Z

Thanks so much!

ray-project#639 accidentally applied event filters for child resources Pods and Services. This change does not filter Pod or Service related events. This means Pod updates will trigger RayCluster reconciliation. closes ray-project#872

#639 accidentally applied event filters for child resources Pods and Services. This change does not filter Pod or Service related events. This means Pod updates will trigger RayCluster reconciliation.

ray-project#639) * feat: update RayCluster `.status.reason` field with pod creation error Makes RayCluster errors related to Pod creation easier more apparent to user.

ray-project#639 accidentally applied event filters for child resources Pods and Services. This change does not filter Pod or Service related events. This means Pod updates will trigger RayCluster reconciliation.

davidxia force-pushed the dxia/patch12 branch 2 times, most recently from 70d2751 to 841bf35 Compare October 17, 2022 19:56

DmitriGekhtman requested review from DmitriGekhtman, Jeffwan and akanso October 17, 2022 19:56

davidxia force-pushed the dxia/patch12 branch from 841bf35 to ea32961 Compare October 17, 2022 20:09

davidxia force-pushed the dxia/patch12 branch 2 times, most recently from 66c75a6 to ad2ecac Compare October 17, 2022 20:27

Jeffwan reviewed Oct 20, 2022

View reviewed changes

davidxia force-pushed the dxia/patch12 branch 2 times, most recently from ac82334 to 0ec6d6a Compare November 1, 2022 15:15

davidxia marked this pull request as ready for review November 1, 2022 15:19

davidxia mentioned this pull request Nov 1, 2022

ray-operator emit RayCluster events and .status.conditions[] for Pod creation errors #670

Closed

DmitriGekhtman mentioned this pull request Nov 4, 2022

[Bug][Investigation] Examine reconciliation backoff when there is an issue creating pods #686

Closed

2 tasks

davidxia force-pushed the dxia/patch12 branch from 0ec6d6a to 966ac67 Compare November 9, 2022 23:37

davidxia commented Nov 10, 2022

View reviewed changes

davidxia force-pushed the dxia/patch12 branch 2 times, most recently from 6339626 to ce48035 Compare November 10, 2022 17:31

davidxia force-pushed the dxia/patch12 branch from 42b95a3 to 720f100 Compare November 23, 2022 18:56

davidxia added 2 commits November 24, 2022 09:19

davidxia force-pushed the dxia/patch12 branch from 720f100 to beba570 Compare November 24, 2022 14:19

fix: watch for RayCluster label and annotation updates

6156872

and emit event on RayCluster for Pod reconciliation errors.

DmitriGekhtman approved these changes Nov 24, 2022

View reviewed changes

DmitriGekhtman requested review from Jeffwan and kevin85421 November 24, 2022 18:28

DmitriGekhtman merged commit 3a64712 into ray-project:master Nov 29, 2022

davidxia deleted the dxia/patch12 branch November 29, 2022 14:37

davidxia mentioned this pull request Jan 23, 2023

fix: only filter RayCluster events for reconciliation #882

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update RayCluster `.status.reason` field with pod creation error #639

feat: update RayCluster `.status.reason` field with pod creation error #639

davidxia commented Oct 17, 2022 •

edited

Loading

davidxia commented Oct 17, 2022

DmitriGekhtman commented Oct 17, 2022

davidxia commented Oct 18, 2022

DmitriGekhtman commented Oct 18, 2022

Jeffwan Oct 18, 2022

Jeffwan commented Oct 20, 2022

Jeffwan commented Oct 20, 2022

davidxia commented Oct 24, 2022

Jeffwan commented Oct 31, 2022

DmitriGekhtman commented Oct 31, 2022

davidxia commented Oct 31, 2022

DmitriGekhtman commented Oct 31, 2022

davidxia commented Nov 1, 2022 •

edited

Loading

DmitriGekhtman commented Nov 3, 2022

DmitriGekhtman commented Nov 4, 2022 •

edited

Loading

DmitriGekhtman commented Nov 4, 2022

DmitriGekhtman commented Nov 7, 2022

davidxia commented Nov 10, 2022

davidxia commented Nov 10, 2022

davidxia Nov 10, 2022

DmitriGekhtman Nov 10, 2022

davidxia Nov 10, 2022

DmitriGekhtman Nov 10, 2022

davidxia Nov 11, 2022

DmitriGekhtman Nov 24, 2022

DmitriGekhtman Nov 24, 2022

davidxia Nov 24, 2022

davidxia Nov 24, 2022

DmitriGekhtman Nov 24, 2022

DmitriGekhtman left a comment

davidxia commented Nov 24, 2022

DmitriGekhtman commented Nov 24, 2022

davidxia commented Nov 29, 2022

feat: update RayCluster .status.reason field with pod creation error #639

feat: update RayCluster .status.reason field with pod creation error #639

Conversation

davidxia commented Oct 17, 2022 • edited Loading

Why are these changes needed?

example before if Pod can't be created due to ResourceQuota exceeded

example after if Pod can't be created due to ResourceQuota exceeded

Related issue number

Checks

davidxia commented Oct 17, 2022

DmitriGekhtman commented Oct 17, 2022

davidxia commented Oct 18, 2022

DmitriGekhtman commented Oct 18, 2022

Choose a reason for hiding this comment

Jeffwan commented Oct 20, 2022

Jeffwan commented Oct 20, 2022

davidxia commented Oct 24, 2022

Jeffwan commented Oct 31, 2022

DmitriGekhtman commented Oct 31, 2022

davidxia commented Oct 31, 2022

DmitriGekhtman commented Oct 31, 2022

davidxia commented Nov 1, 2022 • edited Loading

DmitriGekhtman commented Nov 3, 2022

DmitriGekhtman commented Nov 4, 2022 • edited Loading

DmitriGekhtman commented Nov 4, 2022

DmitriGekhtman commented Nov 7, 2022

davidxia commented Nov 10, 2022

davidxia commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriGekhtman left a comment

Choose a reason for hiding this comment

davidxia commented Nov 24, 2022

DmitriGekhtman commented Nov 24, 2022

davidxia commented Nov 29, 2022

feat: update RayCluster `.status.reason` field with pod creation error #639

feat: update RayCluster `.status.reason` field with pod creation error #639

davidxia commented Oct 17, 2022 •

edited

Loading

davidxia commented Nov 1, 2022 •

edited

Loading

DmitriGekhtman commented Nov 4, 2022 •

edited

Loading