use Patch-based event dedup in EmitNodeWarningEvent - #2569
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jianzhangbjz 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 |
github-actions
Bot
added
kind/enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWarning event flow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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)
pkg/util/util.go (1)
285-297: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd event correlation before creating node warning events.
GenerateNameplusEvents(...).Create(...)creates a new Event for each successful call. This path does not invoke client-go’sEventCorrelator, which performs client-side filtering, aggregation, and count updates. Repeated registration can therefore produce repeated warning Events. Remove the API-server deduplication claim, or add local rate limiting or correlation with the required update permissions.🤖 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/util/util.go` around lines 285 - 297, Update EmitNodeWarningEvent to stop claiming that the API server automatically deduplicates these events, and add local event correlation or rate limiting before creating warning Events so repeated calls are aggregated or suppressed. Ensure any correlation path can update existing Events with the required permissions while preserving the current client and timeout handling.
🤖 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 `@pkg/util/util.go`:
- Around line 285-297: Update EmitNodeWarningEvent to stop claiming that the API
server automatically deduplicates these events, and add local event correlation
or rate limiting before creating warning Events so repeated calls are aggregated
or suppressed. Ensure any correlation path can update existing Events with the
required permissions while preserving the current client and timeout handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6c46aef-822e-40fe-8119-0964df235e3b
📒 Files selected for processing (3)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/util/util.gopkg/util/util_test.go
|
This is being closed because it does not comply with the contribution guidelines. |
|
/reopen |
|
@jianzhangbjz: Failed to re-open PR: state cannot be changed. The fix-events-update-permission branch was force-pushed or recreated. 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. |
|
Hi @mesutoezdil , I've updated it. Could you help have a review? Thanks! |
|
/reopen |
|
@jianzhangbjz: Reopened this PR. 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. |
87ac7b5 to
daeeed8
Compare
| // EmitNodeWarningEvent emits a Warning event on the given Node with deduplication. | ||
| func EmitNodeWarningEvent(node *corev1.Node, reason, message string, dedupWindow time.Duration) { | ||
| // EmitNodeWarningEvent emits a Warning event on the given Node. | ||
| // The Kubernetes API server handles event deduplication automatically |
There was a problem hiding this comment.
is this true for a plain create with generatename? each call makes a new object, no shared name to merge on. if this fires every cycle on a stuck node, will it spam many duplicate events instead of one counted event?
There was a problem hiding this comment.
You're right! However, it doesn't trigger every 30-second cycle; it only triggers when an anomaly is first detected in the GPU topology state. Anyway, let me make a more secure solution.
| "One or more GPU pairs on this node have asymmetric P2P link data; "+ | ||
| "affected pairs scored 0 (possible NVLink hardware or driver issue)", | ||
| time.Hour) | ||
| ) |
There was a problem hiding this comment.
stray comma and closing paren on its own line here. gofmt would not leave it like this.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 36 files with indirect coverage changes 🚀 New features to boost your workflow:
|
daeeed8 to
f5d9aa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@charts/hami/templates/scheduler/clusterrole.yaml`:
- Line 20: Update the device-plugin monitor ClusterRole’s events rule to include
the patch verb alongside its existing permissions. Do not change the scheduler
ClusterRole, since it is bound only to the scheduler service account.
In `@pkg/util/util_test.go`:
- Line 790: Update both tests around the client.KubeClient assignments to save
the existing client value before replacing it, then register t.Cleanup to
restore that value after each test. Ensure cleanup is applied independently to
both tests so package-global client state cannot leak between tests.
- Around line 823-846: Update the test around EmitNodeWarningEvent to assert the
decoy event Create succeeds, expect two events from the unfiltered List, and
locate the event whose InvolvedObject.UID matches nodeUID before asserting its
Count and Message. Keep the decoy event present so the test verifies UID-based
deduplication.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8c9c724-0c8d-4046-ad74-3d0c47748553
📒 Files selected for processing (4)
charts/hami/templates/scheduler/clusterrole.yamlpkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/util/util.gopkg/util/util_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go
- pkg/util/util.go
jianzhangbjz
changed the title
counts, which only needs the "patch" verb. Signed-off-by: Jian Zhang <jiazha666@gmail.com>
f5d9aa5 to
27e8958
Compare
|
Hi @mesutoezdil , I added the |
| - list | ||
| - create | ||
| - update | ||
| - patch |
There was a problem hiding this comment.
emitnodewarningevent has exactly one caller, register.go, which runs under this exact role. this role already had update on events before this pr. so the original update() call was never actually forbidden, rbaccheck flagged it against the scheduler clusterrole instead, which never calls this function at all. does that mean the real bug here was never an rbac violation, just rbaccheck checking the wrong role file?
There was a problem hiding this comment.
Yes, correct. Thanks! I've updated #2567 to avoid this kind of false alarm. Closed it.
|
/close as not a bug |
|
/close |
|
@jianzhangbjz: Closed this PR. 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. |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Replace
Events().Update()withEvents().Patch()inEmitNodeWarningEventto fix an RBAC permission violation. The scheduler ClusterRole only grantscreate, get, liston events — callingUpdate()requires theupdateverb which wasn't granted.The previous attempt removed client-side event dedup entirely, but that would create duplicate events on repeated calls since the API server does not deduplicate by involvedObject+reason. Instead, this uses
Patchto update the count and timestamp of matching events, and addspatchto the events verbs in the scheduler ClusterRole.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Discovered by the
rbaccheckGo AST tool(#2567) that flags RBAC permission violations at CI time.Does this PR introduce a user-facing change?:
No — events are still emitted, just without client-side deduplication. The API server handles deduplication automatically.
This PR was written primarily by Claude Code.
Summary by CodeRabbit