Add NHC operator Ginkgo suite with post-deployment smoke tests

[ugreener]

Summary

Add the NHC operator Ginkgo test suite with four post-deployment smoke tests, following the established SNR/SBR pattern.

Changes

• Create nhc_suite_test.go with JUnit and reportxml reporting (mirrors snr_suite_test.go)
• Create tests/post_deployment.go with four smoke specs:
  • OCP-89629: Verify NHC resources are installed and running (CRD listable, controller pods ready)
  • OCP-89630: Verify NHC CSV annotations (valid-subscription, support, repository, maintainers)
  • OCP-89631: Verify NHC CSV metadata (feature annotations, replaces field, replica count with SNO skip)
  • OCP-89632: Verify NHC container runs as non-root user (pod RunAsNonRoot, manager container restricted security posture)
• Extend internal/nhcparams/const.go with NHC-specific constants (CRD group, CSV pattern, timeouts, ManagerContainerName)
• Extend internal/nhcparams/nhcvars.go with reporter config, deployment name, pod label selector, and required annotations
• Add filterRunningPods helper for security context test (follows per-suite duplication pattern from SBR/FAR/NMO)

Jira: RHWA-1142

Summary by CodeRabbit

• New Features

  • Added end-to-end validation for the NHC and SBR operators, including deployment health, configuration metadata, and recovery workflows.
  • Added split-brain storage arbitration coverage with node recovery and reboot verification.

• Bug Fixes

  • Improved watchdog device discovery so inventory scanning correctly finds all device paths.
  • Strengthened cleanup and reporting so failed test runs leave clearer artifacts and fewer leftover resources.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds NHC post-deployment coverage and expands SBR system tests with remediation, split-brain, and watchdog parsing changes.

Changes

NHC Operator Post-Deployment Tests

Layer / File(s)	Summary
NHC parameters and reporter config tests/nhc-operator/internal/nhcparams/const.go, tests/nhc-operator/internal/nhcparams/nhcvars.go	Adds NHC polling, CRD identity, operator selection, reporter dump targets, and required CSV annotations.
Suite entrypoint and reporting hooks tests/nhc-operator/nhc_suite_test.go	Defines TestNHC and adds failure dump and XML report hooks.
Post-deployment CSV, CRD, and security checks tests/nhc-operator/tests/post_deployment.go	Sets up the suite, waits for a succeeded CSV, verifies NodeHealthCheck listability, checks CSV annotations and metadata, validates replica counts, and inspects controller pod security settings.

SBR Operator System Tests

Layer / File(s)	Summary
SBR constants and test identifiers tests/sbr-operator/internal/sbrparams/const.go	Adds SBR and split-brain constants for CRD identity, timing, names, and remediation expectations.
Shared SBR helpers and watchdog parsing tests/sbr-operator/tests/sbr.go, tests/sbr-operator/tests/watchdog.go	Updates watchdog device parsing, adds node schedulability checks, builds shared SBR unstructured objects, and adds RWX storage discovery plus SBRC readiness polling.
Remediation lifecycle test tests/sbr-operator/tests/remediation.go	Adds the remediation functional suite with SBRC setup, target selection, finalizer validation, deletion, and cleanup.
Split-brain scenario test tests/sbr-operator/tests/split_brain.go	Defines the split-brain suite, injects network failures, observes node state changes, and performs cleanup for SBRC, NHC, pods, and iptables rules.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• medik8s/system-tests#13: Adds similar operator test-suite plumbing with shared params, reporter dumping, and a Ginkgo suite entrypoint.

Suggested reviewers

• razo7
• maximunited

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% 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
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.
Title check	✅ Passed	The title clearly matches the main change: adding an NHC operator Ginkgo suite with post-deployment smoke tests.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/nhc-operator/tests/post_deployment.go (1)

78-90: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Remove unused unstructured object declarations.

The nhcCRD (lines 78-83) and crdObj (lines 85-90) variables are declared but never used. Only nhcList (lines 92-97) is actually used in the test. Removing the unused declarations will improve code clarity.

♻️ Suggested cleanup

-			nhcCRD := &unstructured.Unstructured{}
-			nhcCRD.SetGroupVersionKind(schema.GroupVersionKind{
-				Group:   "apiextensions.k8s.io",
-				Version: "v1",
-				Kind:    "CustomResourceDefinition",
-			})
-
-			crdObj := &unstructured.Unstructured{}
-			crdObj.SetGroupVersionKind(schema.GroupVersionKind{
-				Group:   nhcparams.CRDGroup,
-				Version: nhcparams.CRDVersion,
-				Kind:    "NodeHealthCheck",
-			})
-
 			nhcList := &unstructured.UnstructuredList{}
 			nhcList.SetGroupVersionKind(schema.GroupVersionKind{
 				Group:   nhcparams.CRDGroup,

🤖 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/nhc-operator/tests/post_deployment.go` around lines 78 - 90, Remove the
unused variable declarations in the post_deployment test. The nhcCRD variable
(declared with GroupVersionKind for apiextensions.k8s.io
CustomResourceDefinition) and the crdObj variable (declared with
GroupVersionKind for NodeHealthCheck) are both declared but never used anywhere
in the test logic. Delete both of these unstructured object declaration blocks,
keeping only the nhcList variable which is actually utilized in the test.

🤖 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/nhc-operator/tests/post_deployment.go`:
- Around line 78-90: Remove the unused variable declarations in the
post_deployment test. The nhcCRD variable (declared with GroupVersionKind for
apiextensions.k8s.io CustomResourceDefinition) and the crdObj variable (declared
with GroupVersionKind for NodeHealthCheck) are both declared but never used
anywhere in the test logic. Delete both of these unstructured object declaration
blocks, keeping only the nhcList variable which is actually utilized in the
test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1de87b48-f139-4c73-acb1-ddcc111da616

📥 Commits

Reviewing files that changed from the base of the PR and between f7cf698 and a53eefb.

📒 Files selected for processing (4)

• tests/nhc-operator/internal/nhcparams/const.go
• tests/nhc-operator/internal/nhcparams/nhcvars.go
• tests/nhc-operator/nhc_suite_test.go
• tests/nhc-operator/tests/post_deployment.go

[ugreener]

/test pull-ci-medik8s-system-tests-main-4.22-konflux-e2e-nhc-aws

[ugreener]

/test 4.22-konflux-e2e-nhc-aws

[ugreener]

/test 4.22-konflux-e2e-nhc-aws

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/nhc-operator/tests/post_deployment.go (1)

118-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Use explicit Eventually(... timeout, poll) for pod readiness waits in specs.

Both waits currently route through pod.WaitForAllPodsInNamespaceRunning(...), so poll cadence is not explicit in the spec body. The test guideline requires explicit timeout + poll interval from params in each wait path.

Proposed change pattern

- _, err = pod.WaitForAllPodsInNamespaceRunning(
- 	APIClient,
- 	medik8sparams.OperatorNs,
- 	medik8sparams.DefaultTimeout,
- 	ctrlListOptions,
- )
- Expect(err).ToNot(HaveOccurred(), "NHC controller pods are not running")
+ Eventually(func() error {
+ 	pods, listErr := pod.List(APIClient, medik8sparams.OperatorNs, ctrlListOptions)
+ 	if listErr != nil {
+ 		return listErr
+ 	}
+ 	if len(pods) == 0 {
+ 		return fmt.Errorf("no NHC controller pods found")
+ 	}
+ 	if len(filterRunningPods(pods)) != len(pods) {
+ 		return fmt.Errorf("not all NHC controller pods are running and ready")
+ 	}
+ 	return nil
+ }, medik8sparams.DefaultTimeout, nhcparams.DefaultPollInterval).Should(Succeed(),
+ 	"NHC controller pods are not running")

Apply the same pattern to the wait in the non-root test block.

As per coding guidelines, Use Eventually/Consistently with explicit timeout + poll interval from <op>params/medik8sparams — never use time.Sleep.

Also applies to: 261-266

🤖 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/nhc-operator/tests/post_deployment.go` around lines 118 - 123, The pod
readiness wait in the spec is routed through
pod.WaitForAllPodsInNamespaceRunning, which hides the polling cadence from the
test body; replace it with an explicit Eventually-based wait using the timeout
and poll interval from medik8sparams in this block, and apply the same pattern
to the non-root test wait as well. Keep the wait logic in the relevant
post_deployment test cases and ensure the readiness condition is expressed
directly with explicit timeout + poll rather than via the helper.

Source: Coding guidelines

🧹 Nitpick comments (1)

tests/sbr-operator/tests/watchdog.go (1)

128-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant empty-token guard.

strings.Fields never returns empty tokens, so the if token == "" check is dead code. The equivalent parsing in sbr.go (Lines 126-128) already omits it; aligning the two keeps the discovery logic consistent.

♻️ Proposed simplification

-				for _, token := range strings.Fields(line) {
-					if token == "" {
-						continue
-					}
-
-					devices = append(devices, strings.TrimPrefix(token, "/proc/1/root"))
-				}
+				for _, token := range strings.Fields(line) {
+					devices = append(devices, strings.TrimPrefix(token, "/proc/1/root"))
+				}

🤖 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/sbr-operator/tests/watchdog.go` around lines 128 - 134, The token loop
in the watchdog parsing logic has a redundant empty-string check because
strings.Fields never returns empty tokens. Remove the dead if token == "" guard
from the loop in the watchdog test helper so it matches the parsing approach
used in sbr.go, and keep the device extraction logic centered on strings.Fields
and strings.TrimPrefix.

🤖 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/nhc-operator/tests/post_deployment.go`:
- Around line 405-416: The filterRunningPods readiness check is treating pods
with an empty ContainerStatuses list as ready because allReady starts true and
never flips. Update filterRunningPods so it only appends a candidate when it has
at least one ContainerStatus and every status is Ready, using the existing
candidate.Object.Status.ContainerStatuses loop to guard against empty slices and
false-positive readiness.

---

Outside diff comments:
In `@tests/nhc-operator/tests/post_deployment.go`:
- Around line 118-123: The pod readiness wait in the spec is routed through
pod.WaitForAllPodsInNamespaceRunning, which hides the polling cadence from the
test body; replace it with an explicit Eventually-based wait using the timeout
and poll interval from medik8sparams in this block, and apply the same pattern
to the non-root test wait as well. Keep the wait logic in the relevant
post_deployment test cases and ensure the readiness condition is expressed
directly with explicit timeout + poll rather than via the helper.

---

Nitpick comments:
In `@tests/sbr-operator/tests/watchdog.go`:
- Around line 128-134: The token loop in the watchdog parsing logic has a
redundant empty-string check because strings.Fields never returns empty tokens.
Remove the dead if token == "" guard from the loop in the watchdog test helper
so it matches the parsing approach used in sbr.go, and keep the device
extraction logic centered on strings.Fields and strings.TrimPrefix.

🪄 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: 4ccb63fa-4b4b-453b-b370-68f058513c5a

📥 Commits

Reviewing files that changed from the base of the PR and between f9e4cb7 and ef0bfce.

📒 Files selected for processing (7)

• tests/nhc-operator/internal/nhcparams/const.go
• tests/nhc-operator/tests/post_deployment.go
• tests/sbr-operator/internal/sbrparams/const.go
• tests/sbr-operator/tests/remediation.go
• tests/sbr-operator/tests/sbr.go
• tests/sbr-operator/tests/split_brain.go
• tests/sbr-operator/tests/watchdog.go

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

• tests/nhc-operator/internal/nhcparams/const.go

[ugreener]

/test 4.22-konflux-e2e-nhc-aws

[razo7]

Can we avoid code duplication between operaotrs?

[razo7]

Duplicated helper — extract to tests/internal/

filterRunningPods() is copy-pasted verbatim in FAR (far.go), MDR (mdr.go), and NMO (nmo.go). All four implementations are identical — only the loop variable name changes (farPod/mdrPod/nmoPod/candidate).

Move this to a shared package (e.g. tests/internal/helpers/pod_helpers.go) so all operators reuse one implementation.

[ugreener]

The duplication currently exists in FAR (far.go), SBR (sbr.go), and NHC (three copies total). MDR and NMO do not implement this pod-filtering logic - their suites are simpler.

I will extract FilterRunningPods into tests/internal/helpers/ and switch all six operator suites to use it in a follow-up PR (mechanical refactor, no behavior change).

[razo7]

follow-up PR

Please link the new PR to this commit after it is created

[razo7]

Security context validation block is duplicated across all operators

This ~80-line block (runAsNonRoot → AllowPrivilegeEscalation → Capabilities → ReadOnlyRootFilesystem → SeccompProfile) is nearly identical in FAR, MDR, and NMO tests.

Consider extracting a shared helper like:

func ValidateNonRootSecurityContext(pods []*pod.Builder, containerName string) []string

This would return the error messages list, letting each operator's It block just call it and Fail() if non-empty.

[ugreener]

Agreed on extracting this. The validation block currently exists only in FAR (far.go), SBR (sbr.go), and NHC. MDR and NMO do not have security context checks yet.

One detail to preserve: SBR does not validate ReadOnlyRootFilesystem, while FAR and NHC do. The shared helper should make that difference explicit via a parameter rather than hiding it in duplicated logic.

This will be part of the same follow-up refactoring PR as filterRunningPods, extracting shared test helpers into tests/internal/helpers/.

[razo7]

follow-up PR

Please link the new PR to this commit after it is created

[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