Skip to content

e2e: Add ARM kernel boot parameter validation (iommu) #1539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oblau wants to merge 1 commit into
base: main
Choose a base branch
from
Open

e2e: Add ARM kernel boot parameter validation (iommu) #1539

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions test/e2e/performanceprofile/functests/14_arm/iommu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package __arm

import (
"context"

corev1 "k8s.io/api/core/v1"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

testutils "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/infrastructure"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/label"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/nodes"
)

// TODO: verify label.OpenShift is required — used by other 14_arm files but unclear what filters depend on it
// TODO: verify if label.SpecializedHardware should be added — other 14_arm files use it
Comment on lines +17 to +18

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check label usage patterns in other ARM test files

# Search for Label() calls in 14_arm directory
rg -n 'Label\(' test/e2e/performanceprofile/functests/14_arm/ -A 1

Repository: openshift/cluster-node-tuning-operator

Length of output: 1030

Remove/resolve the TODOs in test/e2e/performanceprofile/functests/14_arm/iommu.go

  • The TODO about label.OpenShift can be removed since iommu.go already includes label.OpenShift in its Describe(... Label(...)) block (unlike the other TODO).
  • The TODO about adding label.SpecializedHardware should be resolved by either adding label.SpecializedHardware to iommu.go’s Label(...) to match the established 14_arm pattern (used in kernelpagesize.go and hugepages.go), or replacing the TODO with a concrete rationale/issue reference.
🤖 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 `@test/e2e/performanceprofile/functests/14_arm/iommu.go` around lines 17 - 18,
Remove the redundant TODO about label.OpenShift (iommu.go already includes
label.OpenShift in its Describe(... Label(...)) block) and resolve the TODO
about label.SpecializedHardware by adding label.SpecializedHardware to the
Describe(... Label(...)) call in iommu.go to match 14_arm pattern (see
kernelpagesize.go/hugepages.go), or replace the TODO with a one-line rationale
or issue reference if you intentionally omit the label; update the
Describe/Label invocation accordingly.

var _ = Describe("kernel boot parameters validation on aarch64", Ordered, Label(string(label.OpenShift), string(label.ARM)), func() {
var (
workerRTNodes []corev1.Node
ctx context.Context
)

BeforeAll(func() {
ctx = context.Background()

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add timeout to context for command execution.

The background context has no timeout. If nodes.ExecCommand hangs when reading /proc/cmdline (e.g., node becomes unreachable), the test will block indefinitely, causing CI pipeline timeouts.

⏱️ Proposed fix to add timeout 
 	BeforeAll(func() {
-		ctx = context.Background()
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+		DeferCleanup(cancel)
 		var err error

Note: You'll need to add "time" to the imports.

Based on learnings: Ginkgo tests should have appropriate timeouts on cluster operations.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ctx = context.Background()
BeforeAll(func() {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
DeferCleanup(cancel)
var err error
🤖 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 `@test/e2e/performanceprofile/functests/14_arm/iommu.go` at line 26, Replace
the plain background context used for command execution with a cancellable
timeout context: change the ctx = context.Background() to something like ctx,
cancel := context.WithTimeout(context.Background(), 30*time.Second) and defer
cancel() so nodes.ExecCommand(...) uses a context that will time out if the
command hangs; also add the "time" import. Ensure the new ctx is the one passed
into nodes.ExecCommand and that cancel() is deferred immediately after creating
the timeout context.

var err error

workerRTNodes, err = nodes.GetByLabels(testutils.NodeSelectorLabels)
Expect(err).ToNot(HaveOccurred())

workerRTNodes, err = nodes.MatchingOptionalSelector(workerRTNodes)
Expect(err).ToNot(HaveOccurred())
Expect(workerRTNodes).ToNot(BeEmpty())

isArm, err := infrastructure.IsARM(ctx, &workerRTNodes[0])
Expect(err).ToNot(HaveOccurred())
if !isArm {
Skip("This test requires an aarch64 arm CPU")
}
})

// Regression test for OCPBUGS-58402
It("should not add iommu.passthrough to the kernel cmdline", func() {
cmdline, err := nodes.ExecCommand(ctx, &workerRTNodes[0], []string{"cat", "/proc/cmdline"})
Expect(err).ToNot(HaveOccurred())
Expect(string(cmdline)).NotTo(ContainSubstring("iommu.passthrough"))
})
Comment on lines +44 to +48

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check node validation patterns in other e2e tests

# Search for patterns where tests iterate over all nodes vs just first node
rg -n 'workerRTNodes\[0\]' test/e2e/performanceprofile/functests/ -B 2 -A 2
rg -n 'for.*range.*workerRTNodes' test/e2e/performanceprofile/functests/ -B 2 -A 2

Repository: openshift/cluster-node-tuning-operator

Length of output: 50394

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo_root="$(pwd)"

# Show relevant sections for ARM fun tests that use workerRTNodes[0]
for f in \
  test/e2e/performanceprofile/functests/14_arm/iommu.go \
  test/e2e/performanceprofile/functests/14_arm/kernelpagesize.go \
  test/e2e/performanceprofile/functests/14_arm/hugepages.go; do
  echo "===== $f (head 120) ====="
  nl -ba "$f" | sed -n '1,140p'
done

echo "===== workerRTNodes usage in 14_arm (match lines) ====="
rg -n 'workerRTNodes\[0\]' test/e2e/performanceprofile/functests/14_arm -n

echo "===== loops over workerRTNodes in 14_arm ====="
rg -n 'for .*range.*workerRTNodes|range .*workerRTNodes' test/e2e/performanceprofile/functests/14_arm -n

echo "===== tests with assertion messages patterns in 14_arm ====="
rg -n 'ToNot\(HaveOccurred\(\).*,"|To\(HaveOccurred\(\).*,"|ToNot\(ContainSubstring\([^)]*\).*,"|ContainSubstring\([^)]*\).*,"' test/e2e/performanceprofile/functests/14_arm -n || true

Repository: openshift/cluster-node-tuning-operator

Length of output: 221

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show relevant sections for ARM fun tests that use workerRTNodes[0]
for f in \
  test/e2e/performanceprofile/functests/14_arm/iommu.go \
  test/e2e/performanceprofile/functests/14_arm/kernelpagesize.go \
  test/e2e/performanceprofile/functests/14_arm/hugepages.go; do
  echo "===== $f (show first 160 lines) ====="
  cat -n "$f" | sed -n '1,160p'
done

echo "===== workerRTNodes usage in 14_arm (match lines) ====="
rg -n 'workerRTNodes\[0\]' test/e2e/performanceprofile/functests/14_arm -n

echo "===== loops over workerRTNodes in 14_arm ====="
rg -n 'for .*range.*workerRTNodes|range .*workerRTNodes' test/e2e/performanceprofile/functests/14_arm -n

echo "===== assertion messages patterns in 14_arm ====="
rg -n 'ToNot\(HaveOccurred\(\).*,"|To\(HaveOccurred\(\).*,"|ToNot\(ContainSubstring\([^)]*\).*,"|ContainSubstring\([^)]*\).*,"' test/e2e/performanceprofile/functests/14_arm -n || true

Repository: openshift/cluster-node-tuning-operator

Length of output: 18091

Improve failure diagnostics for iommu.passthrough kernel cmdline check

  • Add descriptive messages to the Expect(err)... and Expect(...).NotTo(ContainSubstring(...)) assertions (currently no messages).
  • The test only checks workerRTNodes[0], which matches the established pattern in the other ARM fun tests (kernelpagesize.go and hugepages.go also use workerRTNodes[0]), so this can stay—optionally add a short comment clarifying “first node is sufficient”.
diff 
 		cmdline, err := nodes.ExecCommand(ctx, &workerRTNodes[0], []string{"cat", "/proc/cmdline"})
-		Expect(err).ToNot(HaveOccurred())
-		Expect(string(cmdline)).NotTo(ContainSubstring("iommu.passthrough"))
+		Expect(err).ToNot(HaveOccurred(), "failed to read /proc/cmdline from ARM worker node")
+		Expect(string(cmdline)).NotTo(ContainSubstring("iommu.passthrough"), "iommu.passthrough should not be present in /proc/cmdline")
🤖 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 `@test/e2e/performanceprofile/functests/14_arm/iommu.go` around lines 44 - 48,
The test that reads /proc/cmdline via nodes.ExecCommand (in the It block
starting "should not add iommu.passthrough to the kernel cmdline") lacks failure
messages; update the two assertions to include descriptive messages: when
checking error (Expect(err).ToNot(HaveOccurred(), "...")) include context like
"failed to read /proc/cmdline from workerRTNodes[0]" and when asserting absence
(Expect(string(cmdline)).NotTo(ContainSubstring("iommu.passthrough"), "..."))
include the actual cmdline in the message (e.g., "unexpected kernel cmdline
contains iommu.passthrough: %s") so test failures show the value read;
optionally add a short comment above the ExecCommand call noting that checking
workerRTNodes[0] (the first ARM worker) is sufficient.

})