Add event verification helper and FAR lifecycle event assertions - #71
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds Kubernetes event helpers, extends FAR destructive tests with remediation event checks, updates documented pass criteria, and removes temporary SSH keys when permission changes fail. ChangesFAR event verification
SSH key cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoVerify FAR operator Kubernetes Events in destructive tests
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 425-447: Expose a one-attempt event-check helper in events.go,
then replace the two WaitForEvents assertions in the FAR lifecycle and Node
completion checks with Eventually using farparams.FARConditionTimeout and
farparams.DefaultPollInterval. Keep WaitForEvents unchanged and do not wrap it
in Eventually; assert the new helper’s single-attempt result directly within the
polling callback.
In `@tests/internal/helpers/events.go`:
- Around line 26-30: The event helpers at tests/internal/helpers/events.go:26-30
and tests/internal/helpers/events.go:78-81 must bind assertions to the current
remediation: extend InvolvedObjectRef with the UID and occurrence-time boundary,
add involvedObject.uid to the selector, and client-side filter using EventTime,
Series.LastObservedTime, or legacy LastTimestamp. In
tests/far-operator/tests/far_destructive.go:425-447, update createFARCR to
return the API-created object and fail on AlreadyExists, then pass its UID and
CreationTimestamp, along with the target Node UID, to both event checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08a58b1c-ce3f-43fb-b5b7-4f6efc76eb89
📒 Files selected for processing (2)
tests/far-operator/tests/far_destructive.gotests/internal/helpers/events.go
Code Review by Qodo
1.
|
![qodo-2-for-medik8s[bot]](https://avatars.githubusercontent.com/in/4026794?s=60&v=4){kind=small}
b7be8b2 to
5b723d8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/internal/helpers/node_ops.go`:
- Around line 180-185: Update the chmod and write error branches in findSSHKey
to close tmpFile before calling os.Remove, matching the existing close-error
branch; preserve the current error propagation while ensuring cleanup occurs
before removal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6619125b-1f35-49de-8865-94a0b2418da2
📒 Files selected for processing (4)
tests/far-operator/README.mdtests/far-operator/tests/far_destructive.gotests/internal/helpers/events.gotests/internal/helpers/node_ops.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/far-operator/tests/far_destructive.go
- tests/internal/helpers/events.go
5b723d8 to
2ff2c58
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test 4.22-konflux-e2e-far-aws |
2ff2c58 to
43ecf70
Compare
ugreener
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test 4.22-konflux-e2e-far-aws |
Add a shared WaitForEvents helper in tests/internal/helpers/events.go that polls the API server for expected Kubernetes Events on a given involved object (CR or Node). The helper uses field selectors for efficient server-side filtering and wait.PollUntilContextCancel for retry logic. Add event assertions to two FAR destructive tests: - OCP-61229 (standalone worker remediation): verify RemediationStarted, FenceAgentSucceeded, RemediationFinished events on the FAR CR and NodeRemediationCompleted on the target Node - OCP-70638 (leader node remediation): verify the same lifecycle events survive controller failover when the active leader is rebooted The helper is operator-agnostic and can be reused by other operator test suites (NHC, SNR, SBR) that emit events via medik8s/common. Jira: RHWA-1330 Co-Authored-By: Claude <noreply@anthropic.com>
43ecf70 to
6563ab2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test 4.22-konflux-e2e-far-aws |
There was a problem hiding this comment.
Not sure why we need an additional spaces here, but I won't block on that
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eemcmullan, JonahSussman, razo7, ugreener The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Problem
The FAR destructive tests validate remediation outcomes (node reboot, taint application, CR status conditions, workload eviction, controller handover) but do not verify the Kubernetes Events emitted by the FAR operator during the remediation lifecycle. These events are part of the operator's stable contract and are used by monitoring tools, NHC, and cluster administrators to track remediation progress.
Summary
Add a shared event verification helper and use it to assert FAR lifecycle events in two destructive tests that exercise distinct code paths.
Changes
tests/internal/helpers/events.gowith a reusableWaitForEventshelper that polls the API server for expected Kubernetes Events using field selectors. The helper is operator-agnostic and designed for reuse across all medik8s operator test suites (NHC, SNR, SBR)RemediationStarted,FenceAgentSucceeded, andRemediationFinishedevents on the FAR CR, andNodeRemediationCompletedon the target NodewaitForRemediation()completes (reboot + Ready), so all events are expected to be present by that pointJira: RHWA-1330
Summary by CodeRabbit
Tests
Bug Fixes