Merge pull request #320 from wangzhen127/custom-plugin-fix

Don't update condition if status stays False/Unknown for custom plugin

pkg/custompluginmonitor/custom_plugin_monitor.go

@@ -158,77 +158,78 @@ func (c *customPluginMonitor) generateStatus(result cpmtypes.Result) *types.Stat
 			})
 		}
 	} else {
-		// For permanent error changes the condition
+		// For permanent error that changes the condition
 		for i := range c.conditions {
 			condition := &c.conditions[i]
 			if condition.Type == result.Rule.Condition {
-				status := toConditionStatus(result.ExitStatus)
-				// change 1: Condition status change from True to False/Unknown
-				if condition.Status == types.True && status != types.True {
-					condition.Transition = timestamp
-					var defaultConditionReason string
-					var defaultConditionMessage string
-					for j := range c.config.DefaultConditions {
-						defaultCondition := &c.config.DefaultConditions[j]
-						if defaultCondition.Type == result.Rule.Condition {
-							defaultConditionReason = defaultCondition.Reason
-							defaultConditionMessage = defaultCondition.Message
-							break
-						}
+				// The condition reason specified in the rule and the result message
+				// represent the problem happened. We need to know the default condition
+				// from the config, so that we can set the new condition reason/message
+				// back when such problem goes away.
+				var defaultConditionReason string
+				var defaultConditionMessage string
+				for j := range c.config.DefaultConditions {
+					defaultCondition := &c.config.DefaultConditions[j]
+					if defaultCondition.Type == result.Rule.Condition {
+						defaultConditionReason = defaultCondition.Reason
+						defaultConditionMessage = defaultCondition.Message
+						break
 					}
+				}
 
-					inactiveProblemEvents = append(inactiveProblemEvents, util.GenerateConditionChangeEvent(
-						condition.Type,
-						status,
-						defaultConditionReason,
-						timestamp,
-					))
-
-					condition.Status = status
-					condition.Message = defaultConditionMessage
-					condition.Reason = defaultConditionReason
+				needToUpdateCondition := true
+				var newReason string
+				var newMessage string
+				status := toConditionStatus(result.ExitStatus)
+				if condition.Status == types.True && status != types.True {
+					// Scenario 1: Condition status changes from True to False/Unknown
+					newReason = defaultConditionReason
+					if newMessage == "" {
+						newMessage = defaultConditionMessage
+					} else {
+						newMessage = result.Message
+					}
 				} else if condition.Status != types.True && status == types.True {
-					// change 2: Condition status change from False/Unknown to True
-					condition.Transition = timestamp
-					condition.Message = result.Message
-					activeProblemEvents = append(activeProblemEvents, util.GenerateConditionChangeEvent(
-						condition.Type,
-						status,
-						result.Rule.Reason,
-						timestamp,
-					))
-
-					condition.Status = status
-					condition.Reason = result.Rule.Reason
+					// Scenario 2: Condition status changes from False/Unknown to True
+					newReason = result.Rule.Reason
+					newMessage = result.Message
 				} else if condition.Status != status {
-					// change 3: Condition status change from False to Unknown or vice versa
-					condition.Transition = timestamp
-					condition.Message = result.Message
-					inactiveProblemEvents = append(inactiveProblemEvents, util.GenerateConditionChangeEvent(
-						condition.Type,
-						status,
-						result.Rule.Reason,
-						timestamp,
-					))
-
-					condition.Status = status
-					condition.Reason = result.Rule.Reason
-				} else if condition.Status == status &&
+					// Scenario 3: Condition status changes from False to Unknown or vice versa
+					newReason = defaultConditionReason
+					if newMessage == "" {
+						newMessage = defaultConditionMessage
+					} else {
+						newMessage = result.Message
+					}
+				} else if condition.Status == types.True && status == types.True &&
 					(condition.Reason != result.Rule.Reason ||
 						(*c.config.PluginGlobalConfig.EnableMessageChangeBasedConditionUpdate && condition.Message != result.Message)) {
-					// change 4: Condition status do not change.
+					// Scenario 4: Condition status does not change and it stays true.
 					// condition reason changes or
 					// condition message changes when message based condition update is enabled.
+					newReason = result.Rule.Reason
+					newMessage = result.Message
+				} else {
+					// Scenario 5: Condition status does not change and it stays False/Unknown.
+					// This should just be the default reason or message (as a consequence
+					// of scenario 1 and scenario 3 above).
+					needToUpdateCondition = false
+				}
+
+				if needToUpdateCondition {
 					condition.Transition = timestamp
-					condition.Reason = result.Rule.Reason
-					condition.Message = result.Message
+					condition.Status = status
+					condition.Reason = newReason
+					condition.Message = newMessage
+
 					updateEvent := util.GenerateConditionChangeEvent(
 						condition.Type,
 						status,
-						condition.Reason,
+						newReason,
 						timestamp,
 					)
-					if condition.Status == types.True {
+
+					if status == types.True {
 						activeProblemEvents = append(activeProblemEvents, updateEvent)
 					} else {
 						inactiveProblemEvents = append(inactiveProblemEvents, updateEvent)

pkg/custompluginmonitor/types/config.go

@@ -141,5 +141,22 @@ func (cpc CustomPluginConfig) Validate() error {
 		}
 	}
 
+	for _, rule := range cpc.Rules {
+		if rule.Type != types.Perm {
+			continue
+		}
+		conditionType := rule.Condition
+		defaultConditionExists := false
+		for _, cond := range cpc.DefaultConditions {
+			if conditionType == cond.Type {
+				defaultConditionExists = true
+				break
+			}
+		}
+		if !defaultConditionExists {
+			return fmt.Errorf("Permanent problem %s does not have preset default condition.", conditionType)
+		}
+	}
+
 	return nil
 }

pkg/custompluginmonitor/types/config_test.go

@@ -20,6 +20,8 @@ import (
 	"reflect"
 	"testing"
 	"time"
+
+	"k8s.io/node-problem-detector/pkg/types"
 )
 
 func TestCustomPluginConfigApplyConfiguration(t *testing.T) {
@@ -279,6 +281,55 @@ func TestCustomPluginConfigValidate(t *testing.T) {
 			},
 			IsError: true,
 		},
+		"permanent problem has preset default condition": {
+			Conf: CustomPluginConfig{
+				Plugin: customPluginName,
+				PluginGlobalConfig: pluginGlobalConfig{
+					InvokeInterval:  &defaultInvokeInterval,
+					Timeout:         &defaultGlobalTimeout,
+					MaxOutputLength: &defaultMaxOutputLength,
+					Concurrency:     &defaultConcurrency,
+				},
+				DefaultConditions: []types.Condition{
+					{
+						Type:    "TestCondition",
+						Reason:  "TestConditionOK",
+						Message: "Test condition is OK.",
+					},
+				},
+				Rules: []*CustomRule{
+					{
+						Type:      types.Perm,
+						Condition: "TestCondition",
+						Reason:    "TestConditionFail",
+						Path:      "../plugin/test-data/ok.sh",
+						Timeout:   &normalRuleTimeout,
+					},
+				},
+			},
+			IsError: false,
+		},
+		"permanent problem does not have preset default condition": {
+			Conf: CustomPluginConfig{
+				Plugin: customPluginName,
+				PluginGlobalConfig: pluginGlobalConfig{
+					InvokeInterval:  &defaultInvokeInterval,
+					Timeout:         &defaultGlobalTimeout,
+					MaxOutputLength: &defaultMaxOutputLength,
+					Concurrency:     &defaultConcurrency,
+				},
+				Rules: []*CustomRule{
+					{
+						Type:      types.Perm,
+						Condition: "TestCondition",
+						Reason:    "TestConditionFail",
+						Path:      "../plugin/test-data/ok.sh",
+						Timeout:   &normalRuleTimeout,
+					},
+				},
+			},
+			IsError: true,
+		},
 	}
 
 	for desp, utMeta := range utMetas {