How to correctly use context arguments is unclear

[coderanger]

Most of the examples use context.TODO() on each call into the client. Would be nice to have a better explanation of what this is for.

And someday it would be cool to add a .Context field on the reconcile.Request for controllers (and maybe something similar for webhooks, though that's harder), and have the controller machinery create a context.Background() at the start of a request so you can thread it down into calls to the client library and whatnot.

[DirectXMan12]

/kind feature
/priority important-longterm

[sr]

I was wondering about this too, my understanding is the controller runtime doesn't enforce any timeout on reconciliation attempts, which could cause the entire controller to lock up:

https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/controller.go#L215

Doing something like this doesn't seem so bad:

func (r *MyReconciler) Reconcile(req reconcile.Request) (reconcile.Result, error) {
	ctx := context.WithTimeout(context.Background(), r.timeout)

	instance := &api.MyCRD{}
	_ = r.Get(ctx, request.NamespacedName, instane)
	_ = r.someAPI.Call(ctx, &RPCRequest{})

	// ...
}

Though I could also see a new field on controller.Option, with a sane default:

type Options struct {
	MaxConcurrentReconciles int
	ReconcilerTimeout time.Duration // default to a minute?
	Reconciler reconcile.Reconciler
}

Then have the controller pass in a context as part of the request:

func (c *Controller) processNextWorkItem() bool {
	if c.ReconcilerTimeout != time.Duration(0) {
		req = req.WithContext(context.WithTimeout(context.Background(), c.ReconcilerTimeout))
	}
	if result, err := c.Do.Reconcile(req); err != nil {
		// handle
	}
}

[DirectXMan12]

we can't actually enforce timeouts, but we can totally allow you to configure a timeout context builder that gets passed to all reconcile requests that you can pass to clients. It won't help if you manually block yourself, but in the case of well-behaved clients it's probably a good idea.

[DirectXMan12]

since you might want contexts for other things, a ContextBuilder option (with a sane timeout-based helper and good defaults) is probably something we'll want eventually.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[DirectXMan12]

/remove-lifecycle rotten

[seh]

I noticed today that Webhooks get invoked with context.Background. It would be nice if we could inject a context.Context value that would be used as the starting context for each request, in order to attach things like a shared tracing configuration handle.

[DirectXMan12]

/lifecycle frozen

[seh]

It would be nice if we could inject a context.Context value that would be used as the starting context for each request, in order to attach things like a shared tracing configuration handle.

Putting my money where my mouth is, see #549.

[DirectXMan12]

thanks, merged!

[vincepri]

/help
/priority backlog
/kind documentation

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
/priority backlog
/kind documentation

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen

[vincepri]

/remove-lifecycle frozen

@coderanger Seems this issue can now be closed given that most functions and reconcilers accept a context starting from v0.7.x

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[coderanger]

/close

Wow I had forgotten I wrote this :)

[k8s-ci-robot]

@coderanger: Closing this issue.

In response to this:

/close

Wow I had forgotten I wrote this :)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.