Add FAR destructive remediation test scaffold

[ugreener]

Summary

Add the Ginkgo test scaffold for FAR destructive E2E tests. Introduces an Ordered, Serial Describe block with BeforeAll setup (platform detection, AWS credential retrieval, node parameter building, FAR leader identification, 3+ worker node validation), JustAfterEach safety-net cleanup (kubelet restart, node recovery, orphaned CR deletion), and helper functions for FART/FAR CR lifecycle management.

Changes

• New tests/far-operator/tests/far_destructive.go: Serial/Ordered Ginkgo Describe block with:
  • BeforeAll: AWS platform gating, fence agent resolution, FAR controller readiness check, 3+ Ready worker node validation, AWS credential retrieval from CCO Secret, fence_aws shared parameter assembly, per-node EC2 instance ID mapping, FAR leader identification
  • JustAfterEach: safety-net cleanup (kubelet restart, node readiness wait, orphaned FAR/FART CR deletion)
  • buildFARTUnstructured: helper to construct FenceAgentsRemediationTemplate CR as unstructured
  • deleteRemediationCR: unified cleanup helper with poll-until-deleted semantics
  • Placeholder Context blocks for standalone FAR remediation (RHWA-963, 7 tests) and NHC+FAR interop (RHWA-1035, 4 tests)
• New constant RemediationCRDeletionTimeout in tests/far-operator/internal/farparams/const.go

Dependencies

Part 3 of 3 for RHWA-962. Depends on #20 (constants + cluster helpers) and #23 (node disruption helpers).

Merge order: #20, then #23, then this PR. CI (lint/unit) will pass after both dependencies merge.

Jira: RHWA-962

Summary by CodeRabbit

• Tests
  • Added new destructive test coverage for FAR on AWS-only clusters, including setup checks, cleanup safeguards, and groundwork for future remediation scenarios.
  • Improved resource cleanup handling and added a longer wait period for deletion of remediation-related resources.

[coderabbitai]

Warning

Review limit reached

@ugreener, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9da3ee77-1c7b-4ba5-bd75-28f1ccbb7d60

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb6e7f and 49963c8.

📒 Files selected for processing (1)

• tests/far-operator/tests/far_destructive.go

📝 Walkthrough

📝 Walkthrough

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding a FAR destructive remediation test scaffold.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 155-161: The cleanup operations in the destructive flow are
silently ignoring errors by discarding return values with underscore assignment.
Instead of using `_ = farutils.StartKubelet(...)` and `_ =
farutils.WaitForNodeReady(...)` at the specified locations, capture the error
values returned by these functions into variables. Then propagate these captured
errors to the JustAfterEach block where they should be explicitly asserted or
logged to ensure failed cleanup operations become visible in the test output and
prevent cascading failures.
- Around line 159-162: The call to farutils.WaitForNodeReady references an
undefined constant farparams.NodeReadyTimeout at line 161, which will cause a
compilation failure. Replace farparams.NodeReadyTimeout with the correct
exported timeout constant from the tests/far-operator/internal/farparams
package. Check the farparams package to identify the actual exported constant
name that provides the node ready timeout value and update the WaitForNodeReady
function call to use that correct constant name.

🪄 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: 48015800-908d-44a0-98a5-bbf57b3802f2

📥 Commits

Reviewing files that changed from the base of the PR and between f347e04 and c9ead81.

📒 Files selected for processing (1)

• tests/far-operator/tests/far_destructive.go

[openshift-ci]

@ugreener: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	a53a817	link	true	/test lint

Full PR test history. Your PR dashboard.

Details

Instructions 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.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

tests/far-operator/tests/far_destructive.go (2)

159-161: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Undefined constant farparams.NodeReadyTimeout still breaks compilation.

This was previously flagged as a critical typecheck failure and remains unresolved in this version — farparams.NodeReadyTimeout is referenced but not defined per golangci-lint.

#!/bin/bash
rg -n 'NodeReadyTimeout|ReadyTimeout' tests/far-operator/internal/farparams/

🤖 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 159 - 161, The test
in far_destructive.go still references an undefined farparams.NodeReadyTimeout,
causing the compile failure. Update the WaitForNodeReady call to use a timeout
value that actually exists in farparams, or add the missing exported constant in
the farparams package and use that symbol consistently. Make sure the identifier
used by WaitForNodeReady matches a defined timeout symbol so golangci-lint
typecheck passes.

Source: Linters/SAST tools

---

152-176: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Cleanup failures silently swallowed in safety-net path.

StartKubelet/WaitForNodeReady errors are discarded, and deleteFARCR/deleteFARTCR are void functions that only log warnings internally — failed safety-net cleanup (e.g., a stuck kubelet, an undeleted FAR CR) becomes invisible in spec output, risking cascading failures across subsequent ordered specs in this Serial, Ordered suite. This was raised previously; the underlying pattern is still present.

🤖 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 152 - 176, The
safety-net cleanup in the deferred block of far_destructive.go is still
swallowing failures from StartKubelet and WaitForNodeReady, and
deleteFARCR/deleteFARTCR only log internally, so cleanup problems never surface
in the test. Update this cleanup path to capture and report errors from the
kubelet restart/readiness wait and from FAR/FART deletion, and ensure the
deferred safety-net in the far destructive spec fails or logs prominently when
cleanup does not succeed so ordered serial runs can see the issue.

🤖 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/internal/farparams/const.go`:
- Around line 75-77: The new RemediationCRDeletionTimeout constant in farparams
is not being used by the deletion helpers, so the timeout value can drift from
its consumer. Update both deleteFARCR and deleteFARTCR in far_destructive.go to
pass farparams.RemediationCRDeletionTimeout into wait.PollUntilContextTimeout
instead of the hardcoded 2*time.Minute, and keep the constant as the single
source of truth.

In `@tests/far-operator/tests/far_destructive.go`:
- Around line 231-307: `deleteFARCR` and `deleteFARTCR` duplicate the same
cleanup flow and still use raw `wait.PollUntilContextTimeout` with a hardcoded
`2*time.Minute` instead of the project polling pattern. Refactor the shared
delete-and-wait logic into a helper that works for both GVKs, and switch the
post-delete wait to `Eventually`/`Consistently` using
`farparams.DefaultPollInterval` plus `farparams.RemediationCRDeletionTimeout`
(or the matching params constant) so the timeout is sourced from params rather
than inline. Keep the existing `k8sClient.Get`, `Delete`, and `GinkgoWriter`
error handling behavior, but apply it through the shared helper for both
`deleteFARCR` and `deleteFARTCR`.
- Line 66: The tests in far_destructive.go still reference missing helpers in
farutils, so the package won’t compile. Add or restore the expected helpers in
tests/far-operator/internal/farutils, specifically DetectPlatform and
CountReadyWorkerNodes, or update the call sites in the destructive test flow to
use the new symbol names consistently. Make sure the references in the main test
logic (including the platform detection and worker-node readiness checks) match
the available farutils API.

---

Duplicate comments:
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 159-161: The test in far_destructive.go still references an
undefined farparams.NodeReadyTimeout, causing the compile failure. Update the
WaitForNodeReady call to use a timeout value that actually exists in farparams,
or add the missing exported constant in the farparams package and use that
symbol consistently. Make sure the identifier used by WaitForNodeReady matches a
defined timeout symbol so golangci-lint typecheck passes.
- Around line 152-176: The safety-net cleanup in the deferred block of
far_destructive.go is still swallowing failures from StartKubelet and
WaitForNodeReady, and deleteFARCR/deleteFARTCR only log internally, so cleanup
problems never surface in the test. Update this cleanup path to capture and
report errors from the kubelet restart/readiness wait and from FAR/FART
deletion, and ensure the deferred safety-net in the far destructive spec fails
or logs prominently when cleanup does not succeed so ordered serial runs can see
the issue.

🪄 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: 03e359ca-7d44-4178-a7d4-83b3924ce23b

📥 Commits

Reviewing files that changed from the base of the PR and between c9ead81 and a1f612c.

📒 Files selected for processing (2)

• tests/far-operator/internal/farparams/const.go
• tests/far-operator/tests/far_destructive.go

[ugreener]

Resolving 2 stale CodeRabbit threads:

Thread 1 (farparams.NodeReadyTimeout undefined): NodeReadyTimeout is defined at const.go:60-61 (10 * time.Minute), introduced by PR #20 which has since merged. The typecheck failure was expected while the dependency PR was still open.

Thread 2 (RemediationCRDeletionTimeout not wired): The consolidated deleteRemediationCR helper now uses farparams.RemediationCRDeletionTimeout at both the poll timeout and the warning message. The hardcoded 2*time.Minute and the duplicate deleteFARCR/deleteFARTCR functions have been removed.

[coderabbitai]

♻️ Duplicate comments (2)

tests/far-operator/tests/far_destructive.go (2)

148-190: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Make safety-net cleanup failure-scoped and fail visibly.

Line 150 only controls the log; the safety-net still restarts kubelet and deletes recorded CRs after successful specs. Also, Lines 160-174 only print warnings, then Line 176 clears targetNode, so a failed node recovery can be hidden from the ordered suite.

Suggested adjustment

 		JustAfterEach(func() {
 			spec := CurrentSpecReport()
-			if spec.Failed() {
-				GinkgoWriter.Println(
-					"Test failed - running safety net cleanup")
+			if !spec.Failed() {
+				return
 			}
+
+			GinkgoWriter.Println(
+				"Test failed - running safety net cleanup")
@@
 				if err := farutils.StartKubelet(ctx, nodeName); err != nil {
-					GinkgoWriter.Printf(
-						"Warning: safety net failed to start kubelet on %s: %v\n",
-						nodeName, err)
+					AddReportEntry("safety net cleanup",
+						fmt.Sprintf("failed to start kubelet on %s: %v", nodeName, err))
+					Expect(err).ToNot(HaveOccurred())
 				}
@@
 				if err := farutils.WaitForNodeReady(
 					ctx, APIClient, nodeName,
 					farparams.NodeReadyTimeout); err != nil {
-					GinkgoWriter.Printf(
-						"Warning: safety net node %s did not become Ready: %v\n",
-						nodeName, err)
+					AddReportEntry("safety net cleanup",
+						fmt.Sprintf("node %s did not become Ready: %v", nodeName, err))
+					Expect(err).ToNot(HaveOccurred())
 				}

🤖 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 148 - 190, The
safety-net cleanup in JustAfterEach is currently running for every spec and only
logging warnings, so failures can be hidden and cleanup can affect successful
tests. Gate the kubelet restart and CR deletion logic on
CurrentSpecReport().Failed() (or equivalent failed-spec check) inside
JustAfterEach, and make any StartKubelet/WaitForNodeReady failure surface as a
failing test state instead of only printing via GinkgoWriter. Keep the cleanup
references around targetNode, currentFARName, and currentFARTName, but only
clear them after a successful failed-spec cleanup path.

---

253-278: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Use Gomega polling so deletion failures fail the spec.

This still uses raw wait.PollUntilContextTimeout and only logs on timeout. Convert this cleanup wait to EventuallyWithOffset with farparams.RemediationCRDeletionTimeout and farparams.DefaultPollInterval so failures surface in Ginkgo output. As per coding guidelines, "Use Eventually/Consistently with explicit timeout + poll interval from <op>params/medik8sparams — never use time.Sleep".

Suggested adjustment

-	if waitErr := wait.PollUntilContextTimeout(
-		ctx, farparams.DefaultPollInterval, farparams.RemediationCRDeletionTimeout, true,
-		func(ctx context.Context) (bool, error) {
-			if err := k8sClient.Get(ctx, key, obj); err != nil {
-				if k8serrors.IsNotFound(err) {
-					return true, nil
-				}
-
-				return false, nil
-			}
-
-			if delErr := k8sClient.Delete(ctx, obj); delErr != nil {
-				if k8serrors.IsNotFound(delErr) {
-					return true, nil
-				}
-
-				return false, nil
-			}
-
-			return false, nil
-		},
-	); waitErr != nil {
-		GinkgoWriter.Printf(
-			"Warning: %s %s not fully deleted within %s: %v\n",
-			gvk.Kind, name, farparams.RemediationCRDeletionTimeout, waitErr)
-	}
+	EventuallyWithOffset(1, func() error {
+		obj := &unstructured.Unstructured{}
+		obj.SetGroupVersionKind(gvk)
+
+		if err := k8sClient.Get(ctx, key, obj); err != nil {
+			if k8serrors.IsNotFound(err) {
+				return nil
+			}
+
+			return err
+		}
+
+		if err := k8sClient.Delete(ctx, obj); err != nil &&
+			!k8serrors.IsNotFound(err) {
+			return err
+		}
+
+		if err := k8sClient.Get(ctx, key, obj); err != nil {
+			if k8serrors.IsNotFound(err) {
+				return nil
+			}
+
+			return err
+		}
+
+		return fmt.Errorf("%s %s still exists", gvk.Kind, name)
+	}, farparams.RemediationCRDeletionTimeout, farparams.DefaultPollInterval).
+		Should(Succeed(), "%s %s was not deleted in time", gvk.Kind, name)

🤖 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 253 - 278, Replace
the raw wait.PollUntilContextTimeout cleanup loop in the destructive test with
Gomega EventuallyWithOffset so deletion errors fail the spec instead of only
logging a warning. Keep the same deletion check logic using k8sClient.Get,
k8serrors.IsNotFound, and k8sClient.Delete, but express it as an Eventually
assertion with farparams.RemediationCRDeletionTimeout and
farparams.DefaultPollInterval, and remove the GinkgoWriter timeout-only fallback
so the failure is reported directly by Ginkgo.

Source: Coding guidelines

🤖 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.

Duplicate comments:
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 148-190: The safety-net cleanup in JustAfterEach is currently
running for every spec and only logging warnings, so failures can be hidden and
cleanup can affect successful tests. Gate the kubelet restart and CR deletion
logic on CurrentSpecReport().Failed() (or equivalent failed-spec check) inside
JustAfterEach, and make any StartKubelet/WaitForNodeReady failure surface as a
failing test state instead of only printing via GinkgoWriter. Keep the cleanup
references around targetNode, currentFARName, and currentFARTName, but only
clear them after a successful failed-spec cleanup path.
- Around line 253-278: Replace the raw wait.PollUntilContextTimeout cleanup loop
in the destructive test with Gomega EventuallyWithOffset so deletion errors fail
the spec instead of only logging a warning. Keep the same deletion check logic
using k8sClient.Get, k8serrors.IsNotFound, and k8sClient.Delete, but express it
as an Eventually assertion with farparams.RemediationCRDeletionTimeout and
farparams.DefaultPollInterval, and remove the GinkgoWriter timeout-only fallback
so the failure is reported directly by Ginkgo.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: db318145-56bf-445c-865c-d979e3461e07

📥 Commits

Reviewing files that changed from the base of the PR and between a1f612c and 5ead516.

📒 Files selected for processing (2)

• tests/far-operator/internal/farparams/const.go
• tests/far-operator/tests/far_destructive.go

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/far-operator/internal/farparams/const.go

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7, ugreener

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [razo7,ugreener]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment