fix custom plugin monitor condition change #203
Conversation
|
@andyxning: GitHub didn't allow me to request PR reviews from the following users: aga20. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
andyxning
force-pushed
the
fix_custom_plugin_monitor_condition_change
branch
2 times, most recently
from
d68b761 to
34ce300
Compare
| @@ -126,7 +126,8 @@ func (c *customPluginMonitor) generateStatus(result cpmtypes.Result) *types.Stat | |||
| condition := &c.conditions[i] | |||
| if condition.Type == result.Rule.Condition { | |||
| status := toConditionStatus(result.ExitStatus) | |||
| if condition.Status != status || condition.Reason != result.Rule.Reason { | |||
| // change 1: Condition status change from True to False/Unknown | |||
| if condition.Status != status && condition.Status == types.True { | |||
There was a problem hiding this comment.
condition.Status == types.True && status != types.True
There was a problem hiding this comment.
IIRC, true means there is a problem, false means there is no problem. If so, we should reset default here.
There was a problem hiding this comment.
@andyxning What about this? I feel like the current logic in your change is just opposite.
In your code:
status true -> false: reason is updated;
status false -> true: reason is reset to default.
Is that correct?
There was a problem hiding this comment.
What about this? I feel like the current logic in your change is just opposite.
In your code:
status true -> false: reason is updated;
status false -> true: reason is reset to default.
Is that correct?
After a re-think, i am wrong. I will update this.
|
|
||
| condition.Status = status | ||
| condition.Reason = result.Rule.Reason | ||
| } else if condition.Status != status && status == types.True { |
There was a problem hiding this comment.
condition.Status != types.True && status == types.True
| } | ||
| condition.Status = status |
There was a problem hiding this comment.
I feel like most code in the if-else clause can be extracted out.
At least event generation can be extracted.
There was a problem hiding this comment.
Yes. Some code refactor needs to be done. But i prefer we do little refactor on this to make the code more clear for others.
andyxning
force-pushed
the
fix_custom_plugin_monitor_condition_change
branch
from
34ce300 to
e3b3771
Compare
|
Seems ok with manual testing! |
|
|
||
| condition.Status = status | ||
| condition.Reason = result.Rule.Reason | ||
| } else if condition.Status == status && condition.Message != result.Message { |
There was a problem hiding this comment.
WE should only update condition when reason changes.
Message could be spammy and change a lot. See https://github.com/kubernetes/node-problem-detector/blob/master/pkg/systemlogmonitor/log_monitor.go#L142
|
I'll merged this now and we'll send a fix for #203 (comment). |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyxning, Random-Liu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fix #202
/cc @aga20