SBR | detectOnlyMode test (OCP-88876)

[maximunited]

Summary

• Add detect_only.go: Ginkgo test suite for SBRC detectOnlyMode field (OCP-88876)
• Verify Enabled mode is admitted and reflected in SBRC spec
• Verify no StorageBasedRemediation CRs are auto-created while mode is Enabled (30s Consistently window)
• Verify mode can be toggled back to Disabled via MergePatch with agent DaemonSet remaining ready
• Verify agent DaemonSet is garbage-collected after SBRC deletion
• Add SBRAgentDaemonSetPrefix and SBRCReadyTimeout constants to sbrparams
• Add waitForSBRCReady helper to sbr.go

Test plan

• make vet passes
• golangci-lint passes (0 issues)
• /test 4.22-konflux-e2e-sbr-aws-odf

Summary by CodeRabbit

• Tests

  • Added system tests for SBR detect-only mode validation to ensure the feature works as expected.
  • Added system tests for node hang and kernel freeze remediation scenarios.
  • Added SNR CRD validation tests covering schema descriptions and webhook enforcement.

• Documentation

  • Updated SNR operator test documentation with additional test coverage descriptions.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximunited

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 [maximunited]

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

[coderabbitai]

Warning

Review limit reached

@maximunited, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 12 minutes and 53 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

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

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e61bed90-b815-4a2d-a6a7-191de99174e0

📥 Commits

Reviewing files that changed from the base of the PR and between 0933adb and 36ec67c.

📒 Files selected for processing (6)

• tests/sbr-operator/internal/sbrparams/const.go
• tests/sbr-operator/tests/detect_only.go
• tests/sbr-operator/tests/node_hang.go
• tests/sbr-operator/tests/sbr.go
• tests/sbr-operator/tests/sbr_helpers.go
• tests/snr-operator/README.md

📝 Walkthrough

Walkthrough

Adds two new SBR operator Ginkgo test suites (detectOnlyMode validation and node-hang/NHC-based remediation), a new SNR operator CRD negative validation test suite, shared helper functions for both, exported constants/variables consumed by the new tests, and SNR README documentation for four new test scenarios.

Changes

SBR Operator Tests

Layer / File(s)	Summary
SBR constants and shared test helpers tests/sbr-operator/internal/sbrparams/const.go, tests/sbr-operator/tests/sbr_helpers.go	Adds timing/naming constants for DaemonSet GC, NHC API/CRD, node reboot, fencing, and cleanup; adds helper functions for CR fetch/cleanup, controller pod node discovery, RWX storage class discovery, boot ID retrieval, NHC CRD presence detection, and NHC CR cleanup.
waitForSBRCReady polling helper tests/sbr-operator/tests/sbr.go	Adds a polling helper that constructs the agent DaemonSet name and asserts NumberReady >= DesiredNumberScheduled within a configured timeout.
detectOnlyMode test suite tests/sbr-operator/tests/detect_only.go	Validates SBRC spec.detectOnlyMode: creates SBRC in Enabled mode, asserts no new SBR CRs appear, patches to Disabled and verifies field reflection, confirms DaemonSet readiness, deletes SBRC, and asserts agent DaemonSet GC.
Node hang / NHC integration test suite tests/sbr-operator/tests/node_hang.go	Validates SBR-based kernel-freeze remediation: selects a worker node, injects a kernel panic via privileged pod, waits for NHC to create a StorageBasedRemediation CR, asserts FencingSucceeded=True, waits for CR deletion, and confirms node recovery and boot-id change.

SNR Operator Tests

Layer / File(s)	Summary
SNR constants and variables tests/snr-operator/internal/snrparams/const.go, tests/snr-operator/internal/snrparams/snrvars.go	Adds exported constants for SNRC CRD name, description substring, webhook error message, fake node name, and test SNRC name; adds SNRLastErrorNodeNotFound formatted from SNRTestNodeName.
SNR test helpers tests/snr-operator/tests/helpers.go	Adds buildSNRCR to construct unstructured SNR CRs with optional spec, and deferDeleteCR to register Ginkgo cleanup that polls deletion until success or NotFound.
SNR CRD negative validation test suite tests/snr-operator/tests/crd_negative.go	Adds four tests: CRD schema description check for safeTimeToAssumeNodeRebootedSeconds; webhook rejection of non-default SNRC creation; webhook rejection of invalid duration strings and too-small values with aggregate per-field assertions; and status.lastError population polling for a SelfNodeRemediation referencing a non-existent node.
SNR README documentation tests/snr-operator/README.md	Fills in subsections 5–8 documenting the four new test scenarios, their focus labels, and pass criteria.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• medik8s/system-tests#9: Extends the same tests/sbr-operator test infrastructure and sbrparams/const.go that this PR builds upon.
• medik8s/system-tests#10: Also adds constants to tests/sbr-operator/internal/sbrparams/const.go for SBR test-suite identifiers, directly overlapping with this PR's constant additions.

Suggested labels

lgtm

Suggested reviewers

• swgoswam
• mshitrit
• ugreener
• razo7

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'SBR | detectOnlyMode test (OCP-88876)' directly summarizes the main change: a new test for the detectOnlyMode functionality in StorageBasedRemediationConfig, with the issue reference for traceability.
Docstring Coverage	✅ Passed	Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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.}

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[qodo-2-for-medik8s]

PR Summary by Qodo

Add SBRC detectOnlyMode functional tests (OCP-88876)
🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

Description

• Add an ordered Ginkgo suite validating SBRC detectOnlyMode behavior.
• Assert no StorageBasedRemediation CRs are auto-created while detect-only is enabled.
• Add shared constants and a readiness helper for SBRC agent DaemonSet gating.

Diagram

graph TD
  T["Ginkgo detectOnlyMode suite"] --> A["K8s API"] --> R("SBRC CR") --> C["SBRC controller"] --> D["Agent DaemonSet"]
  T --> A --> S("SBR CRs list")
  R --> A
  T --> A
  R --> C
  D --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Gate on SBRC status/conditions instead of DaemonSet readiness

• ➕ Less coupled to DaemonSet naming conventions and implementation details
• ➕ Better reflects the controller's declared readiness contract (if conditions exist)
• ➖ Requires SBRC status conditions to be reliably populated and stable across versions
• ➖ May not guarantee agents are actually running if status is optimistic

2. Use typed clientsets for SBR/SBRC resources instead of unstructured

• ➕ Compile-time safety and clearer field access
• ➕ Easier refactors when APIs evolve
• ➖ Adds dependency on generated types and API packages in the system-tests repo
• ➖ May complicate tests if multiple operator versions/CRD shapes are supported

Recommendation: The PR’s approach is sound for a black-box functional suite: it validates admission, side-effects, and cleanup without depending on internal controller code. If SBRC exposes a stable readiness condition, consider switching waitForSBRCReady to that signal over DaemonSet name/ready-pod checks to reduce coupling; otherwise the current DaemonSet readiness gate is a pragmatic and test-relevant proxy.

Files changed (3) +280 / -0

Refactor (1) +23 / -0

sbr.goAdd helper to wait for SBRC agent DaemonSet readiness +23/-0

Add helper to wait for SBRC agent DaemonSet readiness

• Adds 'waitForSBRCReady' to block until the SBRC agent DaemonSet has at least one ready pod, using the new timeout and polling interval constants. This centralizes readiness gating for tests that depend on active agent pods.

tests/sbr-operator/tests/sbr.go

Tests (1) +249 / -0

detect_only.goNew ordered Ginkgo suite for SBRC detectOnlyMode behavior +249/-0

New ordered Ginkgo suite for SBRC detectOnlyMode behavior

• Adds a functional test that creates an SBRC with 'detectOnlyMode: Enabled', asserts the field is reflected in spec, and verifies no 'StorageBasedRemediation' CRs are created during a consistent observation window. The suite also patches the mode back to 'Disabled' while ensuring the agent DaemonSet remains ready, then deletes the SBRC and asserts the DaemonSet is garbage-collected.

tests/sbr-operator/tests/detect_only.go

Other (1) +8 / -0

const.goAdd SBRC agent DaemonSet prefix and readiness timeout constants +8/-0

Add SBRC agent DaemonSet prefix and readiness timeout constants

• Introduces 'SBRAgentDaemonSetPrefix' for deriving agent DaemonSet names and 'SBRCReadyTimeout' to bound readiness waits before functional assertions run.

tests/sbr-operator/internal/sbrparams/const.go

[qodo-2-for-medik8s]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

1. SBRC recreate race 🐞 Bug ☼ Reliability

Description

In detect_only.go, a stale StorageBasedRemediationConfig is deleted and immediately recreated with
the same name without waiting for deletion to complete, so Create can fail with AlreadyExists while
the old object is still terminating. This can make the suite flaky on slow clusters or after prior
runs left the SBRC in a finalizer-delayed deletion state.

Code

tests/sbr-operator/tests/detect_only.go[R41-60]

