Skip to content

custom-plugin-monitor: Honor default condition status #1266

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
halaney wants to merge 1 commit into
base: master
Choose a base branch
from
+31 −1
Open

custom-plugin-monitor: Honor default condition status #1266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion pkg/custompluginmonitor/custom_plugin_monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,15 @@ func initialConditions(defaults []types.Condition) []types.Condition {
conditions := make([]types.Condition, len(defaults))
copy(conditions, defaults)
for i := range conditions {
conditions[i].Status = types.False
switch conditions[i].Status {
case types.True, types.False, types.Unknown:
case "":
conditions[i].Status = types.False
default:
klog.Warningf("Condition %q has invalid initial status %q, defaulting to %q",
conditions[i].Type, conditions[i].Status, types.False)
conditions[i].Status = types.False
}

@DigitalVeer DigitalVeer Jul 8, 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.

Currently none of the existing configs/*.json files with "plugin": "custom" actually set status on a condition. This means Status would now come in as "" which doesn't match True/False/Unknown, and by the new logic, go to the default branch and log a warning.

That means each CustomPluginMonitor condition without status will log a warning once at startup ( Condition "NTPProblem" has invalid initial status "", defaulting to "False". )

It may be worth honoring the default as such:

    case "":
        conditions[i].Status = types.False

@halaney halaney Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hah, duh! thanks I'll make that change. We've been running this (plus my other open PRs) in our setup, but we use our own custom plugins and do set that.

conditions[i].Transition = time.Now()
}
return conditions
Expand Down
22 changes: 22 additions & 0 deletions pkg/custompluginmonitor/custom_plugin_monitor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,32 @@ import (
"github.com/stretchr/testify/assert"

"k8s.io/node-problem-detector/pkg/problemdaemon"
"k8s.io/node-problem-detector/pkg/types"
)

func TestRegistration(t *testing.T) {
assert.NotPanics(t,
func() { problemdaemon.GetProblemDaemonHandlerOrDie("custom-plugin-monitor") },
"Custom plugin monitor failed to register itself as a problem daemon.")
}

func TestInitialConditions(t *testing.T) {
tests := []struct {
name string
inputStatus types.ConditionStatus
expectedStatus types.ConditionStatus
}{
{name: "TestTrue", inputStatus: types.True, expectedStatus: types.True},
{name: "TestFalse", inputStatus: types.False, expectedStatus: types.False},
{name: "TestUnknown", inputStatus: types.Unknown, expectedStatus: types.Unknown},
{name: "TestUnset", inputStatus: "", expectedStatus: types.False},
{name: "TestInvalid", inputStatus: "garbage", expectedStatus: types.False},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defaults := []types.Condition{{Type: "TestCondition", Status: tt.inputStatus}}
conditions := initialConditions(defaults)
assert.Equal(t, tt.expectedStatus, conditions[0].Status)
})
}
}