Clarify "resourceVersion unset" semantics in Watch

[embano1]

This is a Feature Request

I wasn't sure whether to file as bug or feature (more clarity), thus decided to use "feature" here. I also understand that there have been lots of discussions around watch behavior [1,2], but wanted to seek clarity from a documentation perspective.

[1] #16944
[2] kubernetes/kubernetes#74022

What would you like to be added

The current documentation on resource version for Watches in the API concepts states:

This is correct for API servers without cache. However, the default behavior in kube-apiserver is --watch-cache Default: true. From the Cacher code path for Watch() I read that no matter which RV is passed, it will always be served from the cache:

func (c *Cacher) Watch(ctx context.Context, key string, opts storage.ListOptions) (watch.Interface, error) {
	pred := opts.Predicate
	watchRV, err := c.versioner.ParseResourceVersion(opts.ResourceVersion) // returns 0 on empty string ("")
	if err != nil {
		return nil, err
	}

func (a APIObjectVersioner) ParseResourceVersion(resourceVersion string) (uint64, error) {
	if resourceVersion == "" || resourceVersion == "0" {
		return 0, nil
	}

If my understanding is correct, any client Watch() with empty RV ("") against an API server with cache enabled will not be served via quorum read. If so, the documentation should point this out, e.g. amend the following paragraph:

Start a watch at the most recent resource version, which must be consistent (i.e. served from etcd via a quorum read). [add something about clusters with API server cache enabled]

Why is this needed

A user (controller developer) might be under the false assumption that their Watches() are strongly consistent when not specifying a RV and not knowing whether the actual cluster runs with API server caching or not (e.g. managed environments, separate teams, etc.).

Comments

If my understanding is correct the time travel bug for lists could potentially exist in many custom controllers/CRDs if users rely on strongly consistent RV == "" semantics as described in the documentation.

cc/ @liggitt @jpbetz @lavalamp @wojtek-t @tnozicka @marshtompsxd @lalithsuresh

[k8s-ci-robot]

@embano1: This issue is currently awaiting triage.

SIG Docs takes a lead on issue triage for this website, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[lavalamp]

I think @jpbetz knows the current state on this, this might be a dup

[embano1]

Thx, yeah...I checked the docs (last PR from him actually) and also the issues but could not find clarification around cache/nocache Watch() behavior in the docs.

[wojtek-t]

Yes - this is a bug in documentation, which we should fix.
/assign @jpbetz
Can you please take a care of it?

That said - using watch with RV = 0 or RV unset isn't really a recommended pattern. Generally the "list + watch from there" is the pattern that we recommend everywhere.
It might make sense to also make it very explicit in this doc.

[embano1]

Generally the "list + watch from there" is the pattern that we recommend everywhere.
It might make sense to also make it very explicit in this doc.

Agree.

Follow-up question since most users will likely use ListerWatcher with SharedIndexInformer (reflector) or controller-runtime (which does this implicitly): These controllers don't enforce quorum list on start, thus are also vulnerable to time travel issues even though using list+watch, right? Reason why I am asking is that this behavior likely is not known to the majority of the community. Should we also document this then?

[jpbetz]

The documentation absolutely needs to be updated for this case. I'll try to get to that. I agree that watch with RV="" should be avoided. I'll try to incorporate that into the documentation.

ListerWatcher, SharedIndexInformer or whatever other documentation doesn't capture this also really needs to be updated. I'm less familiar with that documentation, so if anyone is willing to own getting that updated, it would be a huge help. I believe the Reflector documentation is OK since it just delegates to the ListerWatcher provided to NewReflector, but it might be good to see if it can be improved in any way as well.

[embano1]

I'm less familiar with that documentation, so if anyone is willing to own getting that updated, it would be a huge help

Happy to help, wondering whether this would only be in the client-go docs or is there any public documentation for the SDK wrt ListWatch semantics?

I believe the Reflector documentation is OK

Please help me with my understanding: as you said, reflector uses the configured ListerWatcher() typically passed via SharedIndexInformer construction. Most (all?) of the provided typed/dynamic don't explicitly specify a RV, thus using the one injected during ListAndWatch() in the reflector:

func (r *Reflector) ListAndWatch(stopCh <-chan struct{}) error {
	klog.V(3).Infof("Listing and watching %v from %s", r.expectedTypeName, r.name)
	var resourceVersion string

	options := metav1.ListOptions{ResourceVersion: r.relistResourceVersion()}

	if err := func() error {
		initTrace := trace.New("Reflector ListAndWatch", trace.Field{"name", r.name})
		defer initTrace.LogIfLong(10 * time.Second)
		var list runtime.Object
		var paginatedResult bool
		var err error
		listCh := make(chan struct{}, 1)
		panicCh := make(chan interface{}, 1)
		go func() {
			defer func() {
				if r := recover(); r != nil {
					panicCh <- r
				}
			}()
			// Attempt to gather list in chunks, if supported by listerWatcher, if not, the first
			// list request will return the full response.
			pager := pager.New(pager.SimplePageFunc(func(opts metav1.ListOptions) (runtime.Object, error) {
				return r.listerWatcher.List(opts)
			}))

IMHO options is set to "0" based on this code in r.relistResourceVersion():

if r.lastSyncResourceVersion == "" {
		// For performance reasons, initial list performed by reflector uses "0" as resource version to allow it to
		// be served from the watch cache if it is enabled.
		return "0"
	}

Thus, the reflector will List from any and use the returned RV (if valid) to start the Watcher(). Couldn't this also lead to staleness issues, e.g. when the controller starts working from events on an initial ListWatch returned at say RV10 from API server A, then crashes, gets reconnected to server B and starts the same logic from RV8 because server B is lagging?

[jpbetz]

Thus, the reflector will List from any and use the returned RV (if valid) to start the Watcher(). Couldn't this also lead to staleness issues, e.g. when the controller starts working from events on an initial ListWatch returned at say RV10 from API server A, then crashes, gets reconnected to server B and starts the same logic from RV8 because server B is lagging?

Oh right. So the background here is that we did attempt (quite a long time ago, now) to update Reflectors to do the initial list using RV="". The PR (kubernetes/kubernetes#83520) got rolled back due to due to excessive list calls to etcd (kubernetes/kubernetes#86824). I haven't gotten the bandwidth since then to try to tackle this again, but maybe it would be reasonable to provide an option when constructing reflectors to specify that initial lists should be consistent.

For documentation, you're absolutely right, we have the "reflectors can go back in time problem" and we should document that.

[embano1]

Perfect, let me know where I can help and I'll file PR(s). Just don't know all the places from a docs perspective.

[sftim]

/sig api-machinery

[jpbetz]

@embano1 Updating the client-go godocs seems appropriate to me.

@roycaihw Any thoughts on how to best document this?

[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

[dprotaso]

/remove-lifecycle stale

[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

[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

[wojtek-t]

/remove-lifecycle rotten

[sftim]

I could revise #25299 if someone suggests new wording.

[embano1]

@sftim can you please point me to the section where you would like to add the different Watch() semantics? Then I can propose a paragraph on the different behavior using cached API servers, client-go optimizations, different RV= semantics and issues this might cause (e.g. time-travel anomalies, see https://github.com/sieve-project/sieve/blob/main/docs/bugs.md and https://github.com/sieve-project/sieve/blob/main/docs/paper-hotos.pdf)

[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

[sftim]

@embano1 I'd add the extra details about watch actions into https://kubernetes.io/docs/reference/using-api/api-concepts/#efficient-detection-of-changes

[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.