far: Add CP remediation, 2-worker leader failover, and 0-worker topology destructive tests - #80
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
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:
📝 WalkthroughWalkthroughThe change adds topology constants and Kubernetes helpers for FAR remediation tests. It adds control-plane and minimal-worker destructive scenarios, configurable remediation timeouts, cleanup error reporting, health polling, and test documentation. ChangesFAR remediation scenarios
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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 QodoAdd FAR control-plane remediation and 2-worker destructive tests
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/farutils/cluster.go`:
- Around line 180-195: Update UncordonNodes to return and propagate retrieval
and patch errors, retrying each test-cordoned node until it is schedulable. Only
modify nodes that still have farparams.TestCordonAnnotation; remove that
annotation even when the node is already schedulable. Update its callers to
handle the returned cleanup error instead of reporting successful topology
cleanup unconditionally.
In `@tests/far-operator/tests/far_destructive.go`:
- Line 787: Parameterize waitForRemediation to accept recovery timeout values
instead of always using NodeRebootTimeout and NodeReadyTimeout, while preserving
its existing behavior for callers. Update this control-plane test call to pass
CPRebootTimeout and CPNodeReadyTimeout.
- Around line 741-828: Add the shared reporter package’s ReportIfFailed() call
to both new It specifications in
tests/far-operator/tests/far_destructive.go:741-828 and
tests/far-operator/tests/far_destructive.go:844-992, following the package’s
required invocation contract. Ensure each test reports configured namespaces and
CRDs on failure; no other test behavior needs changing.
- Around line 946-964: Move the FAR ReadyReplicas Eventually assertion in the
degraded-capacity test to after remediation has started, using
waitForRemediation or an equivalent target-node-unavailable/processing signal
before evaluating availability. Keep polling the deployment through the degraded
interval and retain the existing FARMinReplicasDuringDegraded threshold.
🪄 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: 7991877d-66b7-4950-b664-ecd6fdf6db10
📒 Files selected for processing (6)
tests/far-operator/internal/farparams/const.gotests/far-operator/internal/farutils/cluster.gotests/far-operator/tests/far_destructive.gotests/far-operator/tests/upgrade.gotests/internal/helpers/clusteroperator.gotests/internal/labels/labels.go
Code Review by Qodo
1.
|
![qodo-2-for-medik8s[bot]](https://avatars.githubusercontent.com/in/4026794?s=60&v=4){kind=small}
ugreener
changed the title
9816acb to
439bfef
Compare
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 203-205: Update the description for “Remediate a Control Plane
Node and Verify etcd Quorum Preservation” to accurately state that etcd
ClusterOperator health is checked before remediation and after node recovery,
rather than claiming quorum is verified continuously during remediation.
- Around line 216-227: Update the “Complete FAR Remediation with Only 2
Schedulable Workers” documentation to remove or revise the claim that a FAR
replica stays Running during degraded capacity until the corresponding assertion
verifies remediation has started. Keep the pass criteria aligned with the actual
test behavior and retain only coverage that is currently implemented.
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 844-849: Update UncordonNodes to return any API restoration error
instead of only logging and returning, then update the AfterEach cleanup around
cordonedNodes to assert that uncordoning succeeds. Preserve clearing
cordonedNodes after the restoration attempt while ensuring the test fails when
worker restoration fails.
🪄 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: b73b89f1-5853-440a-a28a-3132dcb1abe3
📒 Files selected for processing (3)
tests/far-operator/README.mdtests/far-operator/internal/farutils/cluster.gotests/far-operator/tests/far_destructive.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/far-operator/internal/farutils/cluster.go
f0a8a0c to
b197d7a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/far-operator/tests/far_destructive.go (1)
693-698: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the
"etcd"operator name into afarparamsconstant and use it at both call sites.🤖 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 693 - 698, Define a farparams constant for the etcd operator name, then replace the hardcoded "etcd" value in both WaitForClusterOperatorHealthy call sites with that constant.
🤖 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.
Nitpick comments:
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 693-698: Define a farparams constant for the etcd operator name,
then replace the hardcoded "etcd" value in both WaitForClusterOperatorHealthy
call sites with that constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5e26ba0-ea7d-486a-82fa-e699547bfd93
📒 Files selected for processing (2)
tests/far-operator/README.mdtests/far-operator/tests/far_destructive.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/far-operator/README.md
5a8463a to
507287a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/far-operator/tests/far_destructive.go (3)
736-765: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the FAR
Succeededcondition check into a helper.The same condition-scan logic appears at Lines 479-513, at Lines 188-231, and here. A shared helper reduces the duplication and removes the
assertion.Expect(false).To(BeTrue(), ...)pattern at Line 763.♻️ Proposed helper
func expectFARSucceeded(ctx context.Context, farName string) { GinkgoHelper() Eventually(func(assertion Gomega) { farObj := &unstructured.Unstructured{} farObj.SetGroupVersionKind(farGVK) assertion.Expect(APIClient.Get(ctx, client.ObjectKey{ Name: farName, Namespace: medik8sparams.OperatorNs, }, farObj)).To(Succeed()) conditions, found, condErr := unstructured.NestedSlice( farObj.Object, "status", "conditions") assertion.Expect(condErr).ToNot(HaveOccurred()) assertion.Expect(found).To(BeTrue(), "FAR CR has no status.conditions") assertion.Expect(conditions).To(ContainElement(SatisfyAll( HaveKeyWithValue("type", farparams.FARConditionSucceeded), HaveKeyWithValue("status", string(metav1.ConditionTrue)), )), "FAR CR Succeeded condition is not True") }, farparams.FARConditionTimeout, farparams.DefaultPollInterval).Should(Succeed()) }🤖 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 736 - 765, Extract the duplicated FAR Succeeded-condition polling logic into a shared expectFARSucceeded helper, and replace the inline scan at this location and the corresponding checks near the other referenced call sites with helper calls. Keep the API lookup, status.conditions validation, timeout, and polling behavior unchanged, and use a collection assertion such as ContainElement to verify the condition rather than assertion.Expect(false).To(BeTrue()).
693-698: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDefine an etcd ClusterOperator name constant
The
"etcd"literal appears twice. Addfarparams.EtcdClusterOperatorNameand use it in both calls.🤖 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 693 - 698, Define the etcd ClusterOperator name as farparams.EtcdClusterOperatorName and replace both existing "etcd" literals in the relevant test calls with this constant.
948-974: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark both remediation helpers with
GinkgoHelper()Replace both
ExpectWithOffset(1, ...)calls withExpect(...). Ginkgo v2.28.3 supportsGinkgoHelper()and skips nested helper frames when reporting failures.🤖 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 948 - 974, Add GinkgoHelper() at the start of both waitForRemediation and waitForRemediationWithTimeouts, then replace each ExpectWithOffset(1, ...) call with Expect(...). Preserve the existing assertions, messages, and timeout behavior.
🤖 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.
Nitpick comments:
In `@tests/far-operator/tests/far_destructive.go`:
- Around line 736-765: Extract the duplicated FAR Succeeded-condition polling
logic into a shared expectFARSucceeded helper, and replace the inline scan at
this location and the corresponding checks near the other referenced call sites
with helper calls. Keep the API lookup, status.conditions validation, timeout,
and polling behavior unchanged, and use a collection assertion such as
ContainElement to verify the condition rather than
assertion.Expect(false).To(BeTrue()).
- Around line 693-698: Define the etcd ClusterOperator name as
farparams.EtcdClusterOperatorName and replace both existing "etcd" literals in
the relevant test calls with this constant.
- Around line 948-974: Add GinkgoHelper() at the start of both
waitForRemediation and waitForRemediationWithTimeouts, then replace each
ExpectWithOffset(1, ...) call with Expect(...). Preserve the existing
assertions, messages, and timeout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a945a037-b1fc-4715-ab0a-cbc661fe7e71
📒 Files selected for processing (1)
tests/far-operator/tests/far_destructive.go
|
/test 4.22-konflux-e2e-far-aws |
507287a to
1fcbb89
Compare
|
/test 4.22-konflux-e2e-far-aws |
1fcbb89 to
378bce0
Compare
|
/test 4.22-konflux-e2e-far-aws |
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
Nice work but I think you neglected the Leader-on-fenced-node (leader election failover path) of #22 (comment) from FAR test cases
13357d3 to
ccc066c
Compare
|
/test 4.22-konflux-e2e-far-aws |
3428f8d to
f550595
Compare
|
/test 4.22-konflux-e2e-far-aws |
ugreener
changed the title
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
some more changes.Please add a new commit after you adress code review for easier reviewer distinctiction of changes. When there are many commits and many rounds, then we can squash them...
2ec068a to
94c1e40
Compare
|
/test 4.22-konflux-e2e-far-aws |
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
Few more small changes.
Nice progress 👍🏻
|
|
||
| By("Verifying at least 1 FAR replica remains Running during degraded capacity") | ||
|
|
||
| Eventually(func() int32 { |
There was a problem hiding this comment.
The Eventually(ReadyReplicas >= 1) assertion passes trivially on the first poll: at this point the FAR CR was just created and the fence agent hasn't executed — both replicas are still healthy. This proves nothing about degraded-capacity behavior.
To meaningfully verify survival during degraded capacity, first wait for the leader pod to become NotReady (confirming degraded state entered), then assert ReadyReplicas >= 1.
There was a problem hiding this comment.
Fixed in HEAD b05513a (far_destructive.go:948).
The original Eventually(ReadyReplicas >= 1) ran immediately after createFARCR while both replicas were still Ready, so it was satisfied on the first poll before any fencing (Gomega returns on the first passing poll) and never observed degraded capacity.
The fix gates the survival check on the leader replica actually dropping, then verifies survival across the degraded window:
Eventually(readyReplicas).Should(BeNumerically("<", farparams.ExpectedReplicas))waits untilDeployment.Status.ReadyReplicasdrops below 2 (the leader pod no longer counts as Ready). I gate on the deployment-levelReadyReplicasrather than the leader node's Ready condition because the node flips to NotReady before the pod on it stops counting as Ready, so a node-Ready gate would let the survival check start whileReadyReplicasis still 2, i.e. the same trivial pass, just later.Consistently(readyReplicas, farparams.ControllerHandoverTimeout).Should(BeNumerically(">=", 1))then verifies at least the survivor stays Ready throughout the degraded window.
Kept >= 1 (not == 1) so a fast node recovery back to 2 replicas inside the window does not false-fail; the property under test is "never collapses to 0". The < ExpectedReplicas gate is durably observable because the 2-worker topology plus anti-affinity keeps the replacement replica Pending, so ReadyReplicas stays at 1 for the reboot window (noted in a code comment). A transient deployment.Pull error returns ExpectedReplicas, so it neither false-detects a drop nor fails the survival check.
| BeNumerically(">=", 1), | ||
| "FAR should have at least 1 Ready replica during degraded capacity") | ||
|
|
||
| waitForRemediation(ctx, APIClient, targetNode.Name, oldBootID) |
There was a problem hiding this comment.
Missing two assertions that exist in parallel tests:
(a) FAR CR Succeeded condition — the CP test (line 736) and standalone tests both verify Succeeded=True. Without this, a stuck-Processing CR after failover would pass silently.
(b) Lease transfer verification — the existing leader test (OCP-70638) checks HolderIdentity changed to a different pod. Since this test is specifically about leader failover under constrained capacity, confirming the lease actually transferred to the survivor would strengthen coverage.
There was a problem hiding this comment.
Fixed in HEAD 8ff2519. Added both assertions, mirroring the sibling patterns:
(a) FAR CR Succeeded condition (far_destructive.go:1015): after remediation, an Eventually asserts Processing=False, FenceAgentActionSucceeded=True, Succeeded=True on the CR, using the same condition-map pattern as the standalone (OCP-67015) and CP tests. A CR stuck in Processing after failover now fails instead of passing silently.
(b) Lease transfer (far_destructive.go:925 record, :978 verify): the pre-reboot lease HolderIdentity is recorded before the FAR CR is created, and the transfer is verified DURING the degraded window (after the survival check, before waitForRemediation) rather than after recovery. Placing it while the fenced leader is still down means the only other controller host is the survivor, so a changed holder here proves leadership actually moved TO the survivor, not merely that the old pod identity is gone (which a post-recovery check would also accept if the rebooted original node re-acquired the lease). Mirrors OCP-70638's HolderIdentity check (far_destructive.go:574-619).
Both blocks reuse the existing inline patterns; I can extract the shared condition-check and lease-transfer logic into helpers across the standalone/CP/OCP-70638 tests in a follow-up dedup if you prefer, but kept this change scoped to the 2-worker test.
|
|
||
| waitForRemediationWithTimeouts(ctx, APIClient, targetNode.Name, oldBootID, | ||
| farparams.CPRebootTimeout, farparams.CPNodeReadyTimeout) | ||
|
|
There was a problem hiding this comment.
Two fidelity gaps vs. existing FAR test patterns:
(a) No workload pod / eviction check. The standalone worker test and 2-worker test both create a pinned pod and verify eviction. The CP test fences a control plane node via the fence agent, which triggers the OutOfServiceTaint flow. That flow should evict pods from the fenced node. Without a workload pod on the CP node, the test only proves the node reboots and etcd recovers — it doesn’t prove that the pod eviction machinery works on control plane nodes (which have different taints/tolerations than workers). Adding this confirms pods on CP nodes are properly evicted during fencing — matching the source repo’s checkPodDeleted pattern.
(b) No CreationTimestamp check. The standalone worker test (OCP-61229, ~line 386) verifies the timestamp didn’t change to confirm reboot, not node recreation. Boot ID is the primary reboot signal (already verified via waitForRemediationWithTimeouts), but CreationTimestamp is a complementary one-liner that catches a silently destructive scenario: the cloud provider’s MachineSet controller recreating the Node object instead of the fence agent rebooting it. For CP nodes, recreation is especially dangerous (etcd membership, node-specific certificates).
There was a problem hiding this comment.
Fixed in HEAD 283ca4a:
(a) Workload pod / eviction check: Added createWorkloadPod before the FAR CR (far_destructive.go:721) and an eviction check after node recovery (far_destructive.go:733), matching the workload/eviction pattern in the standalone worker and 2-worker tests. createWorkloadPod assigns the pod via spec.NodeName, which bypasses the scheduler (where NoSchedule taints are enforced); NoSchedule does not trigger taint-based eviction of running pods, so the pod is admitted on the CP node. The eviction is triggered by the fencing reboot.
(b) Event verification: The event gap was already addressed in an earlier commit. RemediationStarted, RemediationFinished, and NodeRemediationCompleted are verified. FenceAgentSucceeded is intentionally not asserted as an event on CP targets: it can be lost during the control-plane disruption window because events are best-effort and may not be persisted when the apiserver/etcd member on the fenced node is disrupted. It is covered instead by the durable FenceAgentActionSucceeded status condition (see the comment at far_destructive.go:806).
|
|
||
| By("Verifying etcd ClusterOperator is healthy before remediation") | ||
|
|
||
| Expect(farutils.WaitForClusterOperatorHealthy( |
There was a problem hiding this comment.
Pre-remediation etcd health check uses EtcdRejoinTimeout (10 min) — the post-recovery timeout. A precondition check should fail fast rather than waiting 10 minutes hoping etcd self-heals. Consider using a shorter timeout (e.g., FARConditionTimeout = 2 min) for the pre-test validation.
There was a problem hiding this comment.
Fixed in HEAD 3f36221: the pre-remediation etcd check at far_destructive.go:699 now uses medik8sparams.DefaultTimeout (5 min) instead of farparams.EtcdRejoinTimeout (10 min). When etcd is already healthy, WaitForClusterOperatorHealthy returns on the first healthy poll — the timeout only matters as a failure ceiling. There is no reason to allow the full 10-minute recovery budget before declaring a pre-existing degraded state; 5 min is appropriate for a precondition check. The EtcdRejoinTimeout (10 min) is retained for the post-remediation check at line 732, where the wait must cover CP node reboot and etcd quorum re-establishment.
| By("Verifying minimum Ready worker nodes for destructive tests") | ||
|
|
||
| workerCount, err := helpers.CountReadyWorkerNodes(ctx, APIClient) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Hard Expect fails the suite on clusters with < 3 workers. The new CP test (line 688) and 2-worker test (line 815) both use Skip() for equivalent topology checks. Use the same pattern here for CI-friendliness:
if workerCount < farparams.MinWorkersForDestructiveTests {
Skip(fmt.Sprintf("...", farparams.MinWorkersForDestructiveTests, workerCount))
}There was a problem hiding this comment.
Fixed in HEAD ff364f0: replaced the hard worker-count Expect with Skip() in the standalone destructive BeforeEach. The guard at far_destructive.go:298 now calls Skip(fmt.Sprintf("Standalone destructive tests require at least %d Ready worker nodes, found %d", farparams.MinWorkersForDestructiveTests, workerCount)) when worker count is insufficient, matching the CP test's guard pattern at line 688.
This BeforeEach is scoped to the "Standalone FAR remediation" context only, so the skip does not affect the CP or 2-worker topology tests which have their own independent guards.
| farutils.UncordonNodes(ctx, APIClient, cordonedNodes, GinkgoWriter.Printf) | ||
| } | ||
| }) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Same pattern: Expect(cordonedNodes).ToNot(BeEmpty()) will hard-fail the suite if there are no Ready workers. Check worker count upfront and Skip() if none exist, consistent with the CP and 2-worker tests.
There was a problem hiding this comment.
Fixed in HEAD e6a8b93. Added an upfront Ready-worker count check that Skip()s when there are none to cordon, matching the CP (MinControlPlaneNodes) and 2-worker (MinWorkersForTwoWorkerTest) topology guards. I also converted the Expect(cordonedNodes).ToNot(BeEmpty()) line you flagged into a Skip() (far_destructive.go:1135): with the upfront count guaranteeing at least 1 worker, an empty result there can only mean the topology changed between the count and the cordon (a worker went NotReady), which is a Skip condition, not a suite failure. There is no longer any hard-fail on an empty cordon result.
| ctx context.Context, k8sClient client.Client, | ||
| nodeName, oldBootID string, | ||
| ) { | ||
| waitForRemediationWithTimeouts(ctx, k8sClient, nodeName, oldBootID, |
There was a problem hiding this comment.
ExpectWithOffset(1) in waitForRemediationWithTimeouts is correct when called directly (e.g., CP test at line 724), but off-by-one when called through the waitForRemediation wrapper (extra stack frame). The 6 callers via the wrapper (lines 382, 456, 470, 538, 591, 904) all get the wrong failure location — CI output will point to far_destructive.go:1091 instead of the It block.
Simplest fix — accept a variadic offset so existing callers don't change:
func waitForRemediationWithTimeouts(
ctx context.Context, k8sClient client.Client,
nodeName, oldBootID string,
rebootTimeout, readyTimeout time.Duration,
callerOffset ...int,
) {
offset := 1
if len(callerOffset) > 0 {
offset = callerOffset[0]
}
By("Waiting for node to reboot")
ExpectWithOffset(offset, farutils.WaitForNodeReboot(...)).To(Succeed(), ...)
By("Waiting for node to become Ready")
ExpectWithOffset(offset, farutils.WaitForNodeReady(...)).To(Succeed(), ...)
}
func waitForRemediation(
ctx context.Context, k8sClient client.Client,
nodeName, oldBootID string,
) {
waitForRemediationWithTimeouts(ctx, k8sClient, nodeName, oldBootID,
farparams.NodeRebootTimeout, farparams.NodeReadyTimeout, 2)
}The wrapper passes 2 (skip itself + waitForRemediationWithTimeouts); direct callers pass nothing and get the default 1.
There was a problem hiding this comment.
Fixed in HEAD e6a8b93. The off-by-one is real: waitForRemediation adds a stack frame, so ExpectWithOffset(1) in waitForRemediationWithTimeouts pointed at the wrapper (far_destructive.go:1134 in your comment) for the 6 wrapper callers. Rather than thread a manual callerOffset, I marked both waitForRemediation and waitForRemediationWithTimeouts with GinkgoHelper() and switched to plain Expect (far_destructive.go:1258 and :1273). GinkgoHelper() skips all helper-marked frames when reporting the failure location, so failures are attributed to the calling It for both the direct caller (CP test) and the wrapper callers, and it stays correct if another wrapper layer is ever added (no offset to keep in sync). It is already the convention in this repo (e.g. far-operator/tests/upgrade.go, nhc-operator helpers). createWorkloadPod's ExpectWithOffset(1) is called directly from It blocks, so it is correct as-is and left unchanged.
|
|
||
| return dep.Object.Status.ReadyReplicas | ||
| }, medik8sparams.DefaultTimeout, farparams.DefaultPollInterval).Should( | ||
| BeNumerically(">=", 2), |
There was a problem hiding this comment.
Hardcoded 2 — use farparams.ExpectedReplicas (const.go:19) for consistency with post-deployment tests in far.go. Same at line 1021 in the 0-worker test.
There was a problem hiding this comment.
Fixed in HEAD e6a8b93. Both recovery assertions now use farparams.ExpectedReplicas (const.go:19, int32(2)) instead of the literal 2, in both the matcher and the message: far_destructive.go:1087 (2-worker recovery) and far_destructive.go:1188 (0-worker recovery). The 0-worker occurrence is at line 1188, not 1021 - the branch moved since your comment.
| // extra worker(s). On a cluster with exactly 2 workers there is nothing to | ||
| // uncordon and full recovery only completes after CR deletion; this test | ||
| // targets the 3+-worker CI topology. | ||
| if len(cordonedNodes) > 0 { |
There was a problem hiding this comment.
After inline uncordon, cordonedNodes is not set to nil, so the DeferCleanup safety net will re-run UncordonNodes on already-uncordoned nodes. While UncordonNodes is idempotent, the 0-worker test (line 1008) does cordonedNodes = nil after inline uncordon. Align both for consistency and to avoid unnecessary API calls during cleanup.
There was a problem hiding this comment.
Fixed in HEAD 8ff2519 (far_destructive.go:1072). Added cordonedNodes = nil right after the inline UncordonNodes call, so the DeferCleanup safety net (guarded by len(cordonedNodes) > 0) skips re-uncordoning already-restored nodes, matching the 0-worker test. UncordonNodes is idempotent, so this is a consistency/efficiency fix, not a correctness change.
| } | ||
| } | ||
|
|
||
| assertion.Expect(false).To(BeTrue(), |
There was a problem hiding this comment.
Expect(false).To(BeTrue(), msg) as a fallthrough failure inside Eventually is unconventional in this codebase. Consider tracking a found boolean and asserting after the loop, matching the pattern used elsewhere in this file.
There was a problem hiding this comment.
The Expect(false).To(BeTrue(), msg) fallthrough is not present in far_destructive.go at HEAD e6a8b93. The file uses the found boolean pattern (asserted after the loop) that you referenced, e.g. far_destructive.go:442-447. No code change needed.
64f0458 to
8b51a8b
Compare
Add two non-happy-path FAR destructive test scenarios (RHWA-1284): 1. Control plane node remediation (OCP-90217): Fences a CP node via fence_aws, verifies etcd quorum holds (ClusterOperator health check), and confirms the node reboots and rejoins. 2. Minimal 2-worker topology (OCP-90218): Cordons extra workers to simulate a 2-worker cluster, fences one worker, verifies FAR completes remediation despite degraded capacity (at least 1 FAR replica stays Running), and confirms full recovery. Supporting changes: - Extend BuildAWSNodeParameters to include CP nodes (was worker-only) - Add GetReadyControlPlaneNodes, SelectControlPlaneNode helpers - Add CordonExtraWorkers, UncordonNodes helpers for topology simulation - Add shared WaitForClusterOperatorHealthy helper - Add topology labels (TopologyControlPlane, TopologyMinimalWorker) - Move >= 3 worker guard from shared BeforeEach to per-Context level Co-Authored-By: Claude <noreply@anthropic.com>
- 2-worker (90218): uncordon the extra worker(s) before asserting FAR recovers to 2 replicas. The fenced node stays NoSchedule-tainted until its CR is deleted (JustAfterEach) and the controller's hard topologySpread (whenUnsatisfiable=DoNotSchedule) needs a second untainted schedulable host, so the assertion was unsatisfiable while the extra worker stayed cordoned (pod Pending -> 300s timeout). - CP remediation (90217) and the other event checks: verify lifecycle events with a longer, less rate-limited window (EventVerifyTimeout 5m / EventVerifyInterval 10s) so WaitForEvents does not exhaust the shared client's rate limiter before the events land. The FAR CR Succeeded condition remains the authoritative pass gate. - controller-lifecycle (70636): retry the leader-pod lookup until the leader-election Lease resolves to a live Running pod, tolerating the stale Lease a preceding destructive spec leaves after churning FAR pods.
8b51a8b to
f00539d
Compare
b05513a to
8ff2519
Compare
Reply-round: true
8ff2519 to
e6a8b93
Compare
|
/test 4.22-konflux-e2e-far-aws |
… spec The control-plane remediation spec (test_id 90217) asserted FAR lifecycle Events (RemediationStarted/RemediationFinished on the CR, NodeRemediationCompleted on the Node). Kubernetes Events are best-effort and get dropped when the CP reboot briefly disrupts apiserver/etcd (quorum 3->2 write stall), so the spec flaked non-deterministically on whichever Event was lost (FenceAgentSucceeded, then RemediationFinished on a later run). The remediation itself succeeds every run and is already proven by the durable FAR CR status conditions (Processing=False, FenceAgentActionSucceeded=True, Succeeded=True) plus observable cluster state (boot-ID change, node Ready, workload eviction, etcd recovery), so drop the Event assertions from the control-plane spec only. The worker specs keep the full Event bundle, where fencing does not disrupt the control plane. README test 17 and Polarion OCP-90217 updated to match.
Fix: stop asserting best-effort Kubernetes Events on the control-plane remediation specSymptom: The specific missing event varied between runs: Root cause (test-design bug, not a FAR bug): The remediation itself succeeds every run. The CP node reboots (boot ID changes), rejoins Kubernetes Events are best-effort: client-go's broadcaster drops them when its queue is full ( The earlier fix moved Change: Removed both event-verification steps from the control-plane spec only, the CR events and the Node No FAR product change: Event delivery is best-effort by design; the CR status conditions are the correct contract for verifying remediation. |
|
/test 4.22-konflux-e2e-far-aws |
2 participants
Problem
The FAR destructive test suite only covers happy-path worker-node remediation. Two non-happy-path scenarios identified during PR #22 review (by razo7) are missing: fencing a control plane node (where etcd quorum must be preserved), and remediating with only 2 schedulable workers (where FAR anti-affinity constraints may cause a Pending replica).
Summary
Add 3 new FAR destructive test Contexts: control plane remediation, minimal 2-worker leader failover topology, and 0-worker topology (negative test), with the required helpers and constants.
Changes
BuildAWSNodeParametersto include CP nodes (was worker-only, blocking CP fencing)GetReadyControlPlaneNodes,SelectControlPlaneNodehelpersCordonExtraWorkers,UncordonNodeshelpers withTestCordonAnnotationtrackingWaitForClusterOperatorHealthyhelper (reusable by SNR/NHC)TopologyControlPlane,TopologyMinimalWorker,TopologyZeroWorkerlabels>= 3 workerguard from sharedBeforeEachto per-Context level (was blocking the 2-worker Context)Jira: RHWA-1284
Summary by CodeRabbit
New Features
Improvements