Manager runnableGroup causes a disconnect between contexts

[aphistic]

I was working on a Reconciler today and found that values I stored in the context passed to a Manager's Start method weren't available in the Reconcile context, so I started digging into it.

I ended up tracking it down to the runnableGroup struct, and specifically this line in the newRunnableGroup function: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/runnable_group.go#L110

It's creating an internal context for the group, but it's not being based on any other context. I looked through the code and couldn't find any code that references r.ctx or r.cancel before Start could be called, aside from the StopAndWait method where it's calling r.cancel.

If the Start method is updated to set r.ctx and r.cancel inside the r.startOnce.Do call with the context passed to Start, it seems like the context from the initial manager Start call is connected all the way to the Reconcile context. If StopAndWait is updated to check if r.cancel is nil or not before it tries calling it I think it's possible to implement it safely.

Is this intended functionality? It does seem like it works as expected the way the code is now, but is the intent to split the Manager's Start context from the ultimate Reconcile context?

I was going to submit a PR for this, but I saw you request an issue to be created first, so that's what this is!

[FillZpp]

Hi @aphistic , I used to explain why we don't create contexts of runnable based on the same parent context in #1752 (comment)

Please feel free to optimize it, but keep in mind that we should keep the stop order of different types of runnables :)

[aphistic]

Hi @aphistic , I used to explain why we don't create contexts of runnable based on the same parent context in #1752 (comment)

Ahh, I see! Sorry I missed that issue when I was searching originally. =/

Please feel free to optimize it, but keep in mind that we should keep the stop order of different types of runnables :)

What would you think about a Manager Options field called BaseContext specifically for this case, similar to http.Server's BaseContext? If the function is defined it would be used to retrieve the context used in cases like this, otherwise it would default to context.Background().

[FillZpp]

What would you think about a Manager Options field called BaseContext specifically for this case

Yeah, I'm ok with that. If the BaseContext in Options has been set, we can pass it into newRunnables to be the base context of all types of runnables. WDYT? @vincepri

[hairyhenderson]

I just ran into this - I'd love to see a BaseContext option. @aphistic is this something you're working on? Or should I look at making a PR for this?

[aphistic]

I was waiting to see if @vincepri had any thoughts on it before making a PR, but I could probably do a PR today sometime and we can talk about implementation and stuff in there!