feat(evalhub): emit Kubernetes events for operator-detected evaluation failures (RHAI-278) - #846
Conversation
…lHub (#842) * chore(rbac): add events:create and jobs:patch/get permissions for EvalHub ServiceAccount Add two new ClusterRoles for the EvalHub ServiceAccount: - evalhub-events: grants events create, so the EvalHub server can emit Kubernetes Events against backing Job resources on evaluation lifecycle transitions (EvaluationStarted, EvaluationCompleted, EvaluationFailed, EvaluationThresholdViolated) - evalhub-jobs-writer extended with get and patch verbs, so the server can read-modify-patch the trustyai.opendatahub.io/evaluation-phase label on running Jobs Both new ClusterRoleBindings are added to the OPA policy allowlist. No functional code changes; this is the RBAC prerequisite for RHAI-277. * fix(rbac): add patch verb to evalhub-events ClusterRole for EventRecorder deduplication
* feat(evalhub): emit events for job infrastructure failures Patch evaluation-phase=Failed label and evaluation-status annotation on Job resources. Include deduplication logic to skip event emission if the EvalHub server has already marked the failure. * fix(evalhub): emit EvaluationFailed event only after successful EvalHub POST
…emission (RHOAIENG-80114) (#845) * feat(evalhub): emit events for job infrastructure failures Patch evaluation-phase=Failed label and evaluation-status annotation on Job resources. Include deduplication logic to skip event emission if the EvalHub server has already marked the failure. * fix(evalhub): emit EvaluationFailed event only after successful EvalHub POST * test(evalhub): add lifecycle unit tests for failure reconciler event emission (RHOAIENG-80114) Cover the full Reconcile() path for EvalHubEvaluationJobFailureReconciler and EvalHubEvaluationFailedKueueWorkloadsReconciler using fake.Client and record.FakeRecorder. Tests use a local httptest.Server to absorb EvalHub POST calls and an interceptor to capture the evaluation-phase=Failed label patch before job deletion. Cases added: - dedup: server already set evaluation-phase=Failed → no operator event - OOM-killed adapter container → EvaluationFailed event + label patch + job cleanup - ErrImagePull on init container → EvaluationFailed event + label patch + job cleanup - Kueue QuotaReserved=Inadmissible → EvaluationFailed event + job label + workload annotation
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEvalHub failure reconcilers now emit Kubernetes warning Events, mark failed Jobs, prevent duplicate failure handling, and annotate completed Kueue Workloads. Kustomize and policy RBAC configuration grants the required Event and Job permissions. ChangesEvalHub failure reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FailedJob
participant EvalHubFailureReconciler
participant EvalHub
participant KubernetesAPI
FailedJob->>EvalHubFailureReconciler: report detected failure
EvalHubFailureReconciler->>EvalHub: POST evaluation failure
EvalHub-->>EvalHubFailureReconciler: successful response
EvalHubFailureReconciler->>KubernetesAPI: emit EvaluationFailed Event
EvalHubFailureReconciler->>KubernetesAPI: patch failed Job metadata
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/evalhub/evaluation_job_failure_reconciler.go (1)
433-449: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist the successful report state before completing reconciliation.
A transient Job patch failure leaves the two reconcilers in inconsistent states. In the Job path, the retry repeats the EvalHub POST and emits another Event because
annotationFailurePendingremains set. In the Kueue path, the Workload is marked reported even though the Job never receivesevaluation-phase=Failedorevaluation-status.
controllers/evalhub/evaluation_job_failure_reconciler.go#L433-L449: add a durable post-success state that prevents repeated POSTs and Events when the promotion patch fails.controllers/evalhub/evaluation_failed_kueue_workloads_reconciler.go#L338-L345: retry the Job metadata update and do not setannotationKueueFailedWorkloadEventReporteduntil that update succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/evalhub/evaluation_job_failure_reconciler.go` around lines 433 - 449, In controllers/evalhub/evaluation_job_failure_reconciler.go:433-449, make the successful EvalHub report durable before reconciliation completes so a failed promotion patch prevents repeated POSTs and Events; update the retry path to recognize that durable state and avoid reposting. In controllers/evalhub/evaluation_failed_kueue_workloads_reconciler.go:338-345, retry the Job metadata update and set annotationKueueFailedWorkloadEventReported only after that update succeeds, keeping the Workload state consistent with the Job.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@controllers/evalhub/evaluation_job_failure_reconciler.go`:
- Around line 433-449: In
controllers/evalhub/evaluation_job_failure_reconciler.go:433-449, make the
successful EvalHub report durable before reconciliation completes so a failed
promotion patch prevents repeated POSTs and Events; update the retry path to
recognize that durable state and avoid reposting. In
controllers/evalhub/evaluation_failed_kueue_workloads_reconciler.go:338-345,
retry the Job metadata update and set annotationKueueFailedWorkloadEventReported
only after that update succeeds, keeping the Workload state consistent with the
Job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e344f6d3-07d6-4a18-ab7d-9b43c0b17c22
📒 Files selected for processing (9)
config/components/evalhub/kustomization.yamlconfig/components/evalhub/rbac/evalhub_events_binding.yamlconfig/components/evalhub/rbac/evalhub_events_role.yamlconfig/components/evalhub/rbac/evalhub_jobs_writer_role.yamlcontrollers/evalhub/evaluation_failed_kueue_workloads_reconciler.gocontrollers/evalhub/evaluation_failed_kueue_workloads_reconciler_lifecycle_test.gocontrollers/evalhub/evaluation_job_failure_reconciler.gocontrollers/evalhub/evaluation_job_failure_reconciler_lifecycle_test.gopolicy/rbac.rego
sheltoncyril
left a comment
sheltoncyril
left a comment
There was a problem hiding this comment.
Claude had a few comments
sheltoncyril
left a comment
sheltoncyril
left a comment
There was a problem hiding this comment.
AI code review (max depth, 10-angle sweep). 3 confirmed bugs, 9 additional findings. See inline comments.
Annotate the Workload before emitting the Event and patching Job labels so that workloadFailedEventAlreadyReported acts as the primary dedup gate on retry. Previously, if both patchJobFailureLabels and annotateWorkloadReported failed after a successful POST, the next reconcile would re-POST to EvalHub and re-emit the Event.
When the Workload annotation succeeds but patchJobFailureLabels fails, the Job permanently lacks the evaluation-phase and evaluation-status metadata. Requeue on patchJobFailureLabels failure and add a retry branch that patches only the Job labels when the Workload annotation is already present.
The serverAlreadyHandledFailure early-return path skipped Job deletion, leaving failed Jobs in the namespace indefinitely. Add the same deleteEvalHubFailureSyncedJob call that the failureAlreadyReported path already performs.
The role is currently bound to the operator SA but is intended for the EvalHub server SA once RHAI-277 lands. Update the comment to reflect the current state and planned rebinding.
When promotePatch failed, the retry path re-executed the POST to EvalHub and re-emitted the Kubernetes Event because failurePendingReport only skipped the pending-annotation write. Move the POST inside the !failurePendingReport block so retries skip it, and move Eventf after promotePatch so it only fires once the promote succeeds.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sheltoncyril The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@ruivieira: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What and why
EvalHub evaluation jobs can fail in ways the EvalHub server cannot self-report OOM kills, image pull errors, and Kueue admission failures all happen before or outside the server's callback path. This PR wires Kubernetes event emission into both failure reconcilers so these infrastructure failures are observable via
kubectl describe job/kubectl get events, completing the failure signal chain for RHAI-278.Closes RHOAIENG-80112, RHOAIENG-80113, RHOAIENG-80114.
Type
Testing
Lifecycle unit tests use
fake.Client,record.FakeRecorder, and a localhttptest.Serverto exercise the fullReconcile()path. Cases covered:evaluation-phase=Failedlabel already set by EvalHub server → operator returns early, no duplicate eventOOMKilled→EvaluationFailedwarning event emitted +evaluation-phase=Failedlabel +evaluation-statusannotation stamped on JobErrImagePull→ same lifecycleQuotaReserved=False / Inadmissible→EvaluationFailedwarning event + Job label patch + Workload annotated as reportedChanges
RHOAIENG-80112): addevents:create/patchandjobs:get/patchpermissions; wireEventRecorderinto both failure reconciler constructors viamgr.GetEventRecorderForRHOAIENG-80113): emitWarning / EvaluationFailedafter a successful POST to EvalHub; patchevaluation-phase=Failedandevaluation-statuson the Job; Kueue path also annotates the Workload as reportedRHOAIENG-80114): lifecycle unit tests for both reconcilers covering the four failure scenarios aboveSummary by CodeRabbit
New Features
Bug Fixes
Tests