v0.11 breaks Start() -> Reconcile() context propagation

[nicks]

In controller-runtime v0.10, manager.Start(ctx) would propagate that context to all reconcilers. e.g., if you attached a trace context or logger to the context, all reconcilers would be receive that in their context.

https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.0/pkg/manager#Manager
https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.0/pkg/reconcile#Reconciler

In controller-runtime v0.11, the controller manager creates its own "empty" background context

controller-runtime/pkg/manager/runnable_group.go

Line 110 in f236f03

r.ctx, r.cancel = context.WithCancel(context.Background())

And this is the context passed to the Reconcile(ctx, req) method

Was this an intentional change? are there ways we can add context values in a way that will appear in all reconcilers?

[nicks]

One way to fix this would be to have manager Options accept a parent context, if you'd be willing to accept a PR for it.

[FillZpp]

@nicks Yeah, the reason for this is very clear: we have to control the stop order for different kinds of runnables.

When the ctx from manager.Start(ctx) is closed, the manager will execute engageStopProcedure in defer func, which stops runnables by the following order: Others runnables, LeaderElection runnables, Caches, Webhooks. The former runnables might have dependence on the latter ones. So they should not be created from the same parent context, which will lead them all being closed at once.

[nicks]

🤔 ya, that's tricky. i wish context had a way to disconnect the cancellation, but keep all the attached values

[bhcleek]

I have a context type that facilitates detaching from the parent for cancellation purposes. Would you like a contribution and potentially leveraging it to pass to the runnables?

[tzneal]

Fixed with #1846 ? As soon as it's released anyway.

[hairyhenderson]

Looks like this was included in https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.12.0 - can this issue be closed?

[nicks]

yes! 🥳