tuned:irqs: set default_smp_affinity using irqaffinity= - #1572
Conversation
WalkthroughThe change derives IRQ-affinity CPUs from isolated, reserved, and shared CPUs minus OVS-DPDK CPUs, passes them to tuned templates for conditional kernel arguments, and removes default IRQ affinity mask modification from the irqbalance cleanup script. ChangesIRQ affinity configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NewNodePerformance
participant TunedTemplate
participant Bootloader
NewNodePerformance->>TunedTemplate: Pass derived IRQ-affinity CPU set
TunedTemplate->>Bootloader: Render cmdline_irqaffinity kernel argument
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoSet default IRQ affinity via kernel irqaffinity= when ovsDpdk CPUs are configured
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
[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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go (1)
78-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the CPU-set contract.
Extend
pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.goto cover reserved/isolated/OVS-DPDK overlap, canonical output, and invalid empty results.🤖 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.go` around lines 78 - 86, Extend the tests for the tuned template CPU-set construction around the reserved/isolated merge and OVS-DPDK subtraction, covering overlapping inputs, canonical ordering of the resulting affinity string, and cases where subtraction produces an invalid empty set. Use the existing test helpers and assertions in tuned_test.go, anchoring coverage to the templateIrqAffinityCpus behavior and preserving the current error contract.
🤖 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
`@pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go`:
- Around line 78-86: Extend the tests for the tuned template CPU-set
construction around the reserved/isolated merge and OVS-DPDK subtraction,
covering overlapping inputs, canonical ordering of the resulting affinity
string, and cases where subtraction produces an invalid empty set. Use the
existing test helpers and assertions in tuned_test.go, anchoring coverage to the
templateIrqAffinityCpus behavior and preserving the current error contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5148f8fc-6698-4f5a-875b-80fa874cf6bc
📒 Files selected for processing (3)
assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.shassets/performanceprofile/tuned/openshift-node-performancepkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go
💤 Files with no reviewable changes (1)
- assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh
Code Review by Qodo
1.
|
When OVS-DPDK CPUs are configured, add irqaffinity= to the kernel command line to set default_smp_affinity at boot time instead of manipulating it post-boot via clear-irqbalance-banned-cpus.sh. - Compute irqaffinity as all CPUs (reserved + isolated + shared) minus OVS-DPDK CPUs, so shared CPUs are not accidentally excluded when MixedCpus is enabled. - Add cmdline_irqaffinity placeholder to the Intel x86 profile's ordering block to maintain stable kernel-arg ordering during upgrades. - Remove stale unit tests that verified the now-removed default_smp_affinity manipulation in clear-irqbalance-banned-cpus.sh. AIA Human-AI blend, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: Talor Itzhak <titzhak@redhat.com>
ef6d67e to
a810671
Compare
Call `make render-sync` to generate updated test data. Signed-off-by: Talor Itzhak <titzhak@redhat.com>
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/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml`:
- Line 38: Remove or explicitly gate the unconditional set -x in the
clear-irqbalance-banned-cpus.sh source asset, then regenerate the embedded
payloads in all five listed MachineConfig fixtures:
test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml:38-38,
test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml:38-38,
test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml:37-37,
test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml:37-37,
and
test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml:38-38,
ensuring none enables boot-time shell tracing by default.
🪄 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: 740993a2-405e-4348-b0f9-7ca44ecc5b74
📒 Files selected for processing (18)
test/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
| user: {} | ||
| - contents: | ||
| source: data:text/plain;charset=utf-8;base64,IyEvdXNyL2Jpbi9lbnYgYmFzaApzZXQgLWV1byBwaXBlZmFpbApzZXQgLXgKCiMgY29uc3QKU0VEPSIvdXNyL2Jpbi9zZWQiCiMgdHVuYWJsZSAtIG92ZXJyaWRhYmxlIGZvciB0ZXN0aW5nIHB1cnBvc2VzCklSUUJBTEFOQ0VfQ09ORj0iJHsxOi0vZXRjL3N5c2NvbmZpZy9pcnFiYWxhbmNlfSIKQ1JJT19PUklHX0JBTk5FRF9DUFVTPSIkezI6LS9ldGMvc3lzY29uZmlnL29yaWdfaXJxX2Jhbm5lZF9jcHVzfSIKTk9ORT0wCgojIEJBTk5FRF9DUFVTOiB0aGUgZmluYWwgaGV4IG1hc2sgd3JpdHRlbiB0byBJUlFCQUxBTkNFX0JBTk5FRF9DUFVTLgojICAgMS4gSWYgT1ZTX0RQREtfQ1BVUyBpcyBzZXQgKHZpYSBzeXN0ZW1kIEVudmlyb25tZW50KSwgYmFuIHRob3NlIENQVXMuCiMgICAyLiBPdGhlcndpc2UsIGRlZmF1bHQgdG8gMCAobm8gQ1BVcyBiYW5uZWQsIGFsbCBwYXJ0aWNpcGF0ZSBpbiBiYWxhbmNpbmcpLgppZiBbIC1uICIke09WU19EUERLX0NQVVM6LX0iIF07IHRoZW4KCUJBTk5FRF9DUFVTPSIke09WU19EUERLX0NQVVN9IgplbHNlCglCQU5ORURfQ1BVUz0iJHtOT05FfSIKZmkKClsgISAtZiAiJHtJUlFCQUxBTkNFX0NPTkZ9IiBdICYmIGV4aXQgMAoKJHtTRUR9IC1pICcvXlxzKklSUUJBTEFOQ0VfQkFOTkVEX0NQVVNcYi9kJyAiJHtJUlFCQUxBTkNFX0NPTkZ9IiB8fCBleGl0IDAKIyBDUFUgbnVtYmVycyB3aGljaCBoYXZlIHRoZWlyIGNvcnJlc3BvbmRpbmcgYml0cyBzZXQgdG8gb25lIGluIHRoaXMgbWFzawojIHdpbGwgbm90IGhhdmUgYW55IGlycSdzIGFzc2lnbmVkIHRvIHRoZW0gb24gcmViYWxhbmNlLgojIHNvIHplcm8gbWVhbnMgYWxsIGNwdXMgYXJlIHBhcnRpY2lwYXRpbmcgaW4gbG9hZCBiYWxhbmNpbmcuCmVjaG8gIklSUUJBTEFOQ0VfQkFOTkVEX0NQVVM9JHtCQU5ORURfQ1BVU30iID4+ICIke0lSUUJBTEFOQ0VfQ09ORn0iCgojIHdlIG5vdyBvd24gdGhpcyBjb25maWd1cmF0aW9uLiBCdXQgQ1JJLU8gaGFzIGNvZGUgdG8gcmVzdG9yZSB0aGUgY29uZmlndXJhdGlvbiwKIyBhbmQgdW50aWwgaXQgZ2FpbnMgdGhlIG9wdGlvbiB0byBkaXNhYmxlIHRoaXMgcmVzdG9yZSBmbG93LCB3ZSBuZWVkIHRvIG1ha2UKIyB0aGUgY29uZmlndXJhdGlvbiBjb25zaXN0ZW50IHN1Y2ggYXMgdGhlIENSSS1PIHJlc3RvcmUgd2lsbCBkbyBub3RoaW5nLgppZiBbIC1uICIke0NSSU9fT1JJR19CQU5ORURfQ1BVU30iIF0gJiYgWyAtZiAiJHtDUklPX09SSUdfQkFOTkVEX0NQVVN9IiBdOyB0aGVuCgllY2hvICIke0JBTk5FRF9DUFVTfSIgPiAiJHtDUklPX09SSUdfQkFOTkVEX0NQVVN9IgpmaQoKIyBDUkktTyByZWFkcyAvcHJvYy9pcnEvZGVmYXVsdF9zbXBfYWZmaW5pdHkgdG8gZGVyaXZlIHRoZSBJUlEgYmFubmVkIG1hc2sKIyB3aGVuIHBvZHMgd2l0aCBpcnEtbG9hZC1iYWxhbmNpbmcuY3Jpby5pbz1kaXNhYmxlIGFyZSBzY2hlZHVsZWQuCiMgSWYgd2UgZG9uJ3QgcmVtb3ZlIHRoZSBPVlMtRFBESyBDUFVzIGZyb20gZGVmYXVsdF9zbXBfYWZmaW5pdHkgaGVyZSwKIyBDUkktTyB3aWxsIG92ZXJ3cml0ZSBJUlFCQUxBTkNFX0JBTk5FRF9DUFVTIHdoaWxlIGlnbm9yaW5nIHRoZSBPVlMtRFBESyBDUFVzLgpTTVBfQUZGSU5JVFk9IiR7MzotL3Byb2MvaXJxL2RlZmF1bHRfc21wX2FmZmluaXR5fSIKaWYgWyAiJHtCQU5ORURfQ1BVU30iICE9ICIke05PTkV9IiBdICYmIFsgLWYgIiR7U01QX0FGRklOSVRZfSIgXTsgdGhlbgoJIyBkZWZhdWx0X3NtcF9hZmZpbml0eSBpcyBjb21tYS1zZXBhcmF0ZWQgMzItYml0IGhleCBncm91cHMgKGUuZy4gImZmLGZmZmZmZmZmLGZmZmZmZmZmIikKCUlGUz0nLCcgcmVhZCAtcmEgc21wIDwgIiR7U01QX0FGRklOSVRZfSIKCW49JHsjc21wW0BdfQoJIyBwYWQgQkFOTkVEX0NQVVMgd2l0aCBsZWFkaW5nIHplcm9zIHRvIG1hdGNoIHRoZSBzYW1lIG51bWJlciBvZiBoZXggY2hhcnMKCXBhZGRlZD0iJHtCQU5ORURfQ1BVU30iCgl3aGlsZSBbICR7I3BhZGRlZH0gLWx0ICQoKCBuICogOCApKSBdOyBkbwoJCXBhZGRlZD0iMCR7cGFkZGVkfSIKCWRvbmUKCSMgY2xlYXIgYmFubmVkIGJpdHMgZnJvbSBlYWNoIDMyLWJpdCBncm91cDogcmVzdWx0ID0gc21wICYgfmJhbm5lZAoJcmVzdWx0PSIiCglmb3IgKCggaT0wOyBpPG47IGkrKyApKTsgZG8KCQliYW49IiR7cGFkZGVkOiQoKCBpICogOCApKTo4fSIKCQl2YWw9JChwcmludGYgIiUwOHgiICQoKCAweCR7c21wWyRpXX0gJiB+MHgke2Jhbn0gKSkpCgkJcmVzdWx0Kz0iJHtyZXN1bHQ6Kyx9JHt2YWx9IgoJZG9uZQoJZWNobyAiU2V0dGluZyBkZWZhdWx0X3NtcF9hZmZpbml0eSB0byAke3Jlc3VsdH0gKHJlbW92aW5nIE9WUy1EUERLIENQVXMgJHtCQU5ORURfQ1BVU30gZnJvbSBkZWZhdWx0X3NtcF9hZmZpbml0eSBtYXNrKSIKCWVjaG8gIiR7cmVzdWx0fSIgPiAiJHtTTVBfQUZGSU5JVFl9IgpmaQo= | ||
| source: data:text/plain;charset=utf-8;base64,IyEvdXNyL2Jpbi9lbnYgYmFzaApzZXQgLWV1byBwaXBlZmFpbApzZXQgLXgKCiMgY29uc3QKU0VEPSIvdXNyL2Jpbi9zZWQiCiMgdHVuYWJsZSAtIG92ZXJyaWRhYmxlIGZvciB0ZXN0aW5nIHB1cnBvc2VzCklSUUJBTEFOQ0VfQ09ORj0iJHsxOi0vZXRjL3N5c2NvbmZpZy9pcnFiYWxhbmNlfSIKQ1JJT19PUklHX0JBTk5FRF9DUFVTPSIkezI6LS9ldGMvc3lzY29uZmlnL29yaWdfaXJxX2Jhbm5lZF9jcHVzfSIKTk9ORT0wCgojIEJBTk5FRF9DUFVTOiB0aGUgZmluYWwgaGV4IG1hc2sgd3JpdHRlbiB0byBJUlFCQUxBTkNFX0JBTk5FRF9DUFVTLgojICAgMS4gSWYgT1ZTX0RQREtfQ1BVUyBpcyBzZXQgKHZpYSBzeXN0ZW1kIEVudmlyb25tZW50KSwgYmFuIHRob3NlIENQVXMuCiMgICAyLiBPdGhlcndpc2UsIGRlZmF1bHQgdG8gMCAobm8gQ1BVcyBiYW5uZWQsIGFsbCBwYXJ0aWNpcGF0ZSBpbiBiYWxhbmNpbmcpLgppZiBbIC1uICIke09WU19EUERLX0NQVVM6LX0iIF07IHRoZW4KCUJBTk5FRF9DUFVTPSIke09WU19EUERLX0NQVVN9IgplbHNlCglCQU5ORURfQ1BVUz0iJHtOT05FfSIKZmkKClsgISAtZiAiJHtJUlFCQUxBTkNFX0NPTkZ9IiBdICYmIGV4aXQgMAoKJHtTRUR9IC1pICcvXlxzKklSUUJBTEFOQ0VfQkFOTkVEX0NQVVNcYi9kJyAiJHtJUlFCQUxBTkNFX0NPTkZ9IiB8fCBleGl0IDAKIyBDUFUgbnVtYmVycyB3aGljaCBoYXZlIHRoZWlyIGNvcnJlc3BvbmRpbmcgYml0cyBzZXQgdG8gb25lIGluIHRoaXMgbWFzawojIHdpbGwgbm90IGhhdmUgYW55IGlycSdzIGFzc2lnbmVkIHRvIHRoZW0gb24gcmViYWxhbmNlLgojIHNvIHplcm8gbWVhbnMgYWxsIGNwdXMgYXJlIHBhcnRpY2lwYXRpbmcgaW4gbG9hZCBiYWxhbmNpbmcuCmVjaG8gIklSUUJBTEFOQ0VfQkFOTkVEX0NQVVM9JHtCQU5ORURfQ1BVU30iID4+ICIke0lSUUJBTEFOQ0VfQ09ORn0iCgojIHdlIG5vdyBvd24gdGhpcyBjb25maWd1cmF0aW9uLiBCdXQgQ1JJLU8gaGFzIGNvZGUgdG8gcmVzdG9yZSB0aGUgY29uZmlndXJhdGlvbiwKIyBhbmQgdW50aWwgaXQgZ2FpbnMgdGhlIG9wdGlvbiB0byBkaXNhYmxlIHRoaXMgcmVzdG9yZSBmbG93LCB3ZSBuZWVkIHRvIG1ha2UKIyB0aGUgY29uZmlndXJhdGlvbiBjb25zaXN0ZW50IHN1Y2ggYXMgdGhlIENSSS1PIHJlc3RvcmUgd2lsbCBkbyBub3RoaW5nLgppZiBbIC1uICIke0NSSU9fT1JJR19CQU5ORURfQ1BVU30iIF0gJiYgWyAtZiAiJHtDUklPX09SSUdfQkFOTkVEX0NQVVN9IiBdOyB0aGVuCgllY2hvICIke0JBTk5FRF9DUFVTfSIgPiAiJHtDUklPX09SSUdfQkFOTkVEX0NQVVN9IgpmaQo= |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove set -x from the embedded early-boot script.
The same updated clear-irqbalance-banned-cpus.sh payload enables shell tracing in every affected MachineConfig, causing boot-time command and configuration values to be written to the system journal. Remove or explicitly gate tracing in the source asset, then regenerate all rendered fixtures.
test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml#L38-L38: regenerate the payload without unconditionalset -x.test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml#L38-L38: regenerate the payload without unconditionalset -x.test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml#L37-L37: regenerate the payload without unconditionalset -x.test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml#L37-L37: regenerate the payload without unconditionalset -x.test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml#L38-L38: regenerate the payload without unconditionalset -x.
📍 Affects 5 files
test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml#L38-L38(this comment)test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml#L38-L38test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml#L37-L37test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml#L37-L37test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml#L38-L38
🤖 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/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml`
at line 38, Remove or explicitly gate the unconditional set -x in the
clear-irqbalance-banned-cpus.sh source asset, then regenerate the embedded
payloads in all five listed MachineConfig fixtures:
test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml:38-38,
test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml:38-38,
test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml:37-37,
test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml:37-37,
and
test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml:38-38,
ensuring none enables boot-time shell tracing by default.
|
@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. |
When
ovsDpdkCPUs are configured, add irqaffinity= to the kernel command line to set default_smp_affinity at boot time instead of manipulating it post-boot via clear-irqbalance-banned-cpus.sh.AIA Human-AI blend, New content, Human-initiated, Reviewed, Claude Opus 4.6 v1.0
Summary by CodeRabbit