From 2f5d03280aa0739e84531311a2494ac538a6fea7 Mon Sep 17 00:00:00 2001 From: Zhen Wang Date: Tue, 30 Jul 2019 14:30:31 -0700 Subject: [PATCH 1/2] Don't update condition if status stays False/Unknown for custom plugin --- .../custom_plugin_monitor.go | 109 +++++++++--------- 1 file changed, 55 insertions(+), 54 deletions(-) diff --git a/pkg/custompluginmonitor/custom_plugin_monitor.go b/pkg/custompluginmonitor/custom_plugin_monitor.go index 636aca3a..99d750c0 100644 --- a/pkg/custompluginmonitor/custom_plugin_monitor.go +++ b/pkg/custompluginmonitor/custom_plugin_monitor.go @@ -156,77 +156,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) From 30e20c6a20c53adddc386f95057b159dcb393753 Mon Sep 17 00:00:00 2001 From: Zhen Wang Date: Tue, 30 Jul 2019 15:11:48 -0700 Subject: [PATCH 2/2] Validate that permanent problem has preset default condition --- pkg/custompluginmonitor/types/config.go | 17 +++++++ pkg/custompluginmonitor/types/config_test.go | 51 ++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/pkg/custompluginmonitor/types/config.go b/pkg/custompluginmonitor/types/config.go index de37169f..bdbbcf66 100644 --- a/pkg/custompluginmonitor/types/config.go +++ b/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 } diff --git a/pkg/custompluginmonitor/types/config_test.go b/pkg/custompluginmonitor/types/config_test.go index 1deeffd3..dae09d52 100644 --- a/pkg/custompluginmonitor/types/config_test.go +++ b/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 {