Panic in NodeRef controller

[ncdc]

/kind bug

What steps did you take and what happened:

• Create a cluster using CAPI + CAPA in AWS
• Add a machine
• NodeRef controller panicked

What did you expect to happen:

• No panic

Anything else you would like to add:
I hit this return statement:

cluster-api/pkg/controller/noderef/noderef_controller.go

Line 169 in b37cf11

return reconcile.Result{RequeueAfter: 10 * time.Second}, nil

which returns a nil error. But it crashed at

cluster-api/pkg/controller/noderef/noderef_controller.go

Line 143 in b37cf11

klog.Infof("Set Machine's (%q in namespace %q) NodeRef to %q", machine.Name, machine.Namespace, machine.Status.NodeRef.Name)

because it didn't check to see if the reconcile needed to be requeued, and machine.status.nodeRef was nil.

Environment:

• Cluster-api version: master + v0.1.4

[ncdc]

Once fixed, we'll need to backport to release-0.1.

[pires]

Fixing the panic seems to be as easy as not swallowing the error when returning:

cluster-api/pkg/controller/noderef/noderef_controller.go

Line 169 in b37cf11

return reconcile.Result{RequeueAfter: 10 * time.Second}, nil

Now, I'm not sure this is enough because the code (or its comments) is not clear whether Reconcile is expected to be called again. Assuming it should be, we also need to set Requeue to true as it defaults to false. edited sigs.k8s.io/controller-runtime/pkg/reconcile.Result defines that setting RequeueAfter implicitly sets Requeue to true.

The end result would be:

return reconcile.Result{RequeueAfter: 10 * time.Second}, err

If this makes sense to you, I think the next step would be to figure out tests that prove the fix.

[vincepri]

@pires That seems like a good approach, we can return both

[pires]

I'll be opening a PR to address this today.

[chuckha]

I just hit this and have a patch. I do not se lifecycle active so I will take it.

/assign
/lifecycle active

[ncdc]

@chuckha check with @pires ?

[chuckha]

ah ok, actually this is a good chance for someone else to get a contribution

/remove-lifecycle active
/unassign

Sorry for the noise @pires, you should for sure work on this