storage/cacher: waitUntilWatchCacheFreshAndForceAllEvents checks if storage.RequestWatchProgress is supported

[p0lyn0mial]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[p0lyn0mial]

/assign @wojtek-t

[p0lyn0mial]

I will also open a separate pr for rejecting a watch-list request when the feature is not supported by the underlying store.

[MadhavJivrajani]

@p0lyn0mial trying to grasp this better, please correct me if I'm misunderstanding here and I'm pretty sure this is a pathological scenario, but as I understand it, if Supports returns true, it can also mean that another client can have it disabled. So the worst case as you mentioned here is the request would wait for 3 seconds (blockTimeout) and then fail.

However, if the condition ends up being true, we would call waitingUntilFresh.Add(), which according to:

kubernetes/staging/src/k8s.io/apiserver/pkg/storage/cacher/watch_progress.go

Lines 98 to 102 in 06c9cdc

	shouldRequest := func() bool {
	pr.mux.RLock()
	defer pr.mux.RUnlock()
	return pr.waiting > 0 && !pr.stopped
	}()

means we would request a progress notify from etcd. And this is requested every 100ms:

kubernetes/staging/src/k8s.io/apiserver/pkg/storage/cacher/watch_progress.go

Line 36 in 06c9cdc

progressRequestPeriod = 100 * time.Millisecond

So if the worst case scenario happens and we wait for 3 seconds, its also possible that we send 30 progress notify requests to etcd in that time period for every such request that comes in. Considering this is rare, I guess this isn't too much additional load, but considering this can happen for each such request, do you think its possible for this to behave in unintentional ways?

[p0lyn0mial]

The scenario you described can happen when the api-server is configured to use multiple etcd clusters (in theory, this is possible), and these etcd clusters run on different versions. I was also concerned about this design. After talking with @wojtek-t , I realised this is an extremely exotic scenario, not really seen in practice.

I could see an optimisation where the progress notifier could also read the state of the feature before sending a request to etcd.

[MadhavJivrajani]

@p0lyn0mial that makes sense. Trying something here based on your suggestion: #124928

PTAL 🙏🏼

[fedebongio]

Since Wojtek is already assigned I'm triaging this one.
/triage accepted

[wojtek-t]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 0aa381e8e5150bcefb719c215576e14aa755801f

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, wojtek-t

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:

• staging/src/k8s.io/apiserver/pkg/storage/OWNERS [wojtek-t]

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