feat: Support configuring rule.Reason as a Sprintf format string#1068
feat: Support configuring rule.Reason as a Sprintf format string#1068jessehu wants to merge 3 commits into
Conversation
|
Hi @jessehu. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jessehu 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 |
24e9230 to
411a1ba
Compare
|
/ok-to-test |
|
/retest |
|
Thanks @hakman for review! Looks like pull-npd-e2e-test failed twice. Not sure if it's related to this PR ? |
Unrelated to the PR, please ignore. |
|
/retest |
|
Thanks @hakman. 1 test passed now but 2 tests still failed. Please take a look again. |
|
/retest |
|
/retest |
So we can define one templated Reason to generate many Reasons, e.g. generating Reasons for Nvidia GPU Xid errors in dmesg log.
ea452dd to
50016aa
Compare
|
hi @hakman would you mind review this PR again please? The pull-npd-e2e-test passed now. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
/remove-lifecycle rotten |
|
i also experimented with writing a feature like this, but abandoned it due to the cleanup ramifications. if you set a bunch of similar conditions on nodes they hang around until the node is deleted, will inflate prometheus metrics cardinality etc. i wrote a feature to delete deprecated conditions (conditions that have been removed from config) on startup that wasn't reviewed / approved here that could help with this fact #1022 -- this is in use in my org with great success interested to hear what others think about this situation |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
|
||
| reason := rule.Reason | ||
| // Support configuring rule.Reason as a Sprintf format string and formatting it with the matched capturing groups in rule.Pattern. | ||
| if strings.Contains(reason, "%") { |
There was a problem hiding this comment.
This turns every % into a template.
"Disk90%" becomes --> "Disk90%!(NOVERB)" and can cause us to fail to match.
There was a problem hiding this comment.
@DigitalVeer An invalid template string like "Disk90%" will cause NPD process to panic when launching, so the user can find it out easily rather than not matching silently.
There was a problem hiding this comment.
Thanks, Jesse. I tried to reproduce the panic with a new filelog monitor:
{ "plugin": "filelog", "pluginConfig": { "timestamp": "^(\\S+)", "message": "^\\S+ (.*)$", "timestampFormat": "2006-01-02T15:04:05Z07:00" }, "logPath": "/repro/pr_1068_literal_percent.log", "lookback": "24h", "bufferSize": 1, "source": "literal-percent-e2e", "metricsReporting": true, "conditions": [], "rules": [ { "type": "temporary", "reason": "Disk90%", "pattern": "disk usage reached 90%" } ] }
I wrote this line to the log file:
2026-07-15T09:58:59Z disk usage reached 90%
NPD started normally. After filelog read and matched the line, NPD logged:
Got wrong string "Disk90%!(NOVERB)" for reason "Disk90%" with pattern "disk usage reached 90%"
The process remained alive, but NPD generated no status or event. I could not reproduce either a launch or runtime panic. The match is dropped at runtime instead.
If the goal is to surface bad reason strings easily one option is validating them in ValidateRules which would let us catch them at startup.
There was a problem hiding this comment.
@DigitalVeer I see. "pattern": "disk usage reached 90%" is a valid regex pattern,so NPD doesn't panic in ValidateRules, but "reason": "Disk90%" is not a valid Sprintf templated string. I have added validation for reason in ValidateRules().
Dry-run fmt.Sprintf against the pattern's capturing group count so misconfigured Reason templates (wrong verb count or unsupported verbs for string args) fail at config load time instead of being silently dropped at runtime.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jessehu 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 |
|
@jessehu: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |


So we can define one templated Reason to generate many Reasons, e.g. generating Reasons for Nvidia GPU Xid errors in dmesg log.
Close #1067