Skip to content

feat: Support configuring rule.Reason as a Sprintf format string #1068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jessehu wants to merge 3 commits into
base: master
Choose a base branch
from
+213 −5
Open

feat: Support configuring rule.Reason as a Sprintf format string #1068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fd1dff
feat: Support configuring rule.Reason as a Sprintf format string
jessehu Jun 11, 2025
50016aa
fix: Allow capturing groups in rule.Pattern with non-templated rule.R…
jessehu Aug 20, 2025
aea47a2
feat: Validate rule.Reason as a Sprintf format string in ValidateRules
jessehu Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion pkg/systemlogmonitor/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ limitations under the License.
package systemlogmonitor

import (
"fmt"
"regexp"
"strings"

watchertypes "k8s.io/node-problem-detector/pkg/systemlogmonitor/logwatchers/types"
systemlogtypes "k8s.io/node-problem-detector/pkg/systemlogmonitor/types"
Expand Down Expand Up @@ -62,10 +64,23 @@ func (mc *MonitorConfig) ApplyDefaultConfiguration() {
// ValidateRules verifies whether the regular expressions in the rules are valid.
func (mc MonitorConfig) ValidateRules() error {
for _, rule := range mc.Rules {
_, err := regexp.Compile(rule.Pattern)
re, err := regexp.Compile(rule.Pattern)
if err != nil {
return err
}
// If rule.Reason is used as a Sprintf format string, dry-run it against
// the pattern's capturing groups to verify the verbs line up. This
// mirrors the runtime behavior in generateStatus.
if strings.Contains(rule.Reason, "%") {
args := make([]interface{}, re.NumSubexp())
for i := range args {
args[i] = ""
}
if formatted := fmt.Sprintf(rule.Reason, args...); strings.Contains(formatted, "%!") {
return fmt.Errorf("invalid Sprintf format reason %q for pattern %q: got %q",
rule.Reason, rule.Pattern, formatted)
}
}
}
return nil
}
34 changes: 30 additions & 4 deletions pkg/systemlogmonitor/log_monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"encoding/json"
"fmt"
"os"
"regexp"
"strings"
"time"

"k8s.io/klog/v2"
Expand Down Expand Up @@ -94,6 +96,11 @@ func NewLogMonitorOrDie(configPath string) types.Monitor {
// panic if error occurs.
func initializeProblemMetricsOrDie(rules []systemlogtypes.Rule) {
for _, rule := range rules {
// Skip template reasons (e.g. "NvidiaGPUXid%s") — they are expanded at match time
// and pushing the raw template string produces meaningless Prometheus label values.
if strings.Contains(rule.Reason, "%") {
continue
}
if rule.Type == types.Perm {
err := problemmetrics.GlobalProblemMetricsManager.SetProblemGauge(rule.Condition, rule.Reason, false)
if err != nil {
Expand Down Expand Up @@ -158,6 +165,9 @@ func (l *logMonitor) parseLog(log *systemlogtypes.Log) {
continue
}
status := l.generateStatus(matched, rule)
if status == nil {
continue
}
klog.Infof("New status generated: %+v", status)
l.output <- status
}
Expand All @@ -170,12 +180,28 @@ func (l *logMonitor) generateStatus(logs []*systemlogtypes.Log, rule systemlogty
message := generateMessage(logs, rule.PatternGeneratedMessageSuffix)
var events []types.Event
var changedConditions []*types.Condition

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, "%") {

@DigitalVeer DigitalVeer Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns every % into a template.

"Disk90%" becomes --> "Disk90%!(NOVERB)" and can cause us to fail to match.

@jessehu jessehu Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@DigitalVeer DigitalVeer Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jessehu jessehu Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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().

re := regexp.MustCompile(rule.Pattern)
matches := re.FindStringSubmatch(message)
formatArgs := make([]interface{}, 0)
if len(matches) > 1 {
// Use the matched capturing groups as the arguments for Sprintf.
for _, value := range matches[1:] {
formatArgs = append(formatArgs, value)
}
}
reason = fmt.Sprintf(rule.Reason, formatArgs...)
}

if rule.Type == types.Temp {
// For temporary error only generate event
events = append(events, types.Event{
Severity: types.Warn,
Timestamp: timestamp,
Reason: rule.Reason,
Reason: reason,
Message: message,
})
} else {
Expand All @@ -186,19 +212,19 @@ func (l *logMonitor) generateStatus(logs []*systemlogtypes.Log, rule systemlogty
// Update transition timestamp and message when the condition
// changes. Condition is considered to be changed only when
// status or reason changes.
if condition.Status == types.False || condition.Reason != rule.Reason {
if condition.Status == types.False || condition.Reason != reason {
condition.Transition = timestamp
condition.Message = message
events = append(events, util.GenerateConditionChangeEvent(
condition.Type,
types.True,
rule.Reason,
reason,
message,
timestamp,
))
}
condition.Status = types.True
condition.Reason = rule.Reason
condition.Reason = reason
changedConditions = append(changedConditions, condition)
break
}
Expand Down
167 changes: 167 additions & 0 deletions pkg/systemlogmonitor/log_monitor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,173 @@ func TestGenerateStatusForMetrics(t *testing.T) {
}
}

func TestGenerateStatusForEvents(t *testing.T) {
for c, test := range []struct {
name string
rule logtypes.Rule
expected *types.Status
logs []*logtypes.Log
}{
{
name: "without matching group",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "NvidiaGPUXid",
Pattern: "NVRM: Xid \\(PCI:[^)]+\\): \\d+,.*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
},
},
expected: &types.Status{
Source: testSource,
Events: []types.Event{{
Severity: types.Warn,
Timestamp: time.Unix(1000, 1000),
Reason: "NvidiaGPUXid",
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
}},
},
},
{
name: "one matching group",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "NvidiaGPUXid%s",
Pattern: "NVRM: Xid \\(PCI:[^)]+\\): (\\d+),.*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
},
},
expected: &types.Status{
Source: testSource,
Events: []types.Event{{
Severity: types.Warn,
Timestamp: time.Unix(1000, 1000),
Reason: "NvidiaGPUXid45",
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
}},
},
},
{
name: "two matching groups",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "NvidiaGPUXid%s, Ch%s",
Pattern: "NVRM: Xid \\(PCI:[^)]+\\): (\\d+), Ch (\\d+).*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
},
},
expected: &types.Status{
Source: testSource,
Events: []types.Event{{
Severity: types.Warn,
Timestamp: time.Unix(1000, 1000),
Reason: "NvidiaGPUXid45, Ch00000010",
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
}},
},
},
{
name: "not enough matching groups 1",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "NvidiaGPUXid%s, Ch%s",
Pattern: "NVRM: Xid \\(PCI:[^)]+\\): (\\d+),.*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
},
},
expected: nil,
},
{
name: "not enough matching groups 2",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "NvidiaGPUXid%s",
Pattern: "NVRM: Xid \\(PCI:[^)]+\\): \\d+,.*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
},
},
expected: nil,
},
{
name: "indexed matching groups",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "NvidiaGPUXid%[1]s, Ch%[2]s",
Pattern: "NVRM: Xid \\(PCI:[^)]+\\): (\\d+), Ch (\\d+).*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
},
},
expected: &types.Status{
Source: testSource,
Events: []types.Event{{
Severity: types.Warn,
Timestamp: time.Unix(1000, 1000),
Reason: "NvidiaGPUXid45, Ch00000010",
Message: "[...] NVRM: Xid (PCI:0000:00:00.0): 45, Ch 00000010",
}},
},
},
{
name: "using matching groups in Pattern but not in Reason",
rule: logtypes.Rule{
Type: types.Temp,
Reason: "OOMKilling",
Pattern: "Killed process \\d+ (.+) total-vm:\\d+kB, anon-rss:\\d+kB, file-rss:\\d+kB.*",
},
logs: []*logtypes.Log{
{
Timestamp: time.Unix(1000, 1000),
Message: "kernel: Killed process 13357 (mysqld), UID 27, total-vm:4977676kB, anon-rss:3256736kB, file-rss:0kB, shmem-rss:0kB",
},
},
expected: &types.Status{
Source: testSource,
Events: []types.Event{{
Severity: types.Warn,
Timestamp: time.Unix(1000, 1000),
Reason: "OOMKilling",
Message: "kernel: Killed process 13357 (mysqld), UID 27, total-vm:4977676kB, anon-rss:3256736kB, file-rss:0kB, shmem-rss:0kB",
}},
},
},
} {
l := &logMonitor{
config: MonitorConfig{
Source: testSource,
},
}
(&l.config).ApplyDefaultConfiguration()
got := l.generateStatus(test.logs, test.rule)

if !reflect.DeepEqual(test.expected, got) {
t.Errorf("case %d %s: expected status %+v, got %+v", c+1, test.name, test.expected, got)
}
}
}

func TestInitializeProblemMetricsOrDie(t *testing.T) {
testCases := []struct {
name string
Expand Down