Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the memcached discovery client #68865

Closed
caesarxuchao opened this issue Sep 20, 2018 · 3 comments · Fixed by #70994
Closed

Refactor the memcached discovery client #68865

caesarxuchao opened this issue Sep 20, 2018 · 3 comments · Fixed by #70994
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@caesarxuchao
Copy link
Member

When debugging #68735, I found the cached discovery clients confusing, which in turn makes the DeferredDiscoveryRESTMapper hard to understand. I'll try to summarize the confusion and propose a high-level fix.

We have two implementations of the CacheDiscoveryInterface, one mem-cached, and one disk-cached.

The behavior of the disk-cached discovery client is intuitive,

  • Upon cache misses, it contacts the server to get live results and updates the cache.
  • Its Invalidate() method invalidates local caches, future requests results in server traffic.
  • Its Fresh() method returns true unless pre-existing on-disk cache is used.

The behavior of the mem-cached one is not intuitive,

  • Its cache update logic is implemented in the Invalidate() method, and at the end, the Invalidate() function sets valid=true.
  • Upon cache misses, it returns an error without further contacting the apiserver. It returns a specific error type if the cache is completely empty. It expects the caller to interpret the error and invoke Invalidate() to populate the cache.
  • Its Fresh() method always returns true as long as Invalidate() is called once.

IIUC, mem-cached client is implemented in this way to avoid overloading the apiserver in case the caller keeps causing cache misses with invalid groupVersion. The disk-cached one is used by kubectl, which is intended for human users, so it doesn't have this concern.

I plan to refactor the mem-cached client to:

  • Invalidate() just sets the cache as "not-fresh"
  • Upon cache misses, if the cache is "not-fresh", the client contacts the server to do a discovery, and updates the cache. Otherwise, it reports "cache miss".
  • The client is created with empty cache, and the cache is initially "not-fresh".
  • Remove the specific error type for empty cache, it shouldn't happen anymore.

I will add a Refresh() method to the cached discovery client interface. The method will do the discovery, and marks the cache as fresh upon success.

The current comment on the DeferredDiscoveryRESTMapper.Reset() claims the method marks the cached restmapper as invalid, but does not do discovery. However, if it's using the current memcached client as the underlying discovery client, the claim is false. I plan to give the DeferredDiscoveryRESTMapper an Invalidate method and a Refresh method, so user can choose whether the restmapper is lazily initialized.

/sig api-machinery
/assign

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 20, 2018
@caesarxuchao caesarxuchao added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 20, 2018
@caesarxuchao
Copy link
Member Author

/cc @mbohlool

@mborsz
Copy link
Member

mborsz commented Nov 5, 2018

We have run into #68735 again in scalability test. Any update here?
/cc @mborsz

@yue9944882
Copy link
Member

this will simplify #65527 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants