pkg/daemon: stash the node object

[runcom]

- What I did

The node object is around most of the time (handleNodeUpdated for instance). When it's not a call is sufficient to get it and pass it around This reduces the call frequency to the api server.

This PR has been extracted from #460 and can go in w/o that. It allows us, working on PRs similar to #460 to have less places which can return a transient error from the apiserver and act accordingly.

- How to verify it

e2e should cover most of this I guess

- Description for the changelog

[runcom]

@jlebon ptal

[runcom]

I guess a possible next step after this is to start using the daemon's nodeLister + node.DeepCopy() + nodeClient.Update(n) within GetNode and updateNodeRetry as well, @cgwalters is that what you meant in #409 (comment) ?

[runcom]

/retest

[cgwalters]

I guess a possible next step after this is to start using the daemon's nodeLister + node.DeepCopy() + nodeClient.Update(n) within GetNode and updateNodeRetry as well, @cgwalters is that what you meant in #409 (comment) ?

Well I think rather than Update we can use e.g. strategicpatch.CreateTwoWayMergePatch just like the controllers do right?

But yeah, the main thing is to use the informer's state.

In practice...the daemon is mostly "wait for event, then launch" - so really the simplest may be to just get the node once and stash it in the daemon. Something like:

diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go
index 6a64507..c590374 100644
--- a/pkg/daemon/daemon.go
+++ b/pkg/daemon/daemon.go
@@ -710,6 +710,9 @@ func (dn *Daemon) handleNodeUpdate(old, cur interface{}) {
 
 	// First check if the node that was updated is this daemon's node
 	if node.Name == dn.name {
+
+		dn.node = node
+
 		// Pass to the shared update prep method
 		needUpdate, err := dn.prepUpdateFromCluster()
 		if err != nil {

And then use dn.node everywhere else.

[runcom]

Well I think rather than Update we can use e.g. strategicpatch.CreateTwoWayMergePatch just like the controllers do right?

yes noticed controllers are doing that as well

so really the simplest may be to just get the node once and stash it in the daemon

and yes, this PR is a step behind that as a first pass to start having node objects around instead of getting it every time (especially every time we read annotations). Let me know if you want your changes in this PR or I will follow up to reduce code review

[runcom]

or I can add a commit on top of this, let me know.

[cgwalters]

this PR is a step behind that as a first pass to start having node objects around.

Feels less invasive to the code though to stash it in the Daemon struct though right? All the code in the daemon is always going to operate on exactly one node.

[runcom]

Feels less invasive to the code though to stash it in the Daemon struct though right? All the code in the daemon is always going to operate on exactly one node.

sure, I'll go that way then, though...handleNodeUpdated can be called concurrently? If so, I'm not sure we can have that object stashed. Unless there's a queue (which I'm still learning )

[runcom]

I can see:

	// AddEventHandler adds an event handler to the shared informer using the shared informer's resync
	// period.  Events to a single handler are delivered sequentially, but there is no coordination
	// between different handlers.
	AddEventHandler(handler ResourceEventHandler)

So I guess it's cool to stash it.

[runcom]

But yeah, the main thing is to use the informer's state.

there's the case in CheckStateOnBoot where we don't have the informers at all so code must be refactored more to differentiate both situations.

I guess we can go ahead with the stashing in this PR and follow up to use informers. This PR still reduces the call frequency to the api server (except for updates still like setting node annotations)

[cgwalters]

there's the case in CheckStateOnBoot where we don't have the informers at all so code must be refactored more to differentiate both situations.

Mmm...I think we can just flip the ordering there and do CheckStateOnBoot after the informer cache is available.

Actually, we can/should probably change this down the line to be a periodic job. See also this comment

Now, the "check the on disk state for drift from expected config"
is factored out into a new function. We do this every time on
MCD start, although now we could e.g. change things to do it at
most once a day, writing a "last checked" timestamp in /run or as a node annotation or
whatever.

[runcom]

Mmm...I think we can just flip the ordering there and do CheckStateOnBoot after the informer cache is available.

sounds fine, I'll do it.

Actually, we can/should probably change this down the line to be a periodic job. See also this comment

agree with that (as a follow up)

[runcom]

Mmm...I think we can just flip the ordering there and do CheckStateOnBoot after the informer cache is available.

not really, the informer cache is synced only after dn.Run or maybe we can move CheckStateOnBoot within Run as a first thing to do (if that's what you really meant). if the cache sync fail also, we need to still use the client to update annotations

	if !cache.WaitForCacheSync(stopCh, dn.nodeListerSynced) {
		return dn.nodeWriter.SetUpdateDegradedMsgIgnoreErr("failed to sync cache", dn.kubeClient.CoreV1().Nodes(), dn.name)
	}

[cgwalters]

or maybe we can move CheckStateOnBoot within Run as a first thing to do (if that's what you really meant).

Right, conceptually it's part of the daemon run I think.

if the cache sync fail also, we need to still use the client to update annotations

I think Derek cited that specific bit of code as something we shouldn't be doing; if talking to Kube fails we should retry, not go degraded.

[ashcrow]

level=info msg="Creating cluster..."
level=info msg="Waiting up to 30m0s for the Kubernetes API..."
level=fatal msg="waiting for Kubernetes API: context deadline exceeded"

and

rpc error: code = 2 desc = oci runtime error: exec failed: container "2686d319707...8e25307157c" does not exist
[...]
level=fatal msg="waiting for Kubernetes API: context deadline exceeded"
2019/02/20 15:17:52 Container setup in pod e2e-aws failed, exit code 1, reason Error

[runcom]

figured out what was going on...

/hold cancel

[runcom]

aws toomanybuckets limit?

/retest

[runcom]

/retest

[runcom]

back working and green, and avoid many calls, @cgwalters @kikisdeliveryservice @LorbusChris ptal (tomorrow)

[LorbusChris]

just some nits

[LorbusChris]

nit: the daemon

[LorbusChris]

nit: calls

[LorbusChris]

the NodeBack in the name is due to the func returning the node, right? Wondering whether we could name that differently...although that'd really just be personal preference, not really necessary. Maybe add a tiny comment?

[runcom]

I suck at naming, a lot, so if you have any ideas I can change it lol

[kikisdeliveryservice]

revertNodeAnnotations? resetNodeAnnotations? fallBackNodeAnnotations? <-- i dont even know if those are better or worse tho.. 😆

[runcom]

Taken care of comments, ptal

[kikisdeliveryservice]

level=error msg="\t* aws_route_table_association.route_net.4: timeout while waiting for state to become 'success' (timeout: 5m0s)" <- rate limiting i think.

/test e2e-aws-op

[runcom]

/retest

[runcom]

Passing now

[cgwalters]

Are we targeting this for 4.0 under the "improves upgrades" banner?

[runcom]

Are we targeting this for 4.0 under the "improves upgrades" banner?

This surely has the benefit to have less code paths that can led to an apiserver timeout and error. So I'd say it goes with "improves upgrades" as well where it's more likely the apiserver can stay offline for a while and we need to not error out.

The other side of this PR which I'll work on is substituting the kube client with the informer and afterwards we need to add a retry logic for transient failiures .

How does that sound?

[cgwalters]

Today the code in writer.go updates node annotations. If we cache the node object permanently, then the daemon won't see its own annotation updates.

Today...this probably doesn't matter much but I think we should e.g. have setNodeAnnotations also return the modified node object and update the daemon's cache.

[runcom]

Today...this probably doesn't matter much but I think we should e.g. have setNodeAnnotations also return the modified node object and update the daemon's cache.

I've already account for that and setNodeAnnotations now returns the updated node object https://github.com/openshift/machine-config-operator/pull/464/files#diff-534838ddb77c16bbccf153bf8916b114R169

Going forward, that becomes a daemon method, and automatically re-save the daemon instance on success.

[cgwalters]

OK...I can't find a codepath offhand in the daemon which wants to read back any of the annotations right now.

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom

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:

• OWNERS [cgwalters,runcom]

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

[runcom]

OK...I can't find a codepath offhand in the daemon which wants to read back any of the annotations right now.

I guess we'll find out 😄 but I've spent quite some time checking that and couldn't find one either (100% possible I missed it if there's some)

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.