CNF-23471: Dedicate cpus for dpdk vswitch#1546
Open
Tal-or wants to merge 7 commits into
Open
Conversation
|
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:
WalkthroughThis PR adds OVS-DPDK CPU-set handling across the PerformanceProfile API, validation, controller flow, machine config rendering, runtime scripts, and e2e coverage. It also updates docs, generated fixtures, and test helpers to match the new field and prerequisite checks. ChangesOVS-DPDK CPU flow
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Tal-or The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Tal-or
changed the title
Contributor
|
@Tal-or: This pull request references CNF-23471 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go (1)
565-595:⚠️ Potential issue | 🟡 MinorHandle
CPUListToHexMaskconversion errors ingetIRQBalanceBannedCPUsOptionsinstead of logging and continuing.While
DedicatedCPUsstrings are validated by other components (kubeletconfig and tuned) during profile creation,getIRQBalanceBannedCPUsOptions(line 587) logs the conversion error but returns a machineconfig unit without theDEDICATED_CPUSenvironment variable set. This violates fail-fast principles—if the hex mask conversion fails, the machine config generation should also fail rather than silently proceeding with incomplete configuration. Although downstream validation in kubeletconfig.New() and tuned.NewNodePerformance() will prevent the profile from being applied if the string is invalid, machineconfig.New() should propagate the error instead of logging and returning a partial result.🤖 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 `@pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go` around lines 565 - 595, The getIRQBalanceBannedCPUsOptions function currently logs the CPUListToHexMask conversion error and returns incomplete options instead of failing fast. Modify the function signature to return both a slice of unit options and an error. When the CPUListToHexMask conversion fails, propagate the error by returning it instead of logging and continuing. Then update all callers of getIRQBalanceBannedCPUsOptions to handle the returned error and propagate it up to the machineconfig.New() function so that the entire machine config generation fails on conversion errors rather than creating a partial configuration.
🧹 Nitpick comments (4)
manifests/20-performance-profile.crd.yaml (1)
512-521: ⚡ Quick winConsider documenting prerequisites in the CRD description.
The CRD description provides detailed implementation effects but doesn't mention the prerequisites documented in the Go API types. The Go struct (performanceprofile_types.go:141-147) states "WorkloadPartitioning or --strict-cpu-reservation kubelet CPUManager policy option are a prerequisite for this feature." Users relying on CRD documentation (e.g., via
kubectl explain) won't see this requirement.📝 Suggested improvement
dedicated: description: |- Dedicated defines a set of CPUs fully isolated from the operating system and Kubernetes scheduling, intended for exclusive use by user-space processes (for example, infrastructure networking workloads such as DPDK-based vSwitch or vRouter). These CPUs receive full kernel-level isolation (isolcpus=domain,managed_irq, nohz_full, rcu_nocbs), are excluded from Kubelet scheduling (all QoS classes), banned from irqbalance, and excluded from systemd CPU affinity. + + Prerequisites: Either Workload Partitioning (CPUPartitioningAllNodes) + or the strict-cpu-reservation Kubelet CPUManager policy option must be + enabled. Without these prerequisites, Burstable and BestEffort QoS pods + may still be scheduled on dedicated CPUs. type: string🤖 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 `@manifests/20-performance-profile.crd.yaml` around lines 512 - 521, The "dedicated" CPU configuration description in the CRD is missing important prerequisite information that is documented in the Go API types (performanceprofile_types.go). Add a statement to the description field documenting that WorkloadPartitioning or --strict-cpu-reservation kubelet CPUManager policy option is a prerequisite for using this feature. This ensures users viewing the CRD documentation via kubectl explain or similar tools will be aware of the necessary prerequisites before attempting to use this feature.pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go (1)
732-751: ⚡ Quick winStrengthen dedicated CPU assertions to avoid false positives.
The new checks are permissive/tautological:
ContainElementswon’t catch extra CPUs, and assertingcmdline_cpu_partequals a constant doesn’t validate dedicated propagation. Prefer exact cpuset equality (or explicit negative checks) forisolated_cores, and assert behavior derived from that value.Suggested assertion tightening
- Expect(set.List()).To(ContainElements(4, 5, 10, 11)) + expected, err := cpuset.Parse("4-5,10-11") + Expect(err).ToNot(HaveOccurred()) + Expect(set.Equals(expected)).To(BeTrue())🤖 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 `@pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go` around lines 732 - 751, The test assertions for dedicated CPU handling are too permissive and could miss failures. In the first test ("should include dedicated CPUs in isolated_cores"), replace the ContainElements assertion on set.List() with an exact equality check to ensure isolated_cores contains precisely the expected CPUs and no extras. In the second test ("should include dedicated CPUs in nohz_full and rcu_nocbs via isolated_cores"), instead of just verifying that cmdline_cpu_part matches a constant string, directly assert that the nohz_full and rcu_nocbs kernel parameters in the bootloader section actually include the dedicated CPUs (10-11), confirming the values are properly derived from isolated_cores rather than just checking a static string.test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go (2)
148-174: ⚡ Quick winKernel cmdline CPU regex patterns may produce false positives.
The regex patterns
isolcpus=\S*%d,nohz_full=\S*%d, andrcu_nocbs=\S*%duse\S*(any non-whitespace) before the CPU number. This can match incorrectly when checking for a CPU like "2" in ranges such as "1-20" or multi-digit CPUs like "12".Consider using more precise patterns that account for CPU list/range syntax (e.g., comma-separated, hyphen-separated), or adding word boundaries where appropriate.
♻️ More precise regex example
For CPU lists that use commas and hyphens:
- Expect(cmdlineStr).To( - MatchRegexp(`isolcpus=\S*%d`, cpu), - fmt.Sprintf("isolcpus should include CPU %d (isolated+dedicated)", cpu)) + // Match CPU as standalone number, start/end of range, or in comma-separated list + cpuPattern := fmt.Sprintf(`isolcpus=[^ ]*(\b%d\b|%d-|\-%d)`, cpu, cpu, cpu) + Expect(cmdlineStr).To( + MatchRegexp(cpuPattern), + fmt.Sprintf("isolcpus should include CPU %d (isolated+dedicated)", cpu))🤖 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/2_performance_update/dedicatedcpus.go` around lines 148 - 174, The regex patterns used in the MatchRegexp calls for isolcpus, nohz_full, and rcu_nocbs are too loose with the `\S*%d` format, which can match false positives (e.g., matching "2" within "1-20" or "12"). Replace these patterns with more precise regex that accounts for CPU list syntax using comma and hyphen delimiters, such as patterns that match CPU numbers preceded by commas, hyphens, or the start of the parameter value, and followed by commas, hyphens, or end of parameter. Apply this fix to all three kernel parameter regex patterns in the MatchRegexp calls for isolcpus, nohz_full, and rcu_nocbs within the dedicated CPUs verification loops.
278-282: ⚡ Quick winAdd assertion for IRQBALANCE_BANNED_CPUS while GU pod is running.
The test logs
IRQBALANCE_BANNED_CPUSwhile the GU pod is running (line 278-282) but does not assert that it still contains the dedicated CPU hex mask. Currently, only the post-deletion check (lines 290-299) verifies the mask is preserved. Adding an assertion here would strengthen the test by confirming the mask remains stable throughout the pod lifecycle, not just after deletion.📝 Proposed assertion
By("Verifying IRQBALANCE_BANNED_CPUS still includes dedicated CPUs with pod running") irqConf, err = nodes.ExecCommand(ctx, node, []string{"cat", "/rootfs/etc/sysconfig/irqbalance"}) Expect(err).ToNot(HaveOccurred()) irqConfWithPod := testutils.ToString(irqConf) testlog.Infof("IRQBALANCE_BANNED_CPUS with pod: %s", irqConfWithPod) + Expect(irqConfWithPod).To(ContainSubstring(fmt.Sprintf("IRQBALANCE_BANNED_CPUS=%s", dedicatedHexMask)), + "IRQBALANCE_BANNED_CPUS should still contain dedicated CPU hex mask while pod is running")🤖 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/2_performance_update/dedicatedcpus.go` around lines 278 - 282, After logging the irqConfWithPod value on line 282, add an assertion to verify that IRQBALANCE_BANNED_CPUS still contains the dedicated CPU hex mask while the GU pod is running. This assertion should validate the same mask that is checked in the post-deletion verification (lines 290-299), ensuring the dedicated CPUs are properly reserved throughout the pod lifecycle, not just after the pod is removed. Reference the existing assertion logic from the later check to maintain consistency in how the mask validation is performed.
🤖 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 `@assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh`:
- Around line 41-59: The script pads `BANNED_CPUS` with leading zeros to match
the expected width but does not handle cases where `BANNED_CPUS` is already
wider than `n * 8` characters, causing substring extraction to use incorrect
positions. After calculating `n` from the `smp` array, add validation to check
if the length of `BANNED_CPUS` exceeds `n * 8` characters. If it does, either
truncate `BANNED_CPUS` to the rightmost `n * 8` characters to preserve the
significant bits (or exit with an error message indicating the banned CPU mask
is incompatible with the system topology). This ensures the substring extraction
in the loop at line 53 (the `ban=` assignment) correctly extracts the
appropriate 8-character chunks for each 32-bit group.
In `@assets/performanceprofile/scripts/dedicated-cpus-configure.sh`:
- Around line 9-26: The script assigns the DEDICATED_CPUS variable from the
template but does not validate that it contains a non-empty value before writing
it to multiple cgroup files (cpuset.cpus.exclusive, cpuset.cpus, and
cpuset.cpus.partition). Add a validation check immediately after the
DEDICATED_CPUS assignment to verify the variable is not empty, and exit with a
descriptive error message if it is empty, similar to the existing check for the
CGROUP_PATH directory. This will prevent invalid cgroup states from being
created due to empty template values.
In
`@pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.go`:
- Around line 247-261: The HasStrictCPUReservation function silently ignores
errors when json.Unmarshal fails, which prevents operators from debugging
malformed KubeletConfig payloads. Modify the error handling in the
HasStrictCPUReservation function where json.Unmarshal is called to log the error
before returning false. You will need to add a logger parameter to the function
or access a logger instance available in the package, then log the actual error
returned from the json.Unmarshal call to provide visibility into why the
function returns false.
---
Outside diff comments:
In
`@pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go`:
- Around line 565-595: The getIRQBalanceBannedCPUsOptions function currently
logs the CPUListToHexMask conversion error and returns incomplete options
instead of failing fast. Modify the function signature to return both a slice of
unit options and an error. When the CPUListToHexMask conversion fails, propagate
the error by returning it instead of logging and continuing. Then update all
callers of getIRQBalanceBannedCPUsOptions to handle the returned error and
propagate it up to the machineconfig.New() function so that the entire machine
config generation fails on conversion errors rather than creating a partial
configuration.
---
Nitpick comments:
In `@manifests/20-performance-profile.crd.yaml`:
- Around line 512-521: The "dedicated" CPU configuration description in the CRD
is missing important prerequisite information that is documented in the Go API
types (performanceprofile_types.go). Add a statement to the description field
documenting that WorkloadPartitioning or --strict-cpu-reservation kubelet
CPUManager policy option is a prerequisite for using this feature. This ensures
users viewing the CRD documentation via kubectl explain or similar tools will be
aware of the necessary prerequisites before attempting to use this feature.
In
`@pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go`:
- Around line 732-751: The test assertions for dedicated CPU handling are too
permissive and could miss failures. In the first test ("should include dedicated
CPUs in isolated_cores"), replace the ContainElements assertion on set.List()
with an exact equality check to ensure isolated_cores contains precisely the
expected CPUs and no extras. In the second test ("should include dedicated CPUs
in nohz_full and rcu_nocbs via isolated_cores"), instead of just verifying that
cmdline_cpu_part matches a constant string, directly assert that the nohz_full
and rcu_nocbs kernel parameters in the bootloader section actually include the
dedicated CPUs (10-11), confirming the values are properly derived from
isolated_cores rather than just checking a static string.
In `@test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go`:
- Around line 148-174: The regex patterns used in the MatchRegexp calls for
isolcpus, nohz_full, and rcu_nocbs are too loose with the `\S*%d` format, which
can match false positives (e.g., matching "2" within "1-20" or "12"). Replace
these patterns with more precise regex that accounts for CPU list syntax using
comma and hyphen delimiters, such as patterns that match CPU numbers preceded by
commas, hyphens, or the start of the parameter value, and followed by commas,
hyphens, or end of parameter. Apply this fix to all three kernel parameter regex
patterns in the MatchRegexp calls for isolcpus, nohz_full, and rcu_nocbs within
the dedicated CPUs verification loops.
- Around line 278-282: After logging the irqConfWithPod value on line 282, add
an assertion to verify that IRQBALANCE_BANNED_CPUS still contains the dedicated
CPU hex mask while the GU pod is running. This assertion should validate the
same mask that is checked in the post-deletion verification (lines 290-299),
ensuring the dedicated CPUs are properly reserved throughout the pod lifecycle,
not just after the pod is removed. Reference the existing assertion logic from
the later check to maintain consistency in how the mask validation is performed.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5b28947-9021-4a26-8c35-72850a71ac2e
⛔ Files ignored due to path filters (1)
pkg/apis/performanceprofile/v2/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (27)
assets/performanceprofile/configs/dedicatedcpus.sliceassets/performanceprofile/scripts/clear-irqbalance-banned-cpus.shassets/performanceprofile/scripts/dedicated-cpus-configure.shmanifests/20-performance-profile.crd.yamlpkg/apis/performanceprofile/v2/performanceprofile_types.gopkg/apis/performanceprofile/v2/performanceprofile_validation.gopkg/apis/performanceprofile/v2/performanceprofile_validation_test.gopkg/performanceprofile/controller/performanceprofile/components/components.gopkg/performanceprofile/controller/performanceprofile/components/handler/handler.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/profile/profile.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gopkg/performanceprofile/controller/performanceprofile/components/utils.gopkg/performanceprofile/controller/performanceprofile/components/utils_test.gopkg/performanceprofile/controller/performanceprofile/hypershift/components/handler.gopkg/performanceprofile/controller/performanceprofile/status/errors.gopkg/performanceprofile/controller/performanceprofile/status/status.gopkg/performanceprofile/controller/performanceprofile_controller.gopkg/performanceprofile/controller/performanceprofile_controller_test.gotest/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.gotest/e2e/performanceprofile/functests/3_performance_status/status.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/utils/label/label.go
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
06b8810 to
da5383b
Compare
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go`:
- Around line 123-128: The ResourceVersion comparison logic is broken because
initialProfile.ResourceVersion is assigned to equal
currentProfile.ResourceVersion on line 123, making the condition on line 126
that checks if initialProfile.ResourceVersion != currentProfile.ResourceVersion
always false. To fix this, capture the ResourceVersion value from currentProfile
into a separate variable before calling UpdateWithRetry on initialProfile, then
compare the ResourceVersion of initialProfile after the update against that
captured value to determine whether to call WaitForTuningUpdating and
WaitForTuningUpdated.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a529944-7ec1-4ca7-94e0-208d31d38f09
📒 Files selected for processing (31)
assets/performanceprofile/configs/dedicatedcpus.sliceassets/performanceprofile/scripts/clear-irqbalance-banned-cpus.shassets/performanceprofile/scripts/dedicated-cpus-configure.shpkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gotest/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.gotest/e2e/performanceprofile/functests/3_performance_status/status.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/utils/cluster/cluster.gotest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_tuned.yaml
✅ Files skipped from review due to trivial changes (8)
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/manual_tuned.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go
- assets/performanceprofile/scripts/dedicated-cpus-configure.sh
- pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go
- assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh
- pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go
- assets/performanceprofile/configs/dedicatedcpus.slice
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go
- test/e2e/performanceprofile/functests/3_performance_status/status.go
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.go
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.go
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
da5383b to
d99723e
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
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 `@test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go`:
- Around line 277-282: The test code logs the irqbalance configuration content
in `irqConfWithPod` but never asserts that it actually contains the dedicated
CPU mask as indicated by the "Verifying IRQBALANCE_BANNED_CPUS still includes
dedicated CPUs with pod running" message. After logging the content, add an
assertion that verifies `irqConfWithPod` contains the expected dedicated CPU
mask value to catch regressions where the configuration changes while the pod is
running.
In `@test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go`:
- Around line 755-808: The test modifies the cluster profile (setting
DisableOvsDynamicPinning to true) and creates a GU pod, but the cleanup logic
that reverts the profile and deletes the pod only runs if all preceding
assertions pass. If any assertion fails, the cluster state remains mutated and
can affect subsequent tests. Use Ginkgo's DeferCleanup hook at the beginning of
the test (after saving the original spec) to guarantee that both the GU pod
deletion and profile reversion happen regardless of assertion failures. Wrap the
pod deletion logic and the profile revert logic (which currently happens at the
end with savedSpec restoration) inside a DeferCleanup callback that will execute
even if tests fail earlier.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b187dc62-06d1-4163-9577-b3001646f97e
⛔ Files ignored due to path filters (1)
pkg/apis/performanceprofile/v2/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (37)
assets/performanceprofile/configs/dedicatedcpus.sliceassets/performanceprofile/scripts/clear-irqbalance-banned-cpus.shassets/performanceprofile/scripts/dedicated-cpus-configure.shmanifests/20-performance-profile.crd.yamlpkg/apis/performanceprofile/v2/performanceprofile_types.gopkg/apis/performanceprofile/v2/performanceprofile_validation.gopkg/apis/performanceprofile/v2/performanceprofile_validation_test.gopkg/performanceprofile/controller/performanceprofile/components/components.gopkg/performanceprofile/controller/performanceprofile/components/handler/handler.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/profile/profile.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gopkg/performanceprofile/controller/performanceprofile/components/utils.gopkg/performanceprofile/controller/performanceprofile/components/utils_test.gopkg/performanceprofile/controller/performanceprofile/hypershift/components/handler.gopkg/performanceprofile/controller/performanceprofile/status/errors.gopkg/performanceprofile/controller/performanceprofile/status/status.gopkg/performanceprofile/controller/performanceprofile_controller.gopkg/performanceprofile/controller/performanceprofile_controller_test.gotest/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.gotest/e2e/performanceprofile/functests/3_performance_status/status.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/utils/cluster/cluster.gotest/e2e/performanceprofile/functests/utils/label/label.gotest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
✅ Files skipped from review due to trivial changes (3)
- pkg/performanceprofile/controller/performanceprofile/status/status.go
- assets/performanceprofile/configs/dedicatedcpus.slice
- test/e2e/performanceprofile/functests/utils/label/label.go
🚧 Files skipped from review as they are similar to previous changes (29)
- pkg/performanceprofile/controller/performanceprofile/status/errors.go
- test/e2e/performanceprofile/functests/utils/cluster/cluster.go
- pkg/performanceprofile/controller/performanceprofile/components/components.go
- pkg/apis/performanceprofile/v2/performanceprofile_validation.go
- assets/performanceprofile/scripts/dedicated-cpus-configure.sh
- pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go
- pkg/performanceprofile/controller/performanceprofile/components/utils_test.go
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/profile/profile.go
- pkg/performanceprofile/controller/performanceprofile_controller.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml
- test/e2e/performanceprofile/functests/3_performance_status/status.go
- pkg/performanceprofile/controller/performanceprofile/components/utils.go
- pkg/apis/performanceprofile/v2/performanceprofile_types.go
- manifests/20-performance-profile.crd.yaml
- pkg/performanceprofile/controller/performanceprofile/hypershift/components/handler.go
- pkg/apis/performanceprofile/v2/performanceprofile_validation_test.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.go
- pkg/performanceprofile/controller/performanceprofile_controller_test.go
- assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh
- test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go
Comment on lines
+755
to
+808
| By("Setting disableOvsDynamicPinning=true on the profile") | ||
| currentProfile, err := profiles.GetByNodeLabels(testutils.NodeSelectorLabels) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| savedSpec := *currentProfile.Spec.DeepCopy() | ||
|
|
||
| if currentProfile.Spec.Net == nil { | ||
| currentProfile.Spec.Net = &performancev2.Net{} | ||
| } | ||
| currentProfile.Spec.Net.DisableOvsDynamicPinning = ptr.To(true) | ||
| profiles.UpdateWithRetry(currentProfile) | ||
|
|
||
| profilesupdate.WaitForTuningUpdating(ctx, currentProfile) | ||
| profilesupdate.WaitForTuningUpdated(ctx, currentProfile) | ||
|
|
||
| By("Verifying the activation file is absent after disabling") | ||
| _, err = nodes.ExecCommand(ctx, workerRTNode, cmd) | ||
| Expect(err).To(HaveOccurred(), | ||
| "activation file should not exist when disableOvsDynamicPinning=true") | ||
|
|
||
| By("Verifying OVS processes retain their systemd service affinity") | ||
| ovsAffinitiesAfter := getOvsAffinities(ctx, ovsSystemdServices, workerRTNode) | ||
| for pid, mask := range ovsAffinitiesAfter { | ||
| testlog.Infof("OVS pid %s affinity with dynamic pinning disabled: %s", pid, mask) | ||
| } | ||
|
|
||
| By("Creating a GU pod and verifying OVS affinity does not change") | ||
| guPod := createGuPod(ctx, workerRTNode) | ||
| ovsAffinitiesWithPod := getOvsAffinities(ctx, ovsSystemdServices, workerRTNode) | ||
| for pid, mask := range ovsAffinitiesWithPod { | ||
| testlog.Infof("OVS pid %s affinity with GU pod (dynamic pinning disabled): %s", pid, mask) | ||
| if afterMask, ok := ovsAffinitiesAfter[pid]; ok { | ||
| Expect(mask.Equals(afterMask)).To(BeTrue(), | ||
| fmt.Sprintf("OVS pid %s affinity should not change with dynamic pinning disabled (before: %s, after: %s)", | ||
| pid, afterMask, mask)) | ||
| } | ||
| } | ||
|
|
||
| By("Deleting the GU pod") | ||
| Expect(pods.DeleteAndSync(ctx, testclient.DataPlaneClient, guPod)).To(Succeed()) | ||
|
|
||
| By("Reverting the profile to re-enable OVS dynamic pinning") | ||
| currentProfile, err = profiles.GetByNodeLabels(testutils.NodeSelectorLabels) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| currentProfile.Spec = savedSpec | ||
| profiles.UpdateWithRetry(currentProfile) | ||
|
|
||
| profilesupdate.WaitForTuningUpdating(ctx, currentProfile) | ||
| profilesupdate.WaitForTuningUpdated(ctx, currentProfile) | ||
|
|
||
| By("Verifying the activation file is back after re-enabling") | ||
| _, err = nodes.ExecCommand(ctx, workerRTNode, cmd) | ||
| Expect(err).ToNot(HaveOccurred(), | ||
| "activation file should exist after re-enabling OVS dynamic pinning") | ||
| }) |
There was a problem hiding this comment.
Guarantee rollback/cleanup even when assertions fail.
This spec updates the cluster profile and creates a GU pod, but cleanup/revert runs only on the happy path. Any earlier failure can leave mutated cluster state behind and cascade into later tests.
Suggested hardening (use Ginkgo cleanup hooks)
- savedSpec := *currentProfile.Spec.DeepCopy()
+ savedSpec := *currentProfile.Spec.DeepCopy()
+ var guPod *corev1.Pod
+ DeferCleanup(func() {
+ if guPod != nil {
+ _ = pods.DeleteAndSync(ctx, testclient.DataPlaneClient, guPod)
+ }
+ p, err := profiles.GetByNodeLabels(testutils.NodeSelectorLabels)
+ if err == nil {
+ p.Spec = savedSpec
+ profiles.UpdateWithRetry(p)
+ profilesupdate.WaitForTuningUpdating(ctx, p)
+ profilesupdate.WaitForTuningUpdated(ctx, p)
+ }
+ })
...
- guPod := createGuPod(ctx, workerRTNode)
+ guPod = createGuPod(ctx, workerRTNode)
...
- By("Reverting the profile to re-enable OVS dynamic pinning")
- currentProfile, err = profiles.GetByNodeLabels(testutils.NodeSelectorLabels)
- ...As per coding guidelines, "Review Ginkgo test code for proper setup and cleanup: use BeforeEach/AfterEach for setup and cleanup. Flag tests that create cluster-scoped resources without cleanup".
🤖 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/7_performance_kubelet_node/cgroups.go`
around lines 755 - 808, The test modifies the cluster profile (setting
DisableOvsDynamicPinning to true) and creates a GU pod, but the cleanup logic
that reverts the profile and deletes the pod only runs if all preceding
assertions pass. If any assertion fails, the cluster state remains mutated and
can affect subsequent tests. Use Ginkgo's DeferCleanup hook at the beginning of
the test (after saving the original spec) to guarantee that both the GU pod
deletion and profile reversion happen regardless of assertion failures. Wrap the
pod deletion logic and the profile revert logic (which currently happens at the
end with savedSpec restoration) inside a DeferCleanup callback that will execute
even if tests fail earlier.
Source: Coding guidelines
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
d99723e to
daee559
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go`:
- Around line 192-197: Add assertions to validate that the default_smp_affinity
value excludes dedicated CPUs, rather than only logging the value. In the
dedicatedcpus.go file at lines 192-197, after logging smpAffinityStr, add an
Expect assertion to verify that the smpAffinityStr contains the correct affinity
mask that excludes dedicated CPUs. Apply the same assertion pattern at the
sibling locations in the same file at lines 239-244 and 298-303, ensuring each
verification step includes both the informational log and a concrete assertion
that validates the expected behavior instead of leaving the check untested.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3bc8e3b8-924f-44f4-a51e-3af12547d98d
📒 Files selected for processing (22)
assets/performanceprofile/configs/dedicatedcpus.sliceassets/performanceprofile/scripts/clear-irqbalance-banned-cpus.shassets/performanceprofile/scripts/dedicated-cpus-configure.shpkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gotest/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.gotest/e2e/performanceprofile/functests/3_performance_status/status.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/utils/cluster/cluster.gotest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- assets/performanceprofile/configs/dedicatedcpus.slice
- assets/performanceprofile/scripts/dedicated-cpus-configure.sh
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yaml
- test/e2e/performanceprofile/functests/utils/cluster/cluster.go
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go
- pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go
- test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
- test/e2e/performanceprofile/functests/3_performance_status/status.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml
- assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yaml
- test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.go
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
daee559 to
787f61e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go (1)
92-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMerge
strict-cpu-reservationinstead of replacing the whole experimental annotation.Line 97 overwrites any existing
kubeletconfig.experimentalpayload, so this e2e update can temporarily drop unrelated kubelet options from the target profile. Preserve existing JSON and only setcpuManagerPolicyOptions["strict-cpu-reservation"].Suggested approach
import ( "context" + "encoding/json" "fmt" "strings" "time" @@ if !isWPEnabled { testlog.Infof("Workload partitioning not enabled, adding strict-cpu-reservation via experimental annotation") if currentProfile.Annotations == nil { currentProfile.Annotations = make(map[string]string) } - currentProfile.Annotations["kubeletconfig.experimental"] = `{"cpuManagerPolicyOptions": {"strict-cpu-reservation": "true"}}` + experimental := map[string]interface{}{} + if raw := currentProfile.Annotations["kubeletconfig.experimental"]; raw != "" { + Expect(json.Unmarshal([]byte(raw), &experimental)).To(Succeed()) + } + policyOptions, ok := experimental["cpuManagerPolicyOptions"].(map[string]interface{}) + if !ok { + policyOptions = map[string]interface{}{} + experimental["cpuManagerPolicyOptions"] = policyOptions + } + policyOptions["strict-cpu-reservation"] = "true" + raw, err := json.Marshal(experimental) + Expect(err).ToNot(HaveOccurred()) + currentProfile.Annotations["kubeletconfig.experimental"] = string(raw) }🤖 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/2_performance_update/dedicatedcpus.go` around lines 92 - 98, The annotation assignment at line 97 in the dedicatedcpus.go file unconditionally overwrites the entire `kubeletconfig.experimental` annotation value, losing any existing kubelet configuration options. Instead of directly assigning a new JSON string, check if the annotation already exists, parse the existing JSON payload, merge the `cpuManagerPolicyOptions.strict-cpu-reservation` setting into it while preserving other existing options, and only then set the merged result back to the annotation. This ensures unrelated kubelet options are not dropped during the e2e update.
🤖 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 `@test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go`:
- Around line 267-285: Register pod cleanup immediately after the testpod is
successfully created using DeferCleanup to ensure the pod is cleaned up even if
the test fails at any subsequent step. Add a DeferCleanup call right after the
successful testclient.DataPlaneClient.Create(ctx, testpod) call that registers
deletion of the testpod, while keeping the explicit Delete call at line 282 to
maintain the test's lifecycle assertion flow. This ensures that if
WaitForCondition, the QoS assertion, or any later check fails, the pod will
still be cleaned up and not left running when AfterAll reverts the profile.
---
Duplicate comments:
In `@test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.go`:
- Around line 92-98: The annotation assignment at line 97 in the
dedicatedcpus.go file unconditionally overwrites the entire
`kubeletconfig.experimental` annotation value, losing any existing kubelet
configuration options. Instead of directly assigning a new JSON string, check if
the annotation already exists, parse the existing JSON payload, merge the
`cpuManagerPolicyOptions.strict-cpu-reservation` setting into it while
preserving other existing options, and only then set the merged result back to
the annotation. This ensures unrelated kubelet options are not dropped during
the e2e update.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 915ee603-f527-41d5-bdaf-93cf27bf5671
📒 Files selected for processing (13)
test/e2e/performanceprofile/functests/2_performance_update/dedicatedcpus.gotest/e2e/performanceprofile/functests/3_performance_status/status.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/utils/cluster/cluster.gotest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yaml
- test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go
- test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml
- test/e2e/performanceprofile/functests/3_performance_status/status.go
- test/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
4 times, most recently
from
b10178c to
c58e443
Compare
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
c58e443 to
f398b31
Compare
Contributor
Author
|
/retest |
@Tal-or Disabling OVS Pinning will have a negative impact if we enable workload partitioning. For our use-case we are trying to make ovn-kubernetes skip pmd threads instead: ovn-kubernetes/ovn-kubernetes#6500. Are there any additional reasons you want to expose this to the API? |
Contributor
Author
@amorenoz we're disabling the dynamic pinning so they won't interfere with the OVS DPDK. But setting |
Contributor
|
/cc @MarSik |
|
Hi @Tal-or, I am currently testing this PR with OVS-DPDK. |
jmencak
reviewed
Jul 3, 2026
Contributor
There was a problem hiding this comment.
I was looking at this from the end-user perspective. I wanted to test this, but I didn't get past issues starting kubelet when configuring:
kubeletConfig:
cpuManagerPolicy: static
cpuManagerPolicyOptions:
strict-cpu-reservation: "true"
I left some comments WRT validating user input. I also wonder if we're handling overlapping of Dedicated CPUs with other CPU types correctly.
| if dedicatedCPUs != "" { | ||
| dedicatedMask, err := components.CPUListToHexMask(dedicatedCPUs) | ||
| if err != nil { | ||
| klog.Errorf("failed to convert dedicated CPUs %q to hex mask: %v", dedicatedCPUs, err) |
Contributor
There was a problem hiding this comment.
Can this ever happen? If so (e.g. no webhook to check user-supplied values, or insufficient check for user error input), is the error in the logs sufficient to inform the user about the failure? I.e. the fact IRQ banning is not applied properly as user expected (DEDICATED_CPUS unset)? There will probably be no error status condition set in PerformanceProfile, right? Also, isn't the bad value (0) going to get propagated to default_smp_affinity?
Edit: empty dedicated CPUs will end up with Degraded PerformanceProfile, so I guess that's what we want. Alternatively, we may decide not to allow empty dedicated CPUs.
Contributor
Author
There was a problem hiding this comment.
it should be checked during the API validation. I'll revise this code, thanks.
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
2 times, most recently
from
85a0a8c to
388eb0f
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
2 times, most recently
from
85fe14d to
dd82c29
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai reedit release note summary for PR description |
|
✅ Action performedSummary regeneration triggered. |
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
2 times, most recently
from
3d17715 to
081b505
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
test/e2e/performanceprofile/functests/2_performance_update/ovsdpdk.go (1)
304-328: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGU pod is not cleaned up if an assertion fails before the explicit
Deleteat Line 339.If
WaitForCondition, the QoS assertion (Line 327), or the banned-CPU assertion (Lines 330-336) fails,testpodis left running with no cleanup path since deletion only happens explicitly later in the happy path.🔧 Suggested fix
err = testclient.DataPlaneClient.Create(ctx, testpod) Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + err := testclient.DataPlaneClient.Delete(cleanupCtx, testpod) + Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) + }) podKey := client.ObjectKeyFromObject(testpod)🤖 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/2_performance_update/ovsdpdk.go` around lines 304 - 328, The GU pod created in the OVS DPDK performance update test is only deleted in the happy path, so failures in WaitForCondition, the QOS assertion, or the banned-CPU check can leave it behind. Add a cleanup path immediately after creating the pod in the test flow (around the testpod setup and Create/WaitForCondition logic) so deletion always runs on failure, using the existing testpod variable and the test client cleanup pattern rather than relying on the later explicit Delete.
🧹 Nitpick comments (1)
pkg/performanceprofile/controller/performanceprofile/components/assets_scripts_test.go (1)
131-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a 3-group (>64 CPU) test case to fulfill the earlier test-coverage commitment.
All
DescribeTableentries use at most 2 hex groups (≤64 CPUs). jmencak specifically asked for coverage on systems with more than 64 CPUs to avoid silent bugs, and this was acknowledged ("Agreed, will do."). Consider adding an entry with 3 groups, e.g."ffffffff,ffffffff,ffffffff"with a banned mask spanning the third group, to validate the loop's generality beyond 2 groups.Suggested additional entry
Entry("three groups (>64 cpus), ban in highest group", "ffffffff,ffffffff,ffffffff", "0000000c0000000000000000", "fffffff3,ffffffff,ffffffff"),🤖 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 `@pkg/performanceprofile/controller/performanceprofile/components/assets_scripts_test.go` around lines 131 - 170, Add a new DescribeTable entry in assets_scripts_test.go for a 3-group OVS_DPDK_CPUS mask to cover systems with more than 64 CPUs, since all current cases in the ClearIRQBalanceBannedCPUs test only exercise up to two hex groups. Update the table around the existing ClearIRQBalanceBannedCPUs entries to include a case where the banned CPUs are in the highest group, and assert the resulting default_smp_affinity preserves the other two groups unchanged while clearing the targeted bits.
🤖 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 `@pkg/apis/performanceprofile/v2/performanceprofile_types.go`:
- Around line 145-150: Fix the grammar in the OvsDpdk field comment so the
generated CRD/docs are correct: in the comment on OvsDpdk, change the sentence
about “WorkloadPartitioning or --strict-cpu-reservation kubelet CPUManager
policy option” to use a singular verb (“is a prerequisite”). Keep the rest of
the descriptive text unchanged so the comment remains the source for oc explain
and documentation generation.
In
`@pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go`:
- Around line 608-623: The negative test in
New/clear-irqbalance-banned-cpus.service is checking the wrong string and can
pass even if the behavior is broken. Update the assertion in the
machineconfig_test.go case to verify the service contents do not contain
OVS_DPDK_CPUS, matching the positive expectation used in the nearby test for
OvsDpdkCPUs, and keep the check scoped to the
clear-irqbalance-banned-cpus.service unit content.
In
`@pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go`:
- Around line 556-577: getIRQBalanceBannedCPUsOptions is swallowing
CPUListToHexMask failures and silently omitting the OVS_DPDK_CPUS environment
option; change it to return an error instead of just logging and returning opts.
Update the caller that builds the MachineConfig to handle this error explicitly,
so the failure is surfaced and can mark the profile Degraded rather than
producing a partially rendered config. Ensure the error path stays tied to
getIRQBalanceBannedCPUsOptions and the MachineConfig generation flow that
consumes its result.
In
`@pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go`:
- Around line 744-764: The two tests in tuned_test.go only validate static or
unrelated values, so they do not prove OvsDpdk CPUs are handled correctly.
Update the test around getTunedStructuredData to assert the rendered bootloader
cmdline fields that expand isolated_cores and not_isolated_cores_expanded, and
verify the resulting nohz_full/rcu_nocbs/systemd.cpu_affinity values actually
include or exclude the OvsDpdk CPU set. Use the existing helpers and symbols
like getTunedStructuredData, bootLoaderSection, variables, and isolated_cores to
locate the right assertions.
In `@pkg/performanceprofile/controller/performanceprofile/status/errors.go`:
- Around line 16-22: The error text produced by NewOvsDpdkCPUsPrerequisiteError
in errors.go uses lowercase “ovs-dpdk cpus”, but the test in
performanceprofile_controller_test.go expects a capitalized substring, so the
case-sensitive match will fail. Update the test expectation to match the actual
message returned by NewOvsDpdkCPUsPrerequisiteError (or otherwise make both
strings identical), keeping the substring consistent with the error string used
by OvsDpdkCPUsPrerequisiteError.
In `@test/e2e/performanceprofile/functests/2_performance_update/ovsdpdk.go`:
- Around line 88-100: The BeforeAll setup in ovsdpdk.go adds
currentProfile.Annotations["kubeletconfig.experimental"] when workload
partitioning is disabled, but AfterAll only restores Spec and removes the
OvsDpdk disable annotation. Update the AfterAll cleanup to also delete the
kubeletconfig.experimental annotation (or restore annotations to their original
state) in the same currentProfile handling path so the suite leaves no lingering
cluster state.
---
Duplicate comments:
In `@test/e2e/performanceprofile/functests/2_performance_update/ovsdpdk.go`:
- Around line 304-328: The GU pod created in the OVS DPDK performance update
test is only deleted in the happy path, so failures in WaitForCondition, the QOS
assertion, or the banned-CPU check can leave it behind. Add a cleanup path
immediately after creating the pod in the test flow (around the testpod setup
and Create/WaitForCondition logic) so deletion always runs on failure, using the
existing testpod variable and the test client cleanup pattern rather than
relying on the later explicit Delete.
---
Nitpick comments:
In
`@pkg/performanceprofile/controller/performanceprofile/components/assets_scripts_test.go`:
- Around line 131-170: Add a new DescribeTable entry in assets_scripts_test.go
for a 3-group OVS_DPDK_CPUS mask to cover systems with more than 64 CPUs, since
all current cases in the ClearIRQBalanceBannedCPUs test only exercise up to two
hex groups. Update the table around the existing ClearIRQBalanceBannedCPUs
entries to include a case where the banned CPUs are in the highest group, and
assert the resulting default_smp_affinity preserves the other two groups
unchanged while clearing the targeted bits.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b91ca69d-721e-4b66-b14c-0b8cc502918c
⛔ Files ignored due to path filters (1)
pkg/apis/performanceprofile/v2/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (39)
assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.shassets/performanceprofile/scripts/ovs-dpdk-cpus-configure.shdocs/performanceprofile/performance_profile.mdmanifests/20-performance-profile.crd.yamlpkg/apis/performanceprofile/v2/performanceprofile_types.gopkg/apis/performanceprofile/v2/performanceprofile_validation.gopkg/apis/performanceprofile/v2/performanceprofile_validation_test.gopkg/performanceprofile/controller/performanceprofile/components/assets_scripts_test.gopkg/performanceprofile/controller/performanceprofile/components/components.gopkg/performanceprofile/controller/performanceprofile/components/handler/handler.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.gopkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gopkg/performanceprofile/controller/performanceprofile/components/profile/profile.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.gopkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gopkg/performanceprofile/controller/performanceprofile/components/utils.gopkg/performanceprofile/controller/performanceprofile/components/utils_test.gopkg/performanceprofile/controller/performanceprofile/hypershift/components/handler.gopkg/performanceprofile/controller/performanceprofile/status/errors.gopkg/performanceprofile/controller/performanceprofile/status/status.gopkg/performanceprofile/controller/performanceprofile_controller.gopkg/performanceprofile/controller/performanceprofile_controller_test.gotest/e2e/performanceprofile/functests/2_performance_update/ovsdpdk.gotest/e2e/performanceprofile/functests/3_performance_status/status.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/utils/cluster/cluster.gotest/e2e/performanceprofile/functests/utils/label/label.gotest/e2e/performanceprofile/functests/utils/pods/pods.gotest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
✅ Files skipped from review due to trivial changes (1)
- test/e2e/performanceprofile/functests/utils/label/label.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/performanceprofile/controller/performanceprofile/components/utils_test.go
- test/e2e/performanceprofile/functests/utils/cluster/cluster.go
- test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/kubeletconfig/kubeletconfig.go
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
081b505 to
c72f93c
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
@Tal-or: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
… API Add a new optional field spec.cpu.ovsDpdk (type CPUSet) for reserving CPUs for ovsDpdk PMD threads. Add annotation constant performance.openshift.io/disable-load-balancing-ovs-dpdk to control cgroup partition type (isolated vs member). Update CPU validation to account for ovsDpdk CPUs, NewCPULists to accept ovsDpdk parameter, and add OvsDpdk feature label for E2E tests. Update API docs via make generate-docs. AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
…pdk CPUs Add OvsDpdkCPUsPrerequisiteError and ConditionOvsDpdkCPUsPrerequisiteNotMet for validating that ovsDpdk CPUs require either workload partitioning or CPU partitioning mode enabled. Add MachineConfigOptions fields OvsDpdkCPUs and DisableLoadBalancingForOvsDpdk, with annotation-based toggle using strconv.ParseBool. AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
When spec.cpu.ovsDpdk is set, union those CPUs into the kubelet's reservedSystemCPUs so they are not available for pod scheduling. AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
When spec.cpu.ovsDpdk is set, union those CPUs into isolated_cores so they receive the same tuned parameters as isolated CPUs (isolcpus, nohz_full, rcu_nocbs) and are excluded from systemd.cpu_affinity. AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
When spec.cpu.ovsDpdk is configured: - Create nested ovsdpdk.slice inside ovs.slice/ovs-vswitchd.service/ with cpuset.cpus.partition type controlled by annotation (isolated when disable-load-balancing-ovs-dpdk is truthy, member default) - Expand ovs-vswitchd.service drop-in with Environment=OVS_DPDK_CPUS - Ban ovsDpdk CPUs from IRQ balancing and clear them from /proc/irq/default_smp_affinity - Always write OVS dynamic pinning trigger file - Add ovs-dpdk-cpus-configure oneshot service (After=ovs-vswitchd.service) Template-baked partition type ensures annotation changes trigger MCO rollout (handles isolated-to-member transition correctly). Add bash unit tests for the SMP affinity hex mask manipulation logic. AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
Add E2E tests for ovsDpdk CPU feature: - Update test verifying nested ovsdpdk.slice inside ovs.slice/ovs-vswitchd.service/ with cpuset.cpus.partition - Status test for OvsDpdkCPUsPrerequisiteNotMet condition - Parse kernel cmdline CPU lists as cpusets instead of regex - Pass explicit client to IsWorkloadPartitioningEnabled AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
AIA: Primarily AI, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
Tal-or
force-pushed
the
dedicate_cpus_for_dpdk_vswitch
branch
from
c72f93c to
d721d98
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
spec.cpu.ovsDpdkfield to the PerformanceProfile v2 API, enabling users to designate CPUs for exclusive use by OVS-DPDK.The OVS-DPDK CPUs receives full kernel-level scheduling isolation, are excluded from kubelet scheduling and IRQ balancing, and remain protected from interference across pod lifecycle events.
The controller validates that the cluster meets the prerequisites for the isolation and reports a Degraded status if they are not satisfied.
In addition
"performance.openshift.io/disable-load-balancing-ovs-dpdk"annotation was added to disable kernel load balancing on theovsDpdkCPUs. This PR makes it optional since an extra step is needed from the OVS process side to make this feature fully functional, so we don't want to decouple this extra isolation capability from the general set of parameters we configure for the ovs dpdk set.Includes unit and e2e test coverage.
This work inspired by: https://github.com/mcoquelin/cluster-node-tuning-operator/commits/release-4.20-ovs-dpdk/
Signed-off-by: Talor Itzhak titzhak@redhat.com
Summary by CodeRabbit
spec.cpu.ovsDpdksupport to reserve fully isolated CPUs for OVS-DPDK, including validation and reconciliation.default_smp_affinity, including consistent CRI-O restore behavior.