daemon: use informers to check for updates

[yuqi-zhang]

Change existing watch workflow to an informer instead, as watches
are prone to breaking and restarting the MCD. The Run() function
within MCD is changed from an infinite loop to instead block,
and updates are handled by callbacks via the informer.

[yuqi-zhang]

Marking WIP as the newest version isn't fully tested pending other issues.

cc/ @jlebon @sdemos @ashcrow

[ashcrow]

Makes sense so far. I recommend rebasing though as I think there has been a commit or two in the areas you're updating.

[abhinavdahiya]

you are using an informer that reacts to changes from all the nodes. Please only watch changes to the node the daemon was scheduled.

[sdemos]

@abhinavdahiya do you have a good example of an informer that only watches one instance of a resource? I wasn't sure if you could do that, right now the update callback checks if it's the node it cares about and if it's not it returns immediately, which I thought would be enough.

[yuqi-zhang]

Rebased.
@abhinavdahiya that's a good point, but I'm not sure what the proper method to do so would be. Am I able to just set an informer on a singular node instead of KubeInformerFactory.Core().V1().Nodes()? Right now the handler immediately checks for the node, so there shouldn't be a significant performance penalty.

[ashcrow]

/retest

[yuqi-zhang]

Added some fixes and removing WIP:

• instead of panic, instead reboot the node after setting degraded. This makes it consistent with other failures (the node will sleep post reboot)
• add a check to continue sleeping upon updates while we're degraded to preserve previous run logs (with actual failures)
• stop checking for node updates and deletions for the informer (causes race conditions when update has not fully applied, and we handle those cases elsewhere regardless)

It is possible to filter for nodes to watch, albeit roundabout and adds complexity. I feel that the check in handleUpdate is working, and doesn't cause performance issues to warrant a custom informer. Note that there is the option to use UntilWithSync: https://github.com/kubernetes/client-go/blob/master/tools/watch/until.go#L98 sometime in the future (basically a custom informer that mimics our old workflow without causing issues), but that is with v9.0.0 (we use 8.0.0: https://github.com/openshift/machine-config-operator/blob/master/Gopkg.toml#L60), so I personally think the fix here should work for now.

I've tested with updates to configs, as well as leaving it running for 6+ hours, haven't broken so far or hot looped. I have the latest image built at docker.io/jerzhang/origin-machine-config-daemon:v3.11.0-178-g936c032f if you want to try for yourself.

[yuqi-zhang]

@ashcrow @sdemos PTAL

[ashcrow]

No update needed, but I'm curious: Was there a specific reason to do the informer setup after the Daemon creation? Or is it more so it reads better to you this way rather than:

return  &Daemon{
        ....
        nodeLister:       nodeInformer.Lister(),
        nodeListerSynced: nodeInformer.Informer().HasSynced,
}, nil

[yuqi-zhang]

I noticed that elsewhere we split off the Lister, etc. (e.g. https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller.go#L105). I can change it to your suggested method if you'd like.

[ashcrow]

No this is fine. I was just curious 😄

[ashcrow]

I think a comment here would be helpful. Seeing a closing of the channel without going through the informer docs seems odd.

[ashcrow]

One question, and one nit. Neither are blockers.

[ashcrow]

Will let @sdemos and/or @jlebon and/or @kikisdeliveryservice review and pull the merge lever if they also 👍.

[jlebon]

Minor: indentation looks off here.

[yuqi-zhang]

will fix

[jlebon]

Why change the run() and process() split here? For one thing it makes the error handling a bit less painful in all the calls (i.e. we're just marking degraded on error in one spot instead of on all the out paths).

[yuqi-zhang]

My initial thought was that process() was more of a loop processing commands, whereas now it just blocks at the <-stop, and callbacks happen instead. This is also somewhat consistent with how we have informers elsewhere in this repo. I have no strong feelings either way so I can change that if you feel that makes more sense,

[ashcrow]

IMHO I think it's ok that Run and process are not split in this PR wince the work in #126 was going to have to change process and Run significantly anyway.

[jlebon]

Do I understand correctly that there's nothing preventing this to run at the same time as the initial triggerUpdate()? Or do callbacks not actually occur until after cache.WaitForCacheSync()

[yuqi-zhang]

Good point, I haven't seen this cause an issue/race condition (even when there are repeated failed updates so it should have triggered), but let me double check.

[jlebon]

I think I'd rather we stop watching and exit on error conditions in this callback function and instead only have the "sleep on Degraded" logic at the beginning of the daemon.

[yuqi-zhang]

Sure, should I maybe instead not start the informer if we are degraded? So we never even get to this place?

[jlebon]

Definitely! (I thought that was already the case -- when are callbacks actually active? Is it at nodeInformer.Informer().AddEventHandler() time? In that case, yeah we should move that to after checking if the node is even degraded.)

But also in this function if the node is degraded, let's just stop watching for updates and exit?

[yuqi-zhang]

Added fixup to move the state checking before the informer starts, which degrades early (and doesn't start informer anymore) as well as prevent race conditions between multiple updates. Will squash if it makes sense. Tested.

@jlebon PTAL

[sdemos]

changes all make sense to me, and the logic looks good. leaving it to @jlebon to confirm the changes he requested.

[jlebon]

Minor: at this point, we might as well move these to runStartCmd before we call CheckStateOnBoot?

[yuqi-zhang]

Good point, will fix

[jlebon]

IIUC, there's a race condition here if the node annotation is modified after CheckStateOnBoot(), but before cache is synced and we can start receiving callbacks, right?

Here's an idea: we spawn another goroutine that just listens for notifications on a channel before doing triggerUpdate(). Then in handleUpdate we send a notification. But crucially, we also send a notification after WaitForCacheSync() is done if annotations don't match. That way updates are still always serialized, and we close the race condition.

[yuqi-zhang]

Yes, I thought about this as well, but while testing this it seems that the notifications should still be there for when the informer actually starts listening (Maybe I'm mis-understanding, I used to have a addNode callback as well which triggers despite having happened before the informer starts listening)

Also, during testing, I found that the callback happens quite frequently with the node (once every couple seconds), so updates are generally caught eventually anyways (since we only care about differences in CC vs DC anyways).

[yuqi-zhang]

@jlebon pushed a minor fixup

As discussed, it is probably better to handle via. a new goroutine to ensure no updates are lost, but for the time being this is unlikely to cause any race conditions. Instead I've PR'ed a fix to the other race condition at: #145

We will likely improve this behaviour as follow-up work. If you think this makes sense, I will squash the commits.

Thanks for the reviews!

Edit: will rebase 145 as well when squashing.

[jlebon]

Yup sure, SGTM. To be fair, a race existed even before this PR (i.e. in waitUntilUpdate between comparing dc to cc and actually starting to watch for annotation updates).

[ashcrow]

/test images

[ashcrow]

/test e2e-aws

[ashcrow]

@yuqi-zhang needs a rebase.

[yuqi-zhang]

Rebased, squashed, and re-tested locally

[jlebon]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlebon, yuqi-zhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• cmd/machine-config-daemon/OWNERS [jlebon]
• pkg/daemon/OWNERS [jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment