CNF-20633: Enable nohz_full and skew_tick by default#1564
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@MarSik: This pull request references CNF-20633 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 task 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThe tuned profiles separate ChangesKernel cmdline restructuring
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/RHsyseng/operator-utils@v1.4.13: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20191104093116-d3cd4ed1dbcf: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition@v0.35.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition/v2@v2.26.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/docker/go-units@v0.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-logr/stdr@v1.2.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0 ... [truncated 19340 characters] ... is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/legacy-cloud-providers: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarSik The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/performanceprofile/tuned/openshift-node-performance (1)
151-165: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUpdate the unit tests for the renamed cmdline fragments.
pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gostill expectscmdline_realtime_nohzfulland+skew_tick=1 rcutree.kthread_prio=11. Update it to assertcmdline_cpu_part_nohzfull,cmdline_cpu_tuning, and the reduced realtime value; otherwise the existing test fails.🤖 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 `@assets/performanceprofile/tuned/openshift-node-performance` around lines 151 - 165, Update the expectations in the tuned profile tests to use cmdline_cpu_part_nohzfull and cmdline_cpu_tuning instead of the old cmdline_realtime_nohzfull fragment, and expect the realtime value to contain only rcutree.kthread_prio=11 rather than the combined skew_tick string.
🤖 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.
Outside diff comments:
In `@assets/performanceprofile/tuned/openshift-node-performance`:
- Around line 151-165: Update the expectations in the tuned profile tests to use
cmdline_cpu_part_nohzfull and cmdline_cpu_tuning instead of the old
cmdline_realtime_nohzfull fragment, and expect the realtime value to contain
only rcutree.kthread_prio=11 rather than the combined skew_tick string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75aa5368-8021-4585-b71b-3375c4b622ba
📒 Files selected for processing (11)
assets/performanceprofile/tuned/openshift-node-performanceassets/performanceprofile/tuned/openshift-node-performance-intel-x86test/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_tuned.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_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_tuned.yaml
💤 Files with no reviewable changes (1)
- assets/performanceprofile/tuned/openshift-node-performance-intel-x86
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go (1)
189-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover realtime assertions for
nohz_full.Since
nohz_fullhas been moved to the base CPU configuration and is applied unconditionally regardless of theRealTimehint, these assertions are no longer accurate in the context of realtime toggling tests.
pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go#L189-L189: delete this line. Thecmdline_realtime_nohzfullkey is obsolete and no longer generated.pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go#L208-L208: delete this line. Checkingcmdline_cpu_part_nohzfullhere is conceptually misleading and redundant, as its presence is properly verified in the base worker tests (lines 125 and 161).♻️ Proposed fix
@@ -186,7 +186,6 @@ bootLoaderSection, err := tunedData.GetSection("bootloader") Expect(err).ToNot(HaveOccurred()) - Expect(bootLoaderSection.Key("cmdline_realtime_nohzfull").String()).ToNot(Equal(cmdlineCPUPartNohzfull)) Expect(bootLoaderSection.Key("cmdline_realtime_nosoftlookup").String()).ToNot(Equal(cmdlineRealtimeNosoftlookup)) Expect(bootLoaderSection.Key("cmdline_realtime_common").String()).ToNot(Equal(cmdlineRealtimeCommon)) }) @@ -205,7 +204,6 @@ Expect(sysctl.Key("kernel.sched_rt_runtime_us").String()).To(Equal("-1")) bootLoader, err := tunedData.GetSection("bootloader") Expect(err).ToNot(HaveOccurred()) - Expect(bootLoader.Key("cmdline_cpu_part_nohzfull").String()).To(Equal(cmdlineCPUPartNohzfull)) Expect(bootLoader.Key("cmdline_realtime_nosoftlookup").String()).To(Equal(cmdlineRealtimeNosoftlookup)) Expect(bootLoader.Key("cmdline_realtime_common").String()).To(Equal(cmdlineRealtimeCommon)) })🤖 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` at line 189, Remove the obsolete realtime nohz_full assertions from the tuned tests: delete the assertion at pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go lines 189-189 for cmdline_realtime_nohzfull and the assertion at lines 208-208 for cmdline_cpu_part_nohzfull; retain verification through the existing base worker tests.
🤖 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_test.go`:
- Line 189: Remove the obsolete realtime nohz_full assertions from the tuned
tests: delete the assertion at
pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.go
lines 189-189 for cmdline_realtime_nohzfull and the assertion at lines 208-208
for cmdline_cpu_part_nohzfull; retain verification through the existing base
worker tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b2c01976-3e2c-40c9-9dfa-247cd89db669
📒 Files selected for processing (12)
assets/performanceprofile/tuned/openshift-node-performanceassets/performanceprofile/tuned/openshift-node-performance-intel-x86pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned_test.gotest/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_tuned.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_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_tuned.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_tuned.yaml
💤 Files with no reviewable changes (1)
- assets/performanceprofile/tuned/openshift-node-performance-intel-x86
🚧 Files skipped from review as they are similar to previous changes (9)
- 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
- assets/performanceprofile/tuned/openshift-node-performance
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_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/arm/manual_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_tuned.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/manual_tuned.yaml
The nohz_full kernel arg makes sure no tick interrupts arrive to cpus with just a single schedulable process. That is something we really want. All the conditions from https://github.com/torvalds/linux/blob/5097cbc/Documentation/timers/no_hz.rst#omit-scheduling-clock-ticks-for-cpus-with-only-one-runnable-task are satisfied as well: - There are reserved cpus with tick enabled - The tuning sets rcu_nocbs The kernel command line parameter skew_tick helps to smooth jitter on moderate to large systems with latency-sensitive applications running and this matches exactly our use case. There is also a cleanup of the Intel overrides block that only made sure the kernel arguments stay in the same order. I renamed two tuned variables to the reboot protection is not going to matter and 5.0 major version is a good opportunity to get rid of this.
|
@MarSik: all tests passed! 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. |
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
LGTM overall
left a small comment about behavioral changes which I'm not sure it's intended.
| cmdline_realtime_nohzfull=+nohz_full=${isolated_cores} | ||
| cmdline_realtime_nosoftlookup=+nosoftlockup | ||
| cmdline_realtime_common=+skew_tick=1 rcutree.kthread_prio=11 | ||
| cmdline_realtime_common=rcutree.kthread_prio=11 |
There was a problem hiding this comment.
we are changing the behavior here from append to overwrite (missing the + sign) is it intentional?
| cmdlineCPUPartNohzfull = "+nohz_full=${isolated_cores}" | ||
| cmdlineRealtimeNosoftlookup = "+nosoftlockup" | ||
| cmdlineRealtimeCommon = "+skew_tick=1 rcutree.kthread_prio=11" | ||
| cmdlineRealtimeCommon = "rcutree.kthread_prio=11" |
There was a problem hiding this comment.
ditto, from append to overwrite
3 participants
The nohz_full kernel arg makes sure no tick interrupts arrive to cpus with just a single schedulable process.
That is something we really want.
All the conditions from https://github.com/torvalds/linux/blob/5097cbc/Documentation/timers/no_hz.rst#omit-scheduling-clock-ticks-for-cpus-with-only-one-runnable-task are satisfied as well:
The kernel command line parameter skew_tick helps to smooth jitter on moderate to large systems with latency-sensitive applications running and this matches exactly our use case.
There is also a cleanup of the Intel overrides block that only made sure the kernel arguments stay in the same order. I renamed two tuned variables to the reboot protection is not going to matter and 5.0 major version is a good opportunity to get rid of this.
Summary by CodeRabbit
nohz_fullpartitioning and tick-skew (+skew_tick=1) into dedicated CPU-related settings.cmdline_realtime_commononrcutree.kthread_prio=11.