Client's auto-cache-creation a non-trivial footgun

[DirectXMan12]

Currently, with the delegating client, suppose we have the following scenario:

ctrl.NewControllerManagedBy(mgr).
  For(&corev1.ReplicaSet{}).
  Owns(&corev1.Pod{}).
  Complete(&rsController{Client: mgr.GetClient()})

// later, in the reconciler
  var sec corev1.Secret
  if err := c.Get(ctx, mySecretName, &sec); err != nil {
    return ctrl.Result{}, ctrl.IgnoreNotFound(err)
  }

Because we're using the delegating client, the client will automatically set up a cache for secrets, even though we're not explicitly watching them. This makes cache establishment pretty transparent, which can be nice, but is also fairly magic, and can be a big foot-gun. Consider:

ctrl.NewControllerManagedBy(mgr).
  For(&corev1.ReplicaSet{}).
  Owns(&corev1.Pod{}).
  Complete(&rsController{Client: mgr.GetClient()})

// later, in the reconciler
  var pod corev2.Pod
  if err := c.Get(ctx, req.NamespacedName, &pod); err != nil {
    return ctrl.Result{}, ctrl.IgnoreNotFound(err)
  }

Notice here, we're triggering based off of core/v1.Pod, but we fetch core/v2.Pod. This causes the client to set up a whole new cache (duplicating storage) that's potentially in a different state than the one we're reconciling off of. This can cause confusing behavior, like getting a reconcile for an object we haven't seen yet, a delete for an object that still seems to be in our cache, etc.

I think, before 1.0.0, we might want to reconsider this behavior, and make it optional-opt-in, with a separate mechanism in case you want to set up non-reconcile-triggering caches for whatever reason.

/kind feature
/kind bug

[DirectXMan12]

that second scenario isn't hypothetical, btw -- I helped a user debug pretty much that exact issue the other day. It was non-obvious because they had started to do an incremental migration of their code base to a new CRD version, and things seemed to roughly work fine, but they'd intermittently see issues.

[coderanger]

I think the automatic caching does more good than harm overall, a lot of folks don't even realize it's happening but it makes their code much faster. Maybe require explicit opt-in for a second cache when we already have one for that group+kind? That seems like a thing that would 99.99999% of the time be an oversight.

[DirectXMan12]

Fair! I'm curious how often it's the case that folks end up fetching stuff that's not already watched. My original thought was that in the cases about, we'd throw an error about corev2.Pod and corev1.Secret not being cached yet, and then you'd be able to explicitly say during manager setup "also set up caches for this".

[estroz]

I think the automatic caching does more good than harm overall

@coderanger I definitely see this side of the argument. In the general case, this has worked really well.

you'd be able to explicitly say during manager setup "also set up caches for this".

@DirectXMan12 this is exactly what I'd want to see if we go down the explicit route (i.e. an error during setup and not from within the reconcile loop).

I'm curious how often it's the case that folks end up fetching stuff that's not already watched.

I think the answer to this question is an important factor in this decision: if most people are already de-facto explicitly establishing watches on resources during controller setup, then making that a requirement will not be too painful. Given the nature of controllers, I would say that users are establishing caches for resources like Deployments and Pods associated with custom resources at setup; resources like ConfigMaps, maybe not.

Side note: one component that is not hidden from users is the RBAC necessary to establish a listwatch on a resource, since they must set this up themselves for a controller manager deployment. I have seen quite a few issues filed with RBAC misconfiguration being the root cause. If caching becomes opt-in, at least the required RBAC would be clear(er).

[mhrivnak]

Has there ever been discussion of adding metrics for cache size by resource? That could help someone quickly identify why their controller is using more memory than expected, and point them in the right direction for making changes to reduce the scope of what's cached. As observed, this can be especially significant for a cluster-scoped controller.

[dimitarKiryakov]

Is this resolved by #1249 ?

[alvaroaleman]

Is this resolved by #1249 ?

No, #1249 is about explicitly opting out, this issue is about not implicitly creating caches by default.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.