+		BeforeAll(func() {
+			By("Cleaning up any stale detectOnly StorageBasedRemediationConfig from a previous run")
+
+			stale := buildSBRC(sbrCDetectOnlyTestName, map[string]interface{}{})
+
+			deleteErr := APIClient.Delete(context.TODO(), stale)
+			if deleteErr != nil && !k8serrors.IsNotFound(deleteErr) {
+				GinkgoT().Logf("Warning: pre-test cleanup of %s failed: %v",
+					sbrCDetectOnlyTestName, deleteErr)
+			}
+
+			By("Creating StorageBasedRemediationConfig with detectOnlyMode: Enabled")
+
+			detectOnlySBRC = buildSBRC(sbrCDetectOnlyTestName, map[string]interface{}{
+				"detectOnlyMode": "Enabled",
+			})
+
+			createErr := APIClient.Create(context.TODO(), detectOnlySBRC)
+			Expect(createErr).ToNot(HaveOccurred(),
+				"StorageBasedRemediationConfig with detectOnlyMode: Enabled must be admitted by the API server")

Relevance

⭐⭐⭐ High

PR #11 added stale SBRC cleanup and waits for DaemonSet GC before proceeding; team hardens async
deletions.

PR-#11

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The detectOnly suite performs Delete→Create back-to-back for the same SBRC name, while other SBR
tests explicitly wait for dependent resources to be garbage-collected after deletions, demonstrating
cleanup is asynchronous and needs waiting.

tests/sbr-operator/tests/detect_only.go[41-65]
tests/sbr-operator/tests/sbr.go[659-704]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`BeforeAll` deletes a potentially-stale SBRC and then immediately creates a new SBRC with the same name. Because Kubernetes deletion is asynchronous (and may be delayed by finalizers), the subsequent `Create` can intermittently fail with `AlreadyExists`.

### Issue Context
The existing SBR suite already treats SBRC-related cleanup as asynchronous (it explicitly waits for stale DaemonSets to be GC’d before proceeding), but this new suite does not wait for SBRC deletion before recreating.

### Fix Focus Areas
- tests/sbr-operator/tests/detect_only.go[41-65]

### Suggested fix
After calling `Delete(stale)`, add an `Eventually` loop that `Get`s the SBRC by name/namespace until `IsNotFound(err)` is true (optionally also wait for the associated agent DaemonSet to disappear if you continue relying on deterministic DS naming), then proceed to `Create`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. DaemonSet readiness not awaited 🐞 Bug ☼ Reliability

Description

After patching detectOnlyMode to Disabled, the test does a single DaemonSet Get and immediately
asserts NumberReady>0, which can fail transiently during reconciliation/rollout even when behavior
is correct. This reintroduces rollout-timing flakiness the suite otherwise avoids by using
Eventually-based readiness waits.

Code

tests/sbr-operator/tests/detect_only.go[R195-205]

+				By("Verifying agent DaemonSet remains ready after toggling detectOnlyMode to Disabled")
+
+				dsName := sbrparams.SBRAgentDaemonSetPrefix + sbrCDetectOnlyTestName
+
+				agentDS, getDSErr := APIClient.DaemonSets(medik8sparams.OperatorNs).Get(
+					context.TODO(), dsName, metav1.GetOptions{})
+				Expect(getDSErr).ToNot(HaveOccurred(),
+					"Agent DaemonSet %q must still exist after detectOnlyMode toggle", dsName)
+				Expect(agentDS.Status.NumberReady).To(BeNumerically(">", 0),
+					"Agent DaemonSet %q must have at least one ready pod after detectOnlyMode toggle", dsName)
+			})

Relevance

⭐⭐⭐ High

Team previously accepted wrapping DaemonSet readiness checks in Eventually to avoid rollout flakes
(PR #13).

PR-#13

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The detectOnly test checks DaemonSet readiness only once, while the newly added helper uses
Eventually for this exact condition; past accepted review feedback in this repo also highlights
DaemonSet pod checks must be resilient to rollout timing.

tests/sbr-operator/tests/detect_only.go[195-205]
tests/sbr-operator/tests/sbr.go[631-651]
PR-#13

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test asserts DaemonSet readiness immediately after toggling `detectOnlyMode` back to `Disabled`. DaemonSet readiness is timing-sensitive; a single read can observe a transient `NumberReady==0` during rollout.

### Issue Context
A new helper `waitForSBRCReady` already exists and uses `Eventually` to wait until `NumberReady>0`.

### Fix Focus Areas
- tests/sbr-operator/tests/detect_only.go[159-205]

### Suggested fix
Replace the single-shot `Get` + `Expect(NumberReady>0)` with an `Eventually` that re-gets the DaemonSet and waits for `Status.NumberReady > 0` (or simply call `waitForSBRCReady(sbrCDetectOnlyTestName)` after the patch is confirmed).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Hardcoded DaemonSet naming 🐞 Bug ⚙ Maintainability

Description

waitForSBRCReady (and the detectOnly suite) assumes the agent DaemonSet name is exactly
SBRAgentDaemonSetPrefix+<sbrcName>, which tightly couples the tests to the controller’s internal
naming convention. If the operator ever changes this naming (e.g., adds a suffix/hash), these tests
will fail even if behavior remains correct.

Code

tests/sbr-operator/tests/sbr.go[R634-642]

+func waitForSBRCReady(sbrcName string) {
+	dsName := sbrparams.SBRAgentDaemonSetPrefix + sbrcName
+
+	Eventually(func() error {
+		agentDS, err := APIClient.DaemonSets(medik8sparams.OperatorNs).Get(
+			context.TODO(), dsName, metav1.GetOptions{})
+		if err != nil {
+			return fmt.Errorf("DaemonSet %s not found: %w", dsName, err)
+		}

Relevance

⭐⭐ Medium

No clear prior stance on hardcoding DaemonSet names; SBR tests often list/snapshot DSes, but
labelselector refactor was rejected.

PR-#10
PR-#11

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code constructs a deterministic DaemonSet name and uses Get-by-name, while existing SBR
tests use list/snapshot patterns that don’t depend on a naming convention, indicating the suite
previously avoided this coupling.

tests/sbr-operator/tests/sbr.go[634-642]
tests/sbr-operator/tests/detect_only.go[197-205]
tests/sbr-operator/tests/sbr.go[604-615]
tests/sbr-operator/tests/sbr.go[811-828]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new helper and tests rely on a hardcoded DaemonSet name construction. This makes tests brittle to naming changes in the operator.

### Issue Context
Other SBR tests avoid assuming DaemonSet names by listing/snapshotting DaemonSets and comparing sets.

### Fix Focus Areas
- tests/sbr-operator/tests/sbr.go[634-642]
- tests/sbr-operator/tests/detect_only.go[197-205]

### Suggested fix
Instead of `Get` by constructed name, locate the agent DaemonSet by listing DaemonSets and matching via stable identifiers (labels/ownerReferences pointing to the SBRC, or diffing against a baseline list taken immediately before SBRC creation). Optionally return the discovered DS name from a helper so later assertions reuse the same DS identity.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[maximunited]

Fixed: added an Eventually wait after the best-effort delete in BeforeAll that polls until IsNotFound returns true (timeout: SBRCReadyTimeout / poll: DefaultPollInterval). This prevents the AlreadyExists race where Kubernetes async deletion leaves the old SBRC still terminating when Create fires on a rerun.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf

[qodo-2-for-medik8s]

PR-Agent: could not find a component named 4.22-konflux-e2e-sbr-aws-odf in a supported language in this PR.

[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/sbr-operator/internal/sbrparams/const.go`:
- Line 133: The const.go file in the sbrparams package does not conform to Go
formatting standards. Run gofmt with the write flag on the const.go file to
automatically format it according to the official Go code style guidelines. This
will ensure consistent formatting throughout the file.

In `@tests/sbr-operator/tests/sbr.go`:
- Around line 631-633: The comment for the waitForSBRCReady function and the
assertion message both incorrectly state the function waits for "at least one
ready pod," but the actual implementation checks that NumberReady is greater
than or equal to DesiredNumberScheduled, meaning it waits for the entire
DaemonSet to be ready. Update both the function comment at the beginning of
waitForSBRCReady and the assertion message to accurately reflect that the
function blocks until all scheduled pods in the DaemonSet are ready, not just
one.

🪄 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: a079962f-dbe6-4b25-aad4-d2ea0a82bb8e

📥 Commits

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

📒 Files selected for processing (10)

• tests/sbr-operator/internal/sbrparams/const.go
• tests/sbr-operator/tests/detect_only.go
• tests/sbr-operator/tests/node_hang.go
• tests/sbr-operator/tests/sbr.go
• tests/sbr-operator/tests/sbr_helpers.go
• tests/snr-operator/README.md
• tests/snr-operator/internal/snrparams/const.go
• tests/snr-operator/internal/snrparams/snrvars.go
• tests/snr-operator/tests/crd_negative.go
• tests/snr-operator/tests/helpers.go

[maximunited]

/retest

[maximunited]

/retest

[maximunited]

/retest

[maximunited]

/retest

[maximunited]

/retest

[maximunited]

/test 4.22-konflux-e2e-sbr-aws-odf