[WIP][PoC] cacher: Attempt serving paginated LIST calls from watchCache

[MadhavJivrajani]

What type of PR is this?

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?

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

/sig api-machinery scalability
/assign @wojtek-t
/cc @nikhita
Related to #108003

[k8s-ci-robot]

@MadhavJivrajani: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

[k8s-ci-robot]

@MadhavJivrajani: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

[MadhavJivrajani]

/skip

[MadhavJivrajani]

@wojtek-t for now only the btree backed cache is implemented for storeElements, we'll work on making the store btree backed in watchCache next (without inplemting indexes for the initial PoC as discussed) followed by the actual pagination logic.
I opened a draft PR mainly to try and get early feedback on the current btree approach.

[wojtek-t]

/retest

[fedebongio]

/triage accepted

[wojtek-t]

In general you would need to reimplement something similar to https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go#L135-L323

However - our case is somewhat special, as there is exactly one index defined and only for pods.
So we can probably shortcut it and:

• implement just a single index using a btree [it will be btree with key="index value" and value="set of keys matching that value"]
• use that here (we can have a strict validation that will fail creation if there is more than 1 indexer)
  [we can eventually generalize it, but it's not needed now]

[wojtek-t]

I actually thought about that and in the KEP I proposed to ignored indexes in general:
kubernetes/enhancements#3274

PTAL

[MadhavJivrajani]

Ack
Thanks @wojtek-t! Will get back to this post release :)

[wojtek-t]

As I described here:
https://github.com/kubernetes/enhancements/pull/3274/files#diff-3d93eb20b3400b8a937e7ab25e0aca359f73d8600271605df260b3b83bad06a7R301-R303

let make that very simple:
(1) if we're at "now" - let's have the index implemented the same way as we have now:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go#L70
[so basically something like:

store.lock()
current := store.btree.Clone()
indexed := store.index[myvalue] (probably deep-copied, depending on the implementation)
store.unlock()

// procee the request using "current" and "indexed"

(2) let's ignore that for continuation
[index is just an optimization, and for continuation, we can just go ahead and process all items

[MadhavJivrajani]

@wojtek-t 👋🏼

I am having some difficulty with the implementation:

• Do we replicate (more or less) the limit/continue implementation that exists here:
  

  kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

  Line 563 in f02682c

  func (s *store) GetList(ctx context.Context, key string, opts storage.ListOptions, listObj runtime.Object) error {

• After trying to implement it, I ended up with this function to do a read from the btree while honouring the limit:

func (t *btreeStore) LimitPrefixRead(limit int64, key string) []interface{} {
	t.lock.RLock()
	defer t.lock.Unlock()

	var result []interface{}
	var elementsRetrieved int64
	t.tree.AscendGreaterOrEqual(&storeElement{Key: key}, func(i btree.Item) bool {
		elementKey := i.(*storeElement).Key
		if elementsRetrieved == limit {
			return false
		}
		if !strings.HasPrefix(elementKey, key) {
			return false
		}
		elementsRetrieved++
		result = append(result, i.(interface{}))
		return true
	})

	return result
}

and I call it as

treeClone := cloneOrGetFromContinuationCache()
treeClone.LimitPrefixRead(listOpts.Predicate.Limit, somehowDecodedKeyFromContinueToken)

• Do we need to handle continue tokens timing out?
• Would we need to use Versioner here?
  

  kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

  Line 793 in f02682c

  return s.versioner.UpdateList(listObj, uint64(returnedRV), next, remainingItemCount)

Done so far:

• Implemented cache.Store using btree
  • More specifically:

type btreeIndexer interface {
	cache.Store
	Clone() btreeIndexer
	LimitPrefixRead(limit int64, key string) []interface{}
}

• Removed indexer dependent logic
• Cache for caching btree roots for serving continuation requests
• Change condition for delegating list to storage
  • Update condition in list work estimation (TODO)

Will push the progress once things are more stitched together, the implementation as of now is sort of disparate.
I am a little fuzzy on the above-mentioned aspects, it'd be great if you could provide a few pointers regarding the same! Thanks!

[wojtek-t]

Do we replicate (more or less) the limit/continue implementation that exists here

More-or-less but we should be much simpler:
(a) the logic there needs to handle multiple requests to etcd - we don't need that - we just iterate over the tree until we get enough items (or process the whole tree)
(b) we shouldn't handle the ResourceVersionExactMatch logic - let's just delegate them to etcd directly
[maybe more]

After trying to implement it, I ended up with this function to do a read from the btree while honouring the limit:

Yeah - that sounds roughly what I expected

Do we need to handle continue tokens timing out?

I thought I described it here, but apparently not.
The way I think about it is the following:

• decode continue (which inside has an RV)
• if we have RV in memory - handle that request
• if we don't have it anymore, let's delegate down to etcd

Would we need to use Versioner here?

I don't fully understand the question - can you clarify?

[aramase]

Please feel free to tag /sig auth for any additional reviews.

/remove-sig auth

[k8s-ci-robot]

@MadhavJivrajani: PR needs rebase.

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.

[linxiulei]

It needs a lock to avoid concurrenct map read/write, which crashes the kube-apiserver

[lavalamp]

I am thinking, rather than fix the watch cache like this, what if we focused on making clients use watch w/ catch up event as in kubernetes/enhancements#3667?

[dims]

If you still need this PR then please rebase, if not, please close the PR

[dims]

This PR has the label work-in-progress, please revisit to see if you still need this, please close if not

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this 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 PRs.

This bot triages 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 PR is closed

You can:

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

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

/lifecycle rotten

[MadhavJivrajani]

/remove-lifecycle rotten

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

[dims]

This work-in-progress PR needs a rebase. please rebase if this PR is still needed for the upcoming release.

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

• Reopen this PR with /reopen
• Mark this 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: Closed this PR.

In response to this:

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

This bot triages 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 PR is closed

You can:

• Reopen this PR with /reopen
• Mark this 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.