📖 Add more docs to SyncPeriod

[guettli]

SyncPeriod is likely to get ignored if you use a predicate.

This PR updates the docs to make this more obvious.

Quoting Stefan Büringer

The behavior described is expected and not a bug.
Every SyncPeriod the underlying informers will emit events to trigger reconciliation of all objects. But if there are predicates configured that filter out those events, accordingly the Reconcile func won't be executed.

Related: SyncPeriod does not work if controller is created WithEventFilter(predicate.GenerationChangedPredicate{}) · Issue #2355 · kubernetes-sigs/controller-runtime

[k8s-ci-robot]

Hi @guettli. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[alvaroaleman]

thanks!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3a9ef1ea95f09638ba9f2443adec23981f485c84

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, guettli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

except when the cache was out of sync

I'm not sure that this is true. Do we have evidence that informer sometimes trigger update events with old object != new object during resync?

[guettli]

I wrote that, because I thought that this is what SyncPeriod should do: Check if the local cache is in-sync with the api-server. But the question of Stefan makes sense: Do we have evidence?

@alvaroaleman Do you know more about that?

[sbueringer]

(Simplified version) Every syncPeriod the informer takes all objects that are stored locally and sends out update events for them.

I took a look at the implementation, but for me it's not clear if oldObj/newObj are always identical or not

xref: https://github.com/kubernetes/kubernetes/blob/5cca03792748714547c39330ac9cb8cb9c7c60ae/staging/src/k8s.io/client-go/tools/cache/controller.go#L567

[guettli]

I thought that this is a sync between the api-server and the local cache :-)

[sbueringer]

Nope :) But that's a very frequent misunderstanding

[sbueringer]

Re-reading the rest of the godoc above. While it contains a lot of correct information I can see how it's misleading

[guettli]

@sbueringer then it is time for a new PR: 📖 Docs for SyncPeriod: No sync from api-server. by guettli · Pull Request #3277 · kubernetes-sigs/controller-runtime