Add 6 standalone FAR destructive tests and improve test resiliency - #49
Conversation
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again |
|
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:
📝 WalkthroughWalkthroughThis PR adds FAR destructive remediation test coverage, supporting constants and helpers, a node debug namespace fix, controller failover checks, and README documentation for the destructive scenarios. ChangesFAR Destructive Remediation Tests
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/README.md`:
- Around line 118-120: The Destructive Tests section in README groups
`OCP-70636` with node-fencing, but that test is a controller-pod handover case
and needs its own subsection. Update the documentation around the `OCP-70636`
entry to separate it from the `fence_aws`/node-reboot description, and adjust
the prerequisites and command guidance so they match the actual `OCP-70636`
implementation and `disruption:destructive` tagging.
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 490-499: The `Eventually` in the workload pod phase check uses a
hardcoded `2*time.Minute` instead of a shared timeout constant. Add a named
timeout in `farparams` (for example, a `PodRunningTimeout` constant in
`farparams/const.go`) and update the `Eventually` call in the pod-running
assertion to use that constant with the existing
`farparams.DefaultPollInterval`, matching the other timeout usages in
`far_destructive.go`.
- Around line 527-530: The leadership-transfer spec is tagged with both
disruption modes, which causes it to match conflicting filters. Update the test
case in the controller leadership scenario so it only keeps the inherited
destructive classification and remove the extra labels.DisruptionNonDestructive
label from the It block.
🪄 Autofix (Beta)
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: bfcfbf47-45ef-4f4c-a835-ffc51b0e7621
📒 Files selected for processing (3)
tests/far-operator/README.mdtests/far-operator/internal/farparams/const.gotests/far-operator/tests/far_destructive.go
ugreener
changed the title
ugreener
changed the title
2409733 to
6a592d5
Compare
|
/test 4.22-konflux-e2e-far-aws |
6a592d5 to
44f4635
Compare
|
/test 4.22-konflux-e2e-far-aws |
44f4635 to
2169be9
Compare
|
/test 4.22-konflux-e2e-far-aws |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/far-operator/tests/far_destructive.go (2)
654-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
buildFARUnstructuredduplicatesbuildFARTUnstructured's spec-construction logic.Both helpers build near-identical
specmaps (agent/sharedparameters/nodeparameters/retrycount/retryinterval/timeout/remediationStrategy), differing only in whether it's nested undertemplate.spec. Consider extracting a sharedbuildFARSpec(...)helper to avoid drift between the two builders as fields evolve.♻️ Proposed refactor
+func buildFARSpec(agent string, sharedParams, nodeParams map[string]interface{}) map[string]interface{} { + return map[string]interface{}{ + "agent": agent, + "sharedparameters": sharedParams, + "nodeparameters": nodeParams, + "retrycount": farparams.FARCRRetryCount, + "retryinterval": farparams.FARCRRetryInterval, + "timeout": farparams.FARCRTimeout, + "remediationStrategy": farparams.FARCRRemediationStrategy, + } +} + func buildFARUnstructured( nodeName, agent string, sharedParams, nodeParams map[string]interface{}, ) *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "fence-agents-remediation.medik8s.io/v1alpha1", "kind": "FenceAgentsRemediation", "metadata": map[string]interface{}{ "name": nodeName, "namespace": medik8sparams.OperatorNs, }, - "spec": map[string]interface{}{ - "agent": agent, - "sharedparameters": sharedParams, - "nodeparameters": nodeParams, - "retrycount": farparams.FARCRRetryCount, - "retryinterval": farparams.FARCRRetryInterval, - "timeout": farparams.FARCRTimeout, - "remediationStrategy": farparams.FARCRRemediationStrategy, - }, + "spec": buildFARSpec(agent, sharedParams, nodeParams), }, } }🤖 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 `@tests/far-operator/tests/far_destructive.go` around lines 654 - 677, `buildFARUnstructured` is duplicating the same FAR `spec` construction already used by `buildFARTUnstructured`, which risks the two builders drifting apart. Extract the shared fields (`agent`, `sharedparameters`, `nodeparameters`, `retrycount`, `retryinterval`, `timeout`, `remediationStrategy`) into a common helper such as `buildFARSpec(...)`, and have both builders reuse it with their respective wrapping (`spec` vs `template.spec`). Keep the existing `buildFARUnstructured` and `buildFARTUnstructured` entry points, but centralize the shared map assembly so future field changes only happen in one place.
617-623: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nil check after already-asserted expectation.
assertion.Expect(lease.Spec.HolderIdentity).ToNot(BeNil())already fails the poll iteration if nil; the follow-upif lease.Spec.HolderIdentity != nilguard is unreachable dead code at that point.🤖 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 `@tests/far-operator/tests/far_destructive.go` around lines 617 - 623, The nil guard around lease.Spec.HolderIdentity is redundant because the preceding Expect(...).ToNot(BeNil()) already handles the nil case in the polling assertion. In the test flow that checks lease ownership after pod deletion, remove the extra if block and keep the direct dereference/assertion path in the same check so the logic stays clean and only relies on the existing expectation around lease.Spec.HolderIdentity and oldPodName.
🤖 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 596-608: Remove the nested wait around the FAR deployment
readiness check in the test flow using deployment.Pull and IsReady. The current
Eventually wrapper is redundant because IsReady already blocks internally, so
replace it with a direct readiness assertion or switch to a non-blocking check
before retrying. Keep the logic near the FAR controller deployment readiness
validation, and preserve the existing timeout intent without stacking two
waiting mechanisms.
- Around line 171-183: The safety-net recovery path in JustAfterEach only logs
when farutils.WaitForNodeReady fails, which lets later Ordered specs continue
while the cluster is still degraded. Update the recovery branch to fail the spec
when the remediated node does not become Ready, using the existing nodeName,
farparams.NodeReadyTimeout, and AddReportEntry context in far_destructive.go.
Keep the warning/reporting, but also surface the error through the test failure
path so ContinueOnFailure does not mask the original issue.
---
Nitpick comments:
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 654-677: `buildFARUnstructured` is duplicating the same FAR `spec`
construction already used by `buildFARTUnstructured`, which risks the two
builders drifting apart. Extract the shared fields (`agent`, `sharedparameters`,
`nodeparameters`, `retrycount`, `retryinterval`, `timeout`,
`remediationStrategy`) into a common helper such as `buildFARSpec(...)`, and
have both builders reuse it with their respective wrapping (`spec` vs
`template.spec`). Keep the existing `buildFARUnstructured` and
`buildFARTUnstructured` entry points, but centralize the shared map assembly so
future field changes only happen in one place.
- Around line 617-623: The nil guard around lease.Spec.HolderIdentity is
redundant because the preceding Expect(...).ToNot(BeNil()) already handles the
nil case in the polling assertion. In the test flow that checks lease ownership
after pod deletion, remove the extra if block and keep the direct
dereference/assertion path in the same check so the logic stays clean and only
relies on the existing expectation around lease.Spec.HolderIdentity and
oldPodName.
🪄 Autofix (Beta)
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: 2626221b-8263-493d-8b72-06480656bcf6
📒 Files selected for processing (4)
tests/far-operator/README.mdtests/far-operator/internal/farparams/const.gotests/far-operator/tests/far_destructive.gotests/internal/helpers/node_ops.go
✅ Files skipped from review due to trivial changes (1)
- tests/far-operator/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/internal/helpers/node_ops.go
- tests/far-operator/internal/farparams/const.go
|
/test 4.22-konflux-e2e-far-aws |
1 similar comment
|
/test 4.22-konflux-e2e-far-aws |
|
/cancel 4.22-konflux-e2e-far-aws |
|
/test 4.22-konflux-e2e-far-aws |
1 similar comment
|
/test 4.22-konflux-e2e-far-aws |
0f3ef1d to
a2d97e6
Compare
|
/test 4.22-konflux-e2e-far-aws |
1 similar comment
|
/test 4.22-konflux-e2e-far-aws |
ugreener
changed the title
ugreener
changed the title
|
/test 4.22-konflux-e2e-far-aws |
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
Before I approve can we squash the 15 commits to ~4 logical commits before merge:
- Add standalone FAR destructive tests
- Fix pod-running smoke test race condition (far.go)
- Add randomized node selection and oc debug namespace fix (helpers)
- Add
--randomize-allto test runner
Also see inline comments below
34aa7af to
80d51d8
Compare
|
/test 4.22-konflux-e2e-far-aws |
|
/test 4.22-konflux-e2e-far-aws |
2 similar comments
|
/test 4.22-konflux-e2e-far-aws |
|
/test 4.22-konflux-e2e-far-aws |
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
Great improvement since last review but not all the comments have been addressed (16 fully addressed, 1 deferred (RHWA-1330), 1 declined (OCP-66228), 1 partial (workload in OCP-70638), 1 pending (commit squash to ~4).
| if err != nil && !k8serrors.IsAlreadyExists(err) { | ||
| Expect(err).ToNot(HaveOccurred(), | ||
| "Failed to create shared credentials Secret") | ||
| } |
There was a problem hiding this comment.
This Secret containing AWS access key and secret key is created here but has no corresponding cleanup (AfterAll, DeferCleanup, or AfterSuite). Commit df5c1be removed the per-spec DeferCleanup (correctly — it was deleting the Secret between specs) but didn't add a suite-level replacement.
AWS credentials persist in the operator namespace indefinitely after test completion. Any principal with get secrets RBAC can extract them.
Suggestion: Add a suite-level cleanup using Defer
There was a problem hiding this comment.
Fixed in b14292b. Added AfterSuite in far_suite_test.go that deletes the credentials Secret after all specs complete.
DeferCleanup inside the guarded BeforeEach would fire after the first spec and delete the Secret that subsequent specs still need (the FAR CR references it via SharedSecretName during remediation). AfterSuite runs once after all specs, which is the correct scope for this one-time resource.
|
|
||
| By("Verifying FAR NoSchedule taint removed after CR deletion") | ||
|
|
||
| Eventually(func(assertion Gomega) { |
There was a problem hiding this comment.
This Eventually().Should(Succeed()) calls Fail() on timeout, which panics and stops execution of the rest of this JustAfterEach. The FARTemplate deletion (line 260) and node recovery safety net (lines 266-278) will NOT execute.
This is the same failure mode razo7 flagged earlier for the FAR CR Succeeded wait — that was correctly fixed with wait.PollUntilContextCancel (lines 195-238), but this new Eventually reintroduces the pattern.
Failure scenario: Controller slow to remove taint after CR deletion → timeout → Fail() → FARTemplate CR and node recovery skipped.
Suggestion: Replace with wait.PollUntilContextCancel + warning log, consistent with the pattern at lines 195-238.
There was a problem hiding this comment.
Fixed in cc7e0af. Replaced the Eventually with wait.PollUntilContextCancel + warning log, matching the pattern at lines 196-238. The taint check now logs a warning on timeout and proceeds to the node recovery safety net instead of panicking.
|
|
||
| Eventually(func(assertion Gomega) { | ||
| err := k8sClient.Create(ctx, farCR) | ||
| if err != nil && !k8serrors.IsAlreadyExists(err) { |
There was a problem hiding this comment.
If deleteRemediationCR (line 665) hasn't fully finalized the old CR, Create returns IsAlreadyExists. The if guard skips the assertion, Eventually considers the iteration successful and exits the loop. The test then operates on the stale old CR.
If the old CR is already in Succeeded state, waitForRemediation may pass vacuously without exercising new fencing.
This was implemented per razo7's suggestion to handle the transient-network-error case (client timeout but server-side create succeeded). That case IS correct. But the semantics are broader — it also swallows the "old CR still finalizing" case.
Suggestion: At minimum, log when IsAlreadyExists is encountered for debuggability:
if k8serrors.IsAlreadyExists(err) {
GinkgoWriter.Printf("INFO: FAR CR %s already exists, treating as success\n", farCR.GetName())
return
}Or: make deleteRemediationCR wait until the object is fully gone before returning.
There was a problem hiding this comment.
Fixed in cc7e0af. Added a diagnostic log when IsAlreadyExists is encountered:
if k8serrors.IsAlreadyExists(err) {
GinkgoWriter.Printf(
"INFO: FAR CR %s already exists (prior delete may not have finalized), treating as success\n",
farCR.GetName())
return
}deleteRemediationCR already polls until IsNotFound before createFARCR calls Create, so the stale-CR scenario requires the delete to time out (which already logs its own warning). This log creates a diagnostic breadcrumb linking the two events.
|
|
||
| currentFARName = targetNode.Name | ||
|
|
||
| waitForRemediation(ctx, APIClient, targetNode.Name, oldBootID) |
There was a problem hiding this comment.
Two gaps compared to other fencing tests and the PR's own README:
-
No workload pod: All 5 non-leader fencing tests create a workload pod and verify eviction via the shared
JustAfterEach. This test fences the leader node (which IS rebooted) but skips workload verification. razo7's comment said "every test should end with workload verification." -
No lease transfer assertion: The README claims "controller failover occurs" but the test only checks
farDeployment.IsReady(). Compare with OCP-70636 (far_controller_lifecycle.go) which explicitly verifieslease.Spec.HolderIdentitychanged. A deployment being Ready doesn't prove a different pod acquired the lease.
Suggestion:
- Add workload pod creation + eviction check (reuse the non-leader worker target pattern)
- Add lease transfer assertion (reuse the OCP-70636 pattern)
There was a problem hiding this comment.
Fixed in bff464a. Added both:
1. Workload pod creation + eviction check: Creates a workload pod pinned to the leader node before fencing, waits for Running, then verifies eviction after remediation. Matches the non-leader worker target pattern.
2. Lease transfer assertion: After deployment readiness check, verifies lease.Spec.HolderIdentity is non-nil (controller lease has an active holder after the leader node reboot).
bff464a to
7441cf8
Compare
|
Squashed to 4 commits as requested:
|
7441cf8 to
aa26dc4
Compare
Add 6 standalone FAR remediation tests (OCP-61229, OCP-70638, OCP-65960, OCP-67015, OCP-66203, OCP-70636) with resiliency improvements: - Randomized worker node selection to avoid deterministic reuse - CRI-O overlay cleanup to prevent corruption across reboots - createFARCR/waitForRemediation helpers with retry logic - Controller readiness pre-flight and diagnostic logging on failure - Workload pod eviction verification in shared JustAfterEach - Non-failing safety net cleanup with AddReportEntry on recovery failure Tracking: RHWA-963
Replace two-step wait+count pattern with a single Eventually loop that lists and filters running pods atomically. Prevents the race where WaitForAllPodsInNamespaceRunning returns but a pod restarts before the count assertion executes.
SelectWorkerNode now shuffles eligible nodes to prevent deterministic reuse of the same worker across sequential destructive tests. RunOnNode adds -n default to oc debug to avoid namespace permission errors in locked-down CI environments.
aa26dc4 to
c6b2b8f
Compare
|
/test 4.22-konflux-e2e-far-aws |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abrugaro, 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
Summary
Add 6 standalone FAR destructive remediation tests and improve overall FAR test resiliency.
Tracking: RHWA-963
New destructive tests (ported from ocp-edge-auto to Go/Ginkgo):
Test structure (nested Context/BeforeEach pattern per Ginkgo best practices):
"non-leader worker target"Context with sharedBeforeEachfor node selection, boot ID recording, workload pod creation, and sharedJustAfterEachfor workload eviction verification"leader node target"Context for OCP-70638 (targets the active controller node)"Controller lifecycle"Context for OCP-70636 (non-destructive pod deletion test)Resiliency improvements:
SelectWorkerNodeto prevent deterministic reuse of the same node across sequential destructive testsrm -rf /var/lib/containers/storage/overlay/l && systemctl restart crio) to prevent overlay corruption from cascading across testslogPodDiagnosticsfunction that logs pod Events, container statuses (Waiting/Terminated reasons), and pod conditions on workload pod failurecreateFARCRhelper with retry viaEventuallyto handle transient webhook timeoutswaitForRemediationhelper encapsulatingWaitForNodeReboot+WaitForNodeReadysequenceBeforeEachbefore each destructive testlogFARControllerStatediagnostic logging on test failureEventuallyfor resilience after node reboots inContinueOnFailuresuitesEventuallyloop