OCPBUGS-74211: fix: tolerate all node taints for insights-runtime-extractor DaemonSet - #1325
Conversation
The DaemonSet only tolerated a fixed set of well-known taints, so custom taints (e.g. node-role.kubernetes.io/gpu) blocked scheduling on those nodes. Add a wildcard toleration (operator: Exists) so it runs on every node regardless of taints. OCPBUGS-74211
openshift-ci-robot
added
jira/valid-reference
|
@akshitkumawat: This pull request references Jira Issue OCPBUGS-74211, which is invalid:
Comment 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Runtime Extractor DaemonSet now tolerates all node taints and uses required node affinity to target worker nodes while excluding master and control-plane nodes. Unit tests verify both scheduling behaviors. ChangesRuntime Extractor scheduling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akshitkumawat 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @akshitkumawat. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/runtimeextractor/resources/daemonset_test.go (2)
33-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the new Go test table-driven.
The added test hard-codes one case, while repository guidelines require table-driven tests in Go test files. Put the expected toleration fields in a test-case table and iterate with
t.Run.As per coding guidelines, Go test files must use table-driven tests.
🤖 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/controller/runtimeextractor/resources/daemonset_test.go` around lines 33 - 46, Convert Test_loadRuntimeExtractorDaemonSet_TolerateAllTaints into a table-driven test by defining cases containing the expected toleration operator, key, and effect, then iterate over them with t.Run. Keep the existing DaemonSet loading and assertions, but validate the toleration fields from each case instead of hard-coding them.Source: Coding guidelines
37-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse fatal assertions before indexing
tolerations.
assert.NoError,assert.NotNil, andassert.Lenrecord failures but continue execution. If loading fails or the count is unexpected,tolerations[0]can panic and mask the real assertion failure; use therequireequivalents before the indexed assertions.🤖 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/controller/runtimeextractor/resources/daemonset_test.go` around lines 37 - 45, Update the test around loadRuntimeExtractorDaemonSet to use require.NoError, require.NotNil, and require.Len before indexing tolerations. Keep the existing indexed toleration assertions unchanged, ensuring failures stop execution before tolerations[0] is accessed.
🤖 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/controller/runtimeextractor/resources/daemonset_test.go`:
- Around line 33-46: Convert
Test_loadRuntimeExtractorDaemonSet_TolerateAllTaints into a table-driven test by
defining cases containing the expected toleration operator, key, and effect,
then iterate over them with t.Run. Keep the existing DaemonSet loading and
assertions, but validate the toleration fields from each case instead of
hard-coding them.
- Around line 37-45: Update the test around loadRuntimeExtractorDaemonSet to use
require.NoError, require.NotNil, and require.Len before indexing tolerations.
Keep the existing indexed toleration assertions unchanged, ensuring failures
stop execution before tolerations[0] is accessed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 87570755-c983-4bdb-aa36-4fe68eacedbd
📒 Files selected for processing (2)
pkg/controller/runtimeextractor/resources/daemonset_test.gopkg/controller/runtimeextractor/resources/manifests/runtime-extractor-daemonset.yaml
|
/ok-to-test |
|
/cc @jmesnil |
openshift-ci
Bot
added
the
ok-to-test
openshift-ci
Bot
removed
the
needs-ok-to-test
| # Tolerate all taints so this DaemonSet schedules on every node, including | ||
| # nodes with custom taints (e.g. node-role.kubernetes.io/gpu), consistent | ||
| # with its purpose of running on every node in the cluster. |
There was a problem hiding this comment.
I would remove the comment since the field is fairly self explanatory
| // Test_loadRuntimeExtractorDaemonSet_TolerateAllTaints ensures the DaemonSet tolerates | ||
| // all node taints (including custom ones added by cluster admins), rather than relying | ||
| // on the fixed set of well-known taints the DaemonSet controller tolerates by default. | ||
| func Test_loadRuntimeExtractorDaemonSet_TolerateAllTaints(t *testing.T) { | ||
| ds, err := loadRuntimeExtractorDaemonSet() | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, ds) | ||
|
|
||
| tolerations := ds.Spec.Template.Spec.Tolerations | ||
| assert.Len(t, tolerations, 1, "expected a single wildcard toleration") | ||
| assert.Equal(t, corev1.TolerationOpExists, tolerations[0].Operator) | ||
| assert.Empty(t, tolerations[0].Key, "key must be empty to match all taints") | ||
| assert.Empty(t, tolerations[0].Effect, "effect must be empty to match all taint effects") | ||
| } |
There was a problem hiding this comment.
I would also drop the test since it isn't testing any logic and therefore doesn't add much value.
There was a problem hiding this comment.
@opokornyy thanks for your inputs. I have pushed the new changes to the branch.
|
@akshitkumawat Failures in the ci/prow/insights-runtime-extractor-tests seem to be related to this change |
The wildcard toleration let the DaemonSet schedule onto control-plane nodes too, breaking the e2e test that expects pods only on worker nodes. Add a nodeAffinity that excludes control-plane-only nodes while still allowing compact/SNO nodes that are both control-plane and worker.
|
@akshitkumawat: 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. |
3 participants
Summary
ran on the fixed set of taints the DaemonSet controller tolerates by default
(not-ready, unreachable, disk-pressure, memory-pressure, pid-pressure, unschedulable).
the DaemonSet's pods from scheduling on that node, leaving incomplete cluster coverage.
operator: Exists) to the DaemonSet pod spec so itschedules on every node regardless of taints, per Kubernetes best practice for
cluster-wide DaemonSets.
Changes
pkg/controller/runtimeextractor/resources/manifests/runtime-extractor-daemonset.yaml:add wildcard toleration.
pkg/controller/runtimeextractor/resources/daemonset_test.go: addTest_loadRuntimeExtractorDaemonSet_TolerateAllTaintsverifying the toleration.References
https://issues.redhat.com/browse/OCPBUGS-74211
Test plan
go test ./pkg/controller/runtimeextractor/...passes, including new and existing tests.go build ./pkg/controller/runtimeextractor/...succeeds.Summary by CodeRabbit