controller: Fix "changed" logic to account for nodeReady changes#718
controller: Fix "changed" logic to account for nodeReady changes#718cgwalters wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
size/M
openshift-ci-robot
added
size/L
|
I'm a bit afraid of changing this code now, but looks sane to me and we need to fix #701 anyway /lgtm @kikisdeliveryservice @LorbusChris ptal as well |
openshift-ci-robot
added
lgtm
Continuing on the quest to make the controller logs more useful.
The `nodeChanged` logic was not accounting for the fact that the controller takes readiness into account too. However, rather than change that function, let's just remove it and extend our logic below that which was also effectively doing change detection so it could log it. This also removes the logging from `nodeReady()` which got very noisy in status; we now consistently log just changes in the node controller. However, the change detection logic was also implicitly ignoring nodes which didn't appear to be managed by the MCD at all - think Windows nodes. Let's explicitly skip nodes that don't have a `currentConfig` annotation.
|
OK this one is rebased 🏄♂️ on master which has #719 now. |
|
Ok this is rebased 🏄♂️ on master which as #719 now. |
kikisdeliveryservice
left a comment
kikisdeliveryservice
left a comment
There was a problem hiding this comment.
just a question and a few notes (no updates required).
| if cond.Type == corev1.NodeReady && cond.Status != corev1.ConditionTrue { | ||
| glog.Infof("Node %s is reporting NotReady", node.Name) | ||
| return false | ||
| return fmt.Errorf("Node %s is reporting NotReady", node.Name) |
There was a problem hiding this comment.
nit: these should be lowercase error messages. we can fix later.
| oldReady := isNodeReady(oldNode) | ||
| newReady := isNodeReady(curNode) | ||
| var changed bool | ||
| oldReadyErr := checkNodeReady(oldNode) |
There was a problem hiding this comment.
later: we could make this a small function instead of repeating code
|
went through this and LGTM /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashcrow, cgwalters, kikisdeliveryservice, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold for beta I guess |
|
This was merged as part of #701 |
5 participants
The
nodeChangedlogic was not accounting for the fact thatthe controller takes readiness into account too. However, rather
than change that function, let's just remove it and extend our
logic below that which was also effectively doing change detection
so it could log it.
This also removes the logging from
nodeReady()which got verynoisy in status; we now consistently log just changes in the node
controller.
However, the change detection logic was also implicitly ignoring
nodes which didn't appear to be managed by the MCD at all - think
Windows nodes. Let's explicitly skip nodes that don't have a
currentConfigannotation.