OCPBUGS-83865: Revert stalld backend to sched_debug#1515
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@bartwensley: This pull request references Jira Issue OCPBUGS-83865, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
WalkthroughThe MachineConfig generator now injects two files into generated Ignition MachineConfigs: ChangesStalld sched_debug Backend Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: dgonyier. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bartwensley 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 |
|
/cc @MarSik |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Looks like I need to update some e2e tests... |
bartwensley
force-pushed
the
stalld-backend
branch
from
ad02c11 to
4f402e3
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
/cc @browsell |
| }) | ||
|
|
||
| Context("stalld backend configuration", func() { | ||
| It("should include stalld sysconfig and systemd dropin files", func() { |
There was a problem hiding this comment.
This is checking the files, but not whether systemd consumed them. You should probably add an e2e test too that makes sure systemd noticed the files. See https://github.com/openshift/cluster-node-tuning-operator/blob/main/test/e2e/performanceprofile/functests/utils/systemd/systemd.go#L17 for helper functions. And probably place the test somewhere here https://github.com/openshift/cluster-node-tuning-operator/blob/main/test/e2e/performanceprofile/functests/1_performance/performance.go
There was a problem hiding this comment.
I added a new e2e test as you requested.
bartwensley
force-pushed
the
stalld-backend
branch
from
4f402e3 to
c2837d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/performanceprofile/functests/1_performance/performance.go (1)
346-353: ⚡ Quick winAssert the drop-in path directly to avoid false positives.
This test currently proves
EnvironmentFilescontains/etc/sysconfig/stalld-backend, but that does not guaranteestalld-backend.confis the source. Please also assertDropInPathscontains/etc/systemd/system/stalld.service.d/stalld-backend.conf(or equivalent merged unit output) so the test matches its stated intent.🤖 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/1_performance/performance.go` around lines 346 - 353, The test "Should have the stalld service configured with the stalld-backend drop-in" only checks EnvironmentFiles and can yield false positives; update the test to also assert the systemd "DropInPaths" property contains the actual drop-in file path "/etc/systemd/system/stalld.service.d/stalld-backend.conf" (or the merged unit equivalent) for each node by calling systemd.ShowProperty(context.TODO(), "stalld.service", "DropInPaths", &node) and Expect the output to ContainSubstring that full drop-in path alongside the existing EnvironmentFiles check.
🤖 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 `@test/e2e/performanceprofile/functests/1_performance/performance.go`:
- Around line 346-353: The test "Should have the stalld service configured with
the stalld-backend drop-in" only checks EnvironmentFiles and can yield false
positives; update the test to also assert the systemd "DropInPaths" property
contains the actual drop-in file path
"/etc/systemd/system/stalld.service.d/stalld-backend.conf" (or the merged unit
equivalent) for each node by calling systemd.ShowProperty(context.TODO(),
"stalld.service", "DropInPaths", &node) and Expect the output to
ContainSubstring that full drop-in path alongside the existing EnvironmentFiles
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0da1eb5e-b8ec-4d5f-90e1-2c9c39764110
📒 Files selected for processing (13)
pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gotest/e2e/performanceprofile/functests/1_performance/performance.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/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
✅ Files skipped from review due to trivial changes (2)
- test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/manual_machineconfig.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/no-mcp/openshift-bootstrap-worker_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yaml
- 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
- 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/bootstrap/no-mcp/openshift-bootstrap-master_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go
bartwensley
force-pushed
the
stalld-backend
branch
from
c2837d5 to
e3ed733
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
`@pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go`:
- Around line 273-275: The test dereferences f.Contents.Source directly which
can panic; before using it (where base64Data :=
strings.TrimPrefix(*f.Contents.Source, ...), decoded, err :=
base64.StdEncoding.DecodeString(...), Expect(err).ToNot(HaveOccurred())), add a
guard such as Expect(f.Contents).ToNot(BeNil()) and
Expect(f.Contents.Source).ToNot(BeNil()) (or an if f.Contents == nil ||
f.Contents.Source == nil { Fail("file source is nil") } ) so the test fails with
a clear assertion instead of panicking, then proceed to TrimPrefix/DecodeString
as before.
🪄 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: ec84fbcd-de3c-4439-98bc-ea346bb87c36
📒 Files selected for processing (13)
pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.gopkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.gotest/e2e/performanceprofile/functests/1_performance/performance.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/manual_machineconfig.yamltest/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
✅ Files skipped from review due to trivial changes (3)
- 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/default/manual_machineconfig.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- test/e2e/performanceprofile/functests/1_performance/performance.go
- test/e2e/performanceprofile/testdata/render-expected-output/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/arm/manual_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/no-ref/manual_machineconfig.yaml
- pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-master_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/bootstrap/extra-mcp/openshift-bootstrap-worker_machineconfig.yaml
- test/e2e/performanceprofile/testdata/render-expected-output/default/cpuFrequency/manual_machineconfig.yaml
Updating the MC to revert the stalld backend to sched_debug. This is necessary because the new default queue_task backend causes an unacceptable latency penalty (see RHEL-175242). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Bart Wensley <bwensley@redhat.com>
bartwensley
force-pushed
the
stalld-backend
branch
from
e3ed733 to
a1e094a
Compare
|
/retest |
|
/lgtm |
|
/retest-required |
|
/retest |
|
@bartwensley: 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. |
3 participants
Updating the MC to revert the stalld backend to sched_debug. This is necessary because the new default queue_task backend causes an unacceptable latency penalty (see RHEL-175242).
Summary by CodeRabbit
New Features
Tests