switch MCP watching to full EventHandler implementation #269
Conversation
controllers/openshift_controller.go
Outdated
| for _, condType := range []mcfgv1.MachineConfigPoolConditionType{"Updating", "Updated"} { | ||
| condOld := mcfgv1.GetMachineConfigPoolCondition(statusOld, condType) | ||
| condNew := mcfgv1.GetMachineConfigPoolCondition(statusNew, condType) | ||
| condStatusOld := "<missing>" |
There was a problem hiding this comment.
Is <missing> status string coming from the mcp code ?
There was a problem hiding this comment.
Nope, it's just a string I picked because a) it's not a valid condition value and b) looks good (=is easy to understand) in the log.
It's meant to mean that the condition (Updating or Updated) is literally missing from the MCP's condition array - which it legally can.
There was a problem hiding this comment.
Probably better to add the above as a comment in the code to help us when we look at the code after few months :-)
There was a problem hiding this comment.
I'm not against but I can't think of a wording that said anything that wasn't in the code already. I mean, the whole function is called logMcpChange() so it seems expected that it just composes log messages...
There was a problem hiding this comment.
@pmores I should have worded it better. My suggestion was specific to the <missing> status string.
Maybe you can declare a constant like const missingStatusString .. and use it. When we look back at this code after a few months, we'll know what <missing> is for and not get confused about why we are using <missing> and not an empty string.
Btw just to be clear I'm fine with the code as-is. It was just a suggestion based on past experiences.
There was a problem hiding this comment.
Oh I see, I'm sorry, I misunderstood. Will do!
There was a problem hiding this comment.
Fixed by the last force push.
pmores
force-pushed
the
improve-MCP-watching
branch
2 times, most recently
from
3e3853e
to
9e02f32
Compare
|
sorry about the force push noise, the code to be reviewed isn't changing though |
The main motivation behind this is to give ourselves much more transparency
and control over when precisely we reconcile on an MCP change.
The EnqueueRequestsFromMapFunc() based mechanism used so far invokes the
registered function (mapKataConfigToRequests()) on any and all MCP changes.
Combined with the mapKataConfigToRequests() implementation that filed a
reconcile.Request any time it was invoked without discrimination we were
triggering unnecessarily many reconciliations.
With this change, we only reconcile on relevant MCP changes ("worker" and
"kata-oc") unlike so far when we reconciled on any MCP change, however
unrelated to this controller the changed MCP might have been. In addition,
we only reconcile on MCP update since reconciling on MCP creation or
deletion doesn't seem useful to this controller. We also guard against
spurious MCP changes that are irrelevant to us (e.g. timestamps) by making
sure that we only reconcile when values actually used by the controller
change (machine counts and Updating/Updated conditions).
Signed-off-by: Pavel Mores <pmores@redhat.com>
pmores
force-pushed
the
improve-MCP-watching
branch
from
9e02f32
to
ab11dce
Compare
|
New changes are detected. LGTM label has been removed. |
|
@jensfr Oh I took a lot of effort to ensure that wouldn't be the case. First of all, we never log anything about MCPs other than "kata-oc" and "worker". For those, we get a single line for each of MCP creation, deletion and generic event (I've never seen one of those yet). These are useful as they create a context to make it easier to understand the rest of the logging. So any substantial logging happens only on "worker"/"kata-oc" update. However even update logs only on changes that are relevant to the OSC controller (basically changes in MachineCounts and Updating/Updated conditions). (That's essentially the only purpose of factoring logging out of To give you an idea how it looks in practice here's an example log. It captures what happens when a worker is labelled to match The first 5 lines show us that the labelled worker was drained from "worker" immediately and ensure us that "worker" was left in a good shape. The next 7 lines tell us that the worker joined "kata-oc" as it should ( The final 7 lines (three and a half minutes later) tell us that the new worker became I found this logging extremely useful for understanding what's going on. |
The main motivation behind this is to give ourselves much more transparency and control over when precisely we reconcile on an MCP change.
The EnqueueRequestsFromMapFunc() based mechanism used so far invokes the registered function (mapKataConfigToRequests()) on any and all MCP changes. Combined with the mapKataConfigToRequests() implementation that filed a reconcile.Request any time it was invoked without discrimination we were triggering unnecessarily many reconciliations.
With this change, we only reconcile on relevant MCP changes ("worker" and "kata-oc") unlike so far when we reconciled on any MCP change, however unrelated to this controller the changed MCP might have been. In addition, we only reconcile on MCP update since reconciling on MCP creation or deletion doesn't seem useful to this controller. We also guard against spurious MCP changes that are irrelevant to us (e.g. timestamps) by making sure that we only reconcile when values actually used by the controller change (machine counts and Updating/Updated conditions).
- Description of the problem which is fixed/What is the use case
- What I did
- How to verify it
- Description for the changelog