add custom problem detector plugin #145

andyxning · 2017-10-18T08:45:22Z

This PR will add custom plugin problem detector interface to node-problem-detector.

Proposal: https://docs.google.com/document/d/1jK_5YloSYtboj-DtfjmYKxfNnUxCAvohLnsH5aGCAYQ/edit#

Major changes:

move ApplyDefaultConfiguration for MonitorConfig from function to method. Ref
move ValidateRules for MonitorConfig from function to method for. Ref
move k8s.io/node-problem-detector/pkg/systemlogmonitor/util to k8s.io/node-problem-detector/pkg/util/tomb. Ref
move Rule type for system log config to pkg k8s.io/node-problem-detector/pkg/types. Ref
move LogMonitor interface to k8s.io/node-problem-detector/pkg/types and rename it to Monitor. Ref
Code clean. Ref

This change is

andyxning · 2017-10-24T00:07:17Z

ping @Random-Liu @dchen1107 :)

andyxning · 2017-10-27T14:37:04Z

Ping @Random-Liu @dchen1107

Random-Liu · 2017-11-14T19:15:50Z

@andyxning I'll review this today.

andyxning · 2017-11-15T02:57:30Z

@Random-Liu #144 is also ready to review. :)

Random-Liu

Finished half. Please address comments or reply.
I'll continue review tomorrow.

Random-Liu · 2017-11-15T05:52:39Z

README.md

@@ -14,7 +14,7 @@ enabled by default in the GCE cluster.
 # Background
 There are tons of node problems could possibly affect the pods running on the
 node such as:
-* Hardware issues: Bad cpu, memory or disk;
+* Hardware issues: Bad cpu, memory or disk, ntp service down;


Is ntp service down a hardware issue?

Actually not. Just wan to add ntp as an example. :)

Will move to another type.

Add Infrastructure daemon issues type.

Random-Liu · 2017-11-15T05:53:17Z

README.md

@@ -44,32 +44,38 @@ A problem daemon could be:
 * An existing node health monitoring daemon integrated with node-problem-detector.

 Currently, a problem daemon is running as a goroutine in the node-problem-detector
-binary. In the future, we'll separate node-problem-detector and problem daemons into


Why removing this one? We are still not sure how this should work. :p

I am opinionated about this. :(

You're right. We have not concluded which way to run NPD with different problem daemons is the best one.

Random-Liu · 2017-11-15T05:54:03Z

README.md


 List of supported problem daemons:

 | Problem Daemon |  NodeCondition  | Description |
 |----------------|:---------------:|:------------|
 | [KernelMonitor](https://github.com/kubernetes/node-problem-detector/blob/master/config/kernel-monitor.json) | KernelDeadlock | A system log monitor monitors kernel log and reports problem according to predefined rules. |
 | [AbrtAdaptor](https://github.com/kubernetes/node-problem-detector/blob/master/config/abrt-adaptor.json) | None | Monitor ABRT log messages and report them further. ABRT (Automatic Bug Report Tool) is health monitoring daemon able to catch kernel problems as well as application crashes of various kinds occurred on the host. For more information visit the [link](https://github.com/abrt). |
+| [CustomPluginMonitor](https://github.com/kubernetes/node-problem-detector/blob/master/config/custom-plugin-monitor.json) | On-demand(According to users configuration) | A custom plugin monitor for node-problem-detector to invoke and check various node problems with user defined check scripts. [Proposal is here](https://docs.google.com/document/d/1jK_5YloSYtboj-DtfjmYKxfNnUxCAvohLnsH5aGCAYQ/edit#). |


s/Proposal is here/see proposal here

Random-Liu · 2017-11-15T05:55:26Z

README.md

+* `--custom-plugin-monitors`: List of paths to custom plugin monitor config files, comma separated, e.g.
+  [config/custom-plugin-monitor.json](https://github.com/kubernetes/node-problem-detector/blob/master/config/custom-plugin-monitor.json).
+  Node problem detector will start a separate custom plugin monitor for each configuration. You can
+    use different custom plugin monitors to monitor different node problems.


Random-Liu · 2017-11-15T05:56:04Z

README.md

-http://APISERVER_IP:APISERVER_PORT?inClusterConfig=false
-```
-Refer [heapster docs](https://github.com/kubernetes/heapster/blob/master/docs/source-configuration.md#kubernetes) for a complete list of available options.
+   ```


Why indent?

This is mainly used to make the code block indent with previous content in the same section.

Random-Liu · 2017-11-15T07:30:36Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+	// Validate configurations
+	err = l.config.Validate()
+	if err != nil {
+		glog.Fatalf("Failed to validate custom plugin config %+v. %v", l.config, err)


Random-Liu · 2017-11-15T07:33:10Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+	for {
+		select {
+		case result := <-resultChan:
+			glog.V(3).Infof("Receive new plugin result. %+v", result)


Random-Liu · 2017-11-15T07:33:40Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+
+// NewCustomPluginMonitorOrDie create a new customPluginMonitor, panic if error occurs.
+func NewCustomPluginMonitorOrDie(configPath string) types.Monitor {
+	l := &customPluginMonitor{


nit: do you want to change l to c or something else?

Will do. It is really difficult to recognize. :)

Random-Liu · 2017-11-15T07:52:07Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+		}
+	} else {
+		// For permanent error changes the condition
+		for i := range l.conditions {


Can we also generate an event XXChanged here? (Also do that for log monitor).

Then we don't need to add 2 entries for the same script.

IMHO, I prefer we separate events and conditions. This will make npd more flexible in case people we only need conditions aside from events.

Actually this is an issue I want to fix long time ago. We should always generate an event for condition switch, I think. We also do that in kubelet https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_node_status.go#L430.

Usually condition change is not obvious enough to user.

Random-Liu · 2017-11-15T07:52:51Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+		// For permanent error changes the condition
+		for i := range l.conditions {
+			condition := &l.conditions[i]
+			if condition.Type == result.Rule.Condition {


For custom plugin, I feel like the condition may change back, right?
For log it's hard to identify, but for plugin, we could purely change condition status based on plugin return value.

Yep. With the output of npd plugins, we should update the condition status accordingly. Will do.

andyxning · 2017-11-15T09:59:12Z

config/custom-plugin-monitor.json

+      "type": "temporary",
+      "reason": "NTPIsDown",
+      "path": "./config/plugin/check_ntp.sh",
+      "timeout": "3s"


This is the per plugin timeout config. :)

Random-Liu · 2017-11-16T05:13:57Z

config/custom-plugin-monitor.json

+  "plugin": "custom",
+  "pluginConfig": {
+    "invoke_interval": "30s",
+    "timeout": "5s",


I'm fine with this for now, but please add TODO in code. We should have per-rule interval.

Random-Liu · 2017-11-16T05:14:33Z

config/custom-plugin-monitor.json

+      "path": "./config/plugin/check_ntp.sh",
+      "timeout": "3s"
+    },
+    {


I think we should generate event for condition change. With that, you shouldn't need 2 rules here.

Random-Liu · 2017-11-16T05:21:24Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+					}
+					condition.Status = true
+					condition.Reason = result.Rule.Reason
+					break


Refactored. PTAL.

Random-Liu · 2017-11-16T05:22:44Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+		// For permanent error changes the condition
+		for i := range c.conditions {
+			condition := &c.conditions[i]
+			if condition.Type == result.Rule.Condition {


Combine the logic:

status = (result.ExitStatus >= cpmtypes.NonOK) if condition.Status != status || condition.Reason != result.Rule.Reason { condition.Transition = timestamp condition.Message = result.Message } condition.Status = status condition.Reason = result.Rule.Reason

And please generate event. :P

Once we have a conclusion on emitting events when condition change. I prefer adding this as a TODO and will be addressed in next PR. :)

Random-Liu · 2017-11-16T05:23:23Z

config/custom-plugin-monitor.json

+      "path": "./config/plugin/check_ntp.sh",
+      "timeout": "3s"
+    },
+    {


Sometimes, for the same condition, status is not changed, but reason is changed. Without event, people will not even notice that.

Random-Liu · 2017-11-16T05:32:03Z

pkg/custompluginmonitor/plugin/plugin.go

+			}
+
+			p.Wait()
+			glog.Info("End to run custom plugins")


Finish running custom plugins.

Random-Liu · 2017-11-16T05:39:24Z

pkg/custompluginmonitor/plugin/plugin.go

+	stdout, err := cmd.Output()
+	if err != nil {
+		if _, ok := err.(*exec.ExitError); !ok {
+			glog.Errorf("Error in running plugin %q. %v. %v", rule.Path, err, string(stdout))


glog.Errorf("Error in running plugin %q: error - %v. output - %q", rule.Path, err, string(stdout))

Random-Liu · 2017-11-16T05:40:03Z

pkg/custompluginmonitor/plugin/plugin.go

+
+	// trim suffix useless bytes
+	output = string(stdout)
+	output = strings.TrimSpace(output)


Trimspace should have trimmed newline, I think.

Thanks for pointing this out. Have not realized this before. :)

Random-Liu · 2017-11-16T05:49:57Z

pkg/custompluginmonitor/plugin/plugin.go

+	output = strings.TrimSpace(output)
+	output = strings.TrimRight(output, "\n")
+
+	if cmd.ProcessState.Sys().(syscall.WaitStatus).Signaled() {


If the script is stopped because of timeout, I hope we could clarify that in the message. Currently, it seems that we'll only get signal: killed.

Enhanced log.

Random-Liu · 2017-11-16T05:50:15Z

pkg/custompluginmonitor/plugin/plugin_test.go

+
+import (
+	"testing"
+


nit: remove empty line.

Random-Liu · 2017-11-16T05:53:09Z

@andyxning I have a meeting soon. After that I'll come back and finish review.

Random-Liu · 2017-11-16T07:28:35Z

pkg/custompluginmonitor/types/config.go

+import (
+	"fmt"
+	"time"
+


nit: remove empty line.

Random-Liu · 2017-11-16T07:37:36Z

pkg/custompluginmonitor/types/config.go

+type CustomPluginConfig struct {
+	// Plugin is the name of plugin which is currently used.
+	// Currently supported: custom.
+	Plugin string `json:"plugin, omitempty"`


Do we need this?

I have also thought this when i prepare this PR. In order to be consistent with existing log configs, i choose to retain this Plugin field and the only valid value is custom.

Random-Liu · 2017-11-16T07:38:07Z

pkg/custompluginmonitor/types/config.go

+	// PluginConfig is global plugin configuration.
+	PluginGlobalConfig pluginGlobalConfig `json:"pluginConfig, omitempty"`
+	// Source is the source name of the custom plugin monitor
+	Source string `json:"source"`


Probably, just have Plugin name, and use Plugin name as the source?

This is mainly used to let users define the user specified event source. This should be retained, IMO.

Random-Liu · 2017-11-16T07:38:42Z

pkg/custompluginmonitor/types/config.go

+
+	timeout, err := time.ParseDuration(*cpc.PluginGlobalConfig.TimeoutString)
+	if err != nil {
+		return fmt.Errorf("Error in parsing global timeout %q. %v", *cpc.PluginGlobalConfig.TimeoutString, err)


s/Error/error
s/./:

Random-Liu · 2017-11-16T07:39:29Z

pkg/custompluginmonitor/types/config.go

+
+	invoke_interval, err := time.ParseDuration(*cpc.PluginGlobalConfig.InvokeIntervalString)
+	if err != nil {
+		return fmt.Errorf("Error in parsing invoke interval %q. %v", *cpc.PluginGlobalConfig.InvokeIntervalString, err)


Random-Liu · 2017-11-16T07:41:38Z

pkg/custompluginmonitor/types/config.go

+
+	for _, rule := range cpc.Rules {
+		if _, err := os.Stat(rule.Path); os.IsNotExist(err) {
+			return fmt.Errorf("Rule path %q does not exist. Rule: %+v", rule.Path, rule)


Random-Liu · 2017-11-16T07:42:09Z

pkg/custompluginmonitor/types/config.go

+	}
+
+	for _, rule := range cpc.Rules {
+		if _, err := os.Stat(rule.Path); os.IsNotExist(err) {


We should return any error, right?

Actually we have returned an error to represent the file not exist error. I am probably not understanding this comment quite well. :(

I mean we should return any error returned by Stat, right?

Random-Liu · 2017-11-16T07:44:47Z

pkg/custompluginmonitor/types/config_test.go

+	ruleTimeout := 1 * time.Second
+	ruleTimeoutString := ruleTimeout.String()
+
+	utMetas := []struct {


Use a map, add description for each test case https://github.com/kubernetes-incubator/cri-containerd/blob/master/pkg/server/helpers_test.go#L29.

Or else it's very hard to figure out what each case is testing in the future. And please do this for the other unit tests you added.

Random-Liu · 2017-11-16T07:49:19Z

pkg/types/types.go

@@ -76,3 +76,55 @@ type Status struct {
 	// newest node conditions in this field.
 	Conditions []Condition `json:"conditions"`
 }
+
+// Type is the type of the problem.
+type Type string


I prefer keeping these types in each monitor. :)
At top level, we only care about the status above. Rules are monitor specific.

Random-Liu · 2017-11-16T07:50:19Z

pkg/types/types.go

+
+// Monitor monitors log and custom plugins and reports node problem condition and event according to
+// the rules.
+type Monitor interface {


We should keep this here. I agree.

Random-Liu · 2017-11-16T07:50:53Z

LGTM overall. Please take a look at the comments @andyxning

andyxning · 2017-11-19T14:33:27Z

@Random-Liu Comments addressed. PTAL.

Random-Liu · 2017-11-21T19:33:43Z

pkg/custompluginmonitor/custom_plugin_monitor.go

+					condition.Transition = timestamp
+					condition.Message = result.Message
+				}
+				condition.Status = true


condition.Status = status

Sorry for the logic error. :(

Random-Liu · 2017-11-21T20:02:03Z

pkg/types/types.go

@@ -76,3 +76,55 @@ type Status struct {
 	// newest node conditions in this field.
 	Conditions []Condition `json:"conditions"`
 }
+


Random-Liu · 2017-11-21T21:37:44Z

Please address the last comment. I'll send a PR based on yours.

andyxning · 2017-11-22T02:14:44Z

@Random-Liu Comment addressed. PTAL.

Random-Liu · 2017-11-22T18:53:03Z

LGTM

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 18, 2017

andyxning force-pushed the add_custom_problem_detector_plugin branch 16 times, most recently from 606d921 to b6e8d02 Compare October 19, 2017 07:57

andyxning changed the title ~~[WIP] add custom problem detector plugin~~ add custom problem detector plugin Oct 19, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2017

andyxning force-pushed the add_custom_problem_detector_plugin branch 4 times, most recently from 885e141 to 7b24581 Compare October 19, 2017 10:14

andyxning force-pushed the add_custom_problem_detector_plugin branch 2 times, most recently from 63c0f89 to 1c8fd67 Compare October 29, 2017 07:06

Random-Liu self-assigned this Nov 14, 2017

Random-Liu reviewed Nov 15, 2017

View reviewed changes

andyxning commented Nov 15, 2017

View reviewed changes

andyxning force-pushed the add_custom_problem_detector_plugin branch 5 times, most recently from f532d5c to 2b3e108 Compare November 15, 2017 15:30

Random-Liu reviewed Nov 16, 2017

View reviewed changes

andyxning force-pushed the add_custom_problem_detector_plugin branch from 2b3e108 to 28f8a8f Compare November 19, 2017 12:57

andyxning mentioned this pull request Nov 19, 2017

Tracking issue for adding custom plugins #150

Closed

4 tasks

andyxning force-pushed the add_custom_problem_detector_plugin branch 3 times, most recently from 8995b78 to 51b0a4e Compare November 19, 2017 14:26

Random-Liu reviewed Nov 21, 2017

View reviewed changes

Random-Liu mentioned this pull request Nov 21, 2017

Improve cpm #151

Merged

add custom problem detector plugin

10dbfef

andyxning force-pushed the add_custom_problem_detector_plugin branch from 51b0a4e to 10dbfef Compare November 22, 2017 02:14

Random-Liu merged commit ec470b6 into kubernetes:master Nov 22, 2017


		import (
		"testing"

add custom problem detector plugin #145

add custom problem detector plugin #145

Conversation

andyxning commented Oct 18, 2017 • edited Loading

andyxning commented Oct 24, 2017

andyxning commented Oct 27, 2017

Random-Liu commented Nov 14, 2017

andyxning commented Nov 15, 2017

Random-Liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyxning Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Nov 16, 2017

andyxning commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Nov 21, 2017

andyxning commented Nov 22, 2017

andyxning commented Oct 18, 2017 •

edited

Loading

Random-Liu commented Nov 16, 2017 •

edited

Loading

andyxning Nov 19, 2017 •

edited

Loading

Random-Liu Nov 16, 2017 •

edited

Loading