Skip to content
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

Do not import plugins unnecessarily. #293

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Do not import plugins unnecessarily. #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2d17ee
Do not import plugins unnecessarily.
Random-Liu Jun 14, 2019
d520ca8
Build node-problem-detector from a directory.
Random-Liu Jun 14, 2019
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ version:
-o bin/node-problem-detector \
-ldflags '-X $(PKG)/pkg/version.version=$(VERSION)' \
$(BUILD_TAGS) \
cmd/node_problem_detector.go
./cmd/node-problem-detector

Dockerfile: Dockerfile.in
sed -e 's|@BASEIMAGE@|$(BASEIMAGE)|g' $< >$@
Expand Down
0 cmd/node_problem_detector.go → ...problem-detector/node_problem_detector.go
View file
Open in desktop
File renamed without changes.
0 cmd/plugins.go → cmd/node-problem-detector/plugins.go
View file
Open in desktop
File renamed without changes.
43 changes: 29 additions & 14 deletions cmd/options/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ import (

"github.com/spf13/pflag"

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

Expand Down Expand Up @@ -80,7 +78,11 @@ type NodeProblemDetectorOptions struct {
}

func NewNodeProblemDetectorOptions() *NodeProblemDetectorOptions {
return &NodeProblemDetectorOptions{MonitorConfigPaths: types.ProblemDaemonConfigPathMap{}}
npdo := &NodeProblemDetectorOptions{MonitorConfigPaths: types.ProblemDaemonConfigPathMap{}}
for _, problemDaemonName := range problemdaemon.GetProblemDaemonNames() {
npdo.MonitorConfigPaths[problemDaemonName] = &[]string{}
}
return npdo
}

// AddFlags adds node problem detector command line options to pflag.
Expand Down Expand Up @@ -108,7 +110,6 @@ func (npdo *NodeProblemDetectorOptions) AddFlags(fs *pflag.FlagSet) {
"127.0.0.1", "The address to bind the Prometheus scrape endpoint.")

for _, problemDaemonName := range problemdaemon.GetProblemDaemonNames() {
npdo.MonitorConfigPaths[problemDaemonName] = &[]string{}
fs.StringSliceVar(
npdo.MonitorConfigPaths[problemDaemonName],
"config."+string(problemDaemonName),
Expand Down Expand Up @@ -142,34 +143,48 @@ func (npdo *NodeProblemDetectorOptions) ValidOrDie() {
}
}

// Plugin names for custom plugin monitor and system log monitor.
// Hard code them here to:
// 1) Handle deprecated flags for --system-log-monitors and --custom-plugin-monitors.
// 2) Avoid direct dependencies to packages in those plugins, so that those plugins
// can be disabled at compile time.
const (
customPluginMonitorName = "custom-plugin-monitor"
systemLogMonitorName = "system-log-monitor"
)

// SetConfigFromDeprecatedOptionsOrDie sets NPD option using deprecated options.
func (npdo *NodeProblemDetectorOptions) SetConfigFromDeprecatedOptionsOrDie() {
if len(npdo.SystemLogMonitorConfigPaths) != 0 {
if npdo.MonitorConfigPaths[systemlogmonitor.SystemLogMonitorName] == nil {
npdo.MonitorConfigPaths[systemlogmonitor.SystemLogMonitorName] = &[]string{}
if npdo.MonitorConfigPaths[systemLogMonitorName] == nil {
// As long as the problem daemon is registered, MonitorConfigPaths should
// not be nil.
panic("System log monitor is not supported")
}

if len(*npdo.MonitorConfigPaths[systemlogmonitor.SystemLogMonitorName]) != 0 {
if len(*npdo.MonitorConfigPaths[systemLogMonitorName]) != 0 {
panic("Option --system-log-monitors is deprecated in favor of --config.system-log-monitor. They cannot be set at the same time.")
}

*npdo.MonitorConfigPaths[systemlogmonitor.SystemLogMonitorName] = append(
*npdo.MonitorConfigPaths[systemlogmonitor.SystemLogMonitorName],
*npdo.MonitorConfigPaths[systemLogMonitorName] = append(
*npdo.MonitorConfigPaths[systemLogMonitorName],
npdo.SystemLogMonitorConfigPaths...)
npdo.SystemLogMonitorConfigPaths = []string{}
}

if len(npdo.CustomPluginMonitorConfigPaths) != 0 {
if npdo.MonitorConfigPaths[custompluginmonitor.CustomPluginMonitorName] == nil {
npdo.MonitorConfigPaths[custompluginmonitor.CustomPluginMonitorName] = &[]string{}
if npdo.MonitorConfigPaths[customPluginMonitorName] == nil {
// As long as the problem daemon is registered, MonitorConfigPaths should
// not be nil.
panic("Custom plugin monitor is not supported")
}

if len(*npdo.MonitorConfigPaths[custompluginmonitor.CustomPluginMonitorName]) != 0 {
if len(*npdo.MonitorConfigPaths[customPluginMonitorName]) != 0 {
panic("Option --custom-plugin-monitors is deprecated in favor of --config.custom-plugin-monitor. They cannot be set at the same time.")
}

*npdo.MonitorConfigPaths[custompluginmonitor.CustomPluginMonitorName] = append(
*npdo.MonitorConfigPaths[custompluginmonitor.CustomPluginMonitorName],
*npdo.MonitorConfigPaths[customPluginMonitorName] = append(
*npdo.MonitorConfigPaths[customPluginMonitorName],
npdo.CustomPluginMonitorConfigPaths...)
npdo.CustomPluginMonitorConfigPaths = []string{}
}
Expand Down
61 changes: 38 additions & 23 deletions cmd/options/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (

"github.com/stretchr/testify/assert"

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

Expand Down Expand Up @@ -255,15 +253,15 @@ func TestSetConfigFromDeprecatedOptionsOrDie(t *testing.T) {
name: "no deprecated options",
orig: NodeProblemDetectorOptions{
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-a", "config-b"},
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-c", "config-d"},
systemLogMonitorName: &[]string{"config-a", "config-b"},
customPluginMonitorName: &[]string{"config-c", "config-d"},
},
},
expectPanic: false,
wanted: NodeProblemDetectorOptions{
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-a", "config-b"},
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-c", "config-d"},
systemLogMonitorName: &[]string{"config-a", "config-b"},
customPluginMonitorName: &[]string{"config-c", "config-d"},
},
},
},
Expand All @@ -272,13 +270,16 @@ func TestSetConfigFromDeprecatedOptionsOrDie(t *testing.T) {
orig: NodeProblemDetectorOptions{
SystemLogMonitorConfigPaths: []string{"config-a", "config-b"},
CustomPluginMonitorConfigPaths: []string{"config-c", "config-d"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
customPluginMonitorName: &[]string{},
systemLogMonitorName: &[]string{},
},
},
expectPanic: false,
wanted: NodeProblemDetectorOptions{
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-a", "config-b"},
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-c", "config-d"},
systemLogMonitorName: &[]string{"config-a", "config-b"},
customPluginMonitorName: &[]string{"config-c", "config-d"},
},
},
},
Expand All @@ -287,14 +288,15 @@ func TestSetConfigFromDeprecatedOptionsOrDie(t *testing.T) {
orig: NodeProblemDetectorOptions{
SystemLogMonitorConfigPaths: []string{"config-a", "config-b"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-c", "config-d"},
customPluginMonitorName: &[]string{"config-c", "config-d"},
systemLogMonitorName: &[]string{},
},
},
expectPanic: false,
wanted: NodeProblemDetectorOptions{
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-a", "config-b"},
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-c", "config-d"},
systemLogMonitorName: &[]string{"config-a", "config-b"},
customPluginMonitorName: &[]string{"config-c", "config-d"},
},
},
},
Expand All @@ -303,14 +305,15 @@ func TestSetConfigFromDeprecatedOptionsOrDie(t *testing.T) {
orig: NodeProblemDetectorOptions{
CustomPluginMonitorConfigPaths: []string{"config-a", "config-b"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-c", "config-d"},
customPluginMonitorName: &[]string{},
systemLogMonitorName: &[]string{"config-c", "config-d"},
},
},
expectPanic: false,
wanted: NodeProblemDetectorOptions{
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-c", "config-d"},
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-a", "config-b"},
systemLogMonitorName: &[]string{"config-c", "config-d"},
customPluginMonitorName: &[]string{"config-a", "config-b"},
},
},
},
Expand All @@ -319,30 +322,42 @@ func TestSetConfigFromDeprecatedOptionsOrDie(t *testing.T) {
orig: NodeProblemDetectorOptions{
SystemLogMonitorConfigPaths: []string{"config-a"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-b"},
systemLogMonitorName: &[]string{"config-b"},
},
},
expectPanic: true,
wanted: NodeProblemDetectorOptions{
},
{
name: "using deprecated & new options on CustomPluginMonitor",
orig: NodeProblemDetectorOptions{
CustomPluginMonitorConfigPaths: []string{"config-a"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
systemlogmonitor.SystemLogMonitorName: &[]string{"config-b"},
customPluginMonitorName: &[]string{"config-b"},
},
},
expectPanic: true,
},
{
name: "using deprecated & new options on CustomPluginMonitor",
name: "using deprecated options when SystemLogMonitor is not registered",
orig: NodeProblemDetectorOptions{
CustomPluginMonitorConfigPaths: []string{"config-a"},
SystemLogMonitorConfigPaths: []string{"config-a"},
CustomPluginMonitorConfigPaths: []string{"config-b"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-b"},
customPluginMonitorName: &[]string{},
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably change to:

types.ProblemDaemonConfigPathMap{
    customPluginMonitorName: &[]string{},
}

And similar for below test case,

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh just realized that github does not highlight the selected code.
So to clarify:
I think we should change MonitorConfigPaths: types.ProblemDaemonConfigPathMap{}, into

MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
    customPluginMonitorName: &[]string{},
},

Copy link
Member Author

@Random-Liu Random-Liu Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not needed, because we are not using custom plugin monitor deprecated flag, it is fine to not register that plugin. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

expectPanic: true,
wanted: NodeProblemDetectorOptions{
},
{
name: "using deprecated options when CustomPluginMonitor is not registered",
orig: NodeProblemDetectorOptions{
SystemLogMonitorConfigPaths: []string{"config-a"},
CustomPluginMonitorConfigPaths: []string{"config-b"},
MonitorConfigPaths: types.ProblemDaemonConfigPathMap{
custompluginmonitor.CustomPluginMonitorName: &[]string{"config-b"},
systemLogMonitorName: &[]string{},
},
},
expectPanic: true,
},
}

Expand Down