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

Simplify the cache #817

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Simplify the cache #817

merged 4 commits into from
Dec 5, 2024

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Oct 17, 2024

Based on the cache implementation improvement/simplification plan discussed
before, this change refactors it along the same lines with additional
improvements. See
https://gist.github.com/darkowlzz/34bc8b37d016b8aeb1de6660173652eb for the
initial improvement idea. It also updates oci/auth package which used the cache,
to remove the auth token caching. Auth token caching will be done at a higher
level by an OCI client.

At the time of writing the initial improvement plan, I was focused on the cache
interface and the exposed API. But while implementing the changes, I realized
how tightly the cache and their metrics were coupled, which may have contributed
to the verbose API. I'll explain some of it below for the record along with some
other explanations related to the changes.

dependency on kubernetes

The initial implementation was based on client-go's cache.Store interface and
was implemented around kubernetes style objects with object metadata. Due to
this, the implementation used kubernetes related go packages. Any user of the
cache, for example the git package for caching the git auth tokens, when
importing the cache, also imports all the kubernetes dependencies. This is
undesirable to keep simple packages independent of kubernetes dependencies. The
simplification changes the design around objects and makes it based on generic
value.

verbose interface

The initial Store interface, which the cache implementations implement, had too
many behaviors defined. It seemed to leak the implementation detail on the
interface. A cache interface needs ways to perform Get, Set and Delete. All the
other operations like resize, listing the keys, etc can be done per
implementation or build on top of the simple base interface if needed. It's
possible that we may have use case for some of the extra behaviors in the
future, but at present, we don't seem to have them. To simplify the interface,
the Store interface now has Get(), Set() and Delete() which work on string key
and generic type value.

complexity due to StoreObject

Because the initial implementation was based on kubernetes like objects,
any/most operations require building a StoreObject which is passed to the
cache for setting Get/Set/Delete. This made the cache usage code complex for
simple use. Most of the cache use cases in flux will store string or byte slice.
There's no known use case for caching kubernetes style object. Which makes the
need to create StoreObject for every use unnecessary. The new implementation
explicitly requires a string key and generic type value.

cache metrics

The initial cache was implemented to be self-instrumented. For all the
operations, it records metrics on its own. But cache event metrics which
recorded cache hit or miss were defined to be associated with object being
reconciled, similar to the metrics in the runtime package. It is desirable to
provide metrics for cache usage by flux object reconciliation. Since the initial
implementation required a StoreObject for every operation, it extracted the
associated reconciling object details from the StoreObject being passed. This
creates a strong coupling between the cache and the metrics, which is
reflected in the API. It's a nice property but at the cost of complexity on the
API. In addition, the implementation wasn't consistent in regards to the
associated object information on the metric. Some operations like GetByKey()
didn't consider the need for the reconciled object.
The new implementation decouple the cache and the metrics, making the metrics
that require the associated reconciling object to be explicitly called by the
caller who may have the information about the reconciling object.

The new implementation also introduced methods to delete the metrics when the
associated objects are deleted, like we do for any metric that's associated with
flux objects.

cache metrics prefix

The cache Options now support setting a metrics prefix to be added to all the
metrics registered by a cache. This is to prevent metrics name conflict when
using multiple instances of the cache in a program with the same metrics
registerer. Refer fluxcd/source-controller#1644 for more details.

remove cache from OCI auth

Initially, the cache was used in OCI auth package to cache the obtained auth
token. But later on, it was decided to not cache at the login manager level and
let the caller of such login functions to perform the cache as they have more
context about the scenario. #776 introduced
the cache in oci/auth package along with expiry time from the auth providers.
The cache is now removed and to return the expiry time to the caller, a new
function LoginWithExpiry() is introduced. A auth token caching client can call
LoginWithExpiry() to obtain the TTL of the obtained token and use it in the
cache.

Following is a list of overall changes in brief:

  • Remove dependency on kubernetes.
  • Simplify the Store interface to only have Get, Set and Delete.
  • Add new RecordCacheEvent() method in the cache implementations for recording the cache events, hit or miss, along with labels of the associated flux object being reconciled. The labels name, namespace and kind for this metric are not required to be configured when creating the cache. They are predefined in the created metrics recorder, similar to the runtime metrics recorder for the rest of flux. RecordCacheEvent() has to be called by the caller of cache operations explicitly along with the label information of the associated object being reconciled. The cache no longer has the knowledge of the object being reconciled, decoupled.
  • With the decoupled cache and reconciling object for metrics, the storeOptions no longer need to have the generic type defined. Simplifying the usage.
  • Add new DeleteCacheEvent() method in the cache implementations for deleting the cache events, hit or miss, which are associated with the object being reconciled. When the reconciling object is deleted, these metrics associated with the object must also be deleted.
  • Updated all the tests to use simple string cache.
  • Get() now returns a pointer instead of a separate exists boolean. If the pointer is nil, the item is not found in the cache.
  • Get() takes a key and returns a cached item for it. GetByKey() is removed as Get() does the same now.
  • In order to prevent the cache metrics to conflict when multiple caches are used against a common metrics registerer, the metrics constructor now accepts a prefix. This can be used to add a prefix to all the registered metrics of a cache.
  • Revert the cache introduced in oci/auth in [OCI] Cache Login credentials #776, keeping the expiration time which can be used by a cache at a higher level. Introduce LoginWithExpiry() to return the auth expiry time, keeping the existing Login() signature as it is. Auth token caching for OCI will be implemented separately in the OCI client.
  • Add doc.go for the package to show its usage.

OCI integration test run with the above changes: https://github.com/fluxcd/pkg/actions/runs/11743637711

@darkowlzz darkowlzz force-pushed the refactor-cache branch 3 times, most recently from 543974d to 02d44f7 Compare November 7, 2024 16:54
@darkowlzz darkowlzz marked this pull request as ready for review November 7, 2024 20:06
@darkowlzz darkowlzz requested review from stefanprodan and a team as code owners November 7, 2024 20:06
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @darkowlzz 🥇

cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
@dipti-pai
Copy link
Member

dipti-pai commented Nov 26, 2024

Reviewed the changes and tested the following scenarios by using the cache to store credentials used for github app authentication -

  1. Cache miss - Credentials are not present in the cache.
  2. Set credentials into the cache after successful authentication.
  3. Cache hit - Credentials are present in the cache and retrieved successfully.

Copy link
Member

@dipti-pai dipti-pai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks.

@stefanprodan
Copy link
Member

@darkowlzz please rebase

- Remove dependency on kubernetes.
- Simplify the Store interface to only have Get, Set and Delete.
- Add new RecordCacheEvent() method in the cache implementations for
  recording the cache events, hit or miss, along with labels of the
  associated flux object being reconciled. The labels name, namespace
  and kind for this metric are not required to be configured when
  creating the cache. They are predefined in the created metrics
  recorder, similar to the runtime metrics recorder for the rest of
  flux. RecordCacheEvent() has to be called by the caller of cache
  operations explicitly along with the label information of the
  associated object being reconciled. The cache no longer has the
  knowledge of the object being reconciled, decoupled.
- With the decoupled cache and reconciling object for metrics, the
  storeOptions no longer need to have the generic type defined.
  Simplifying the usage.
- Add new DeleteCacheEvent() method in the cache implementations for
  deleting the cache events, hit or miss, which are associated with the
  object being reconciled. When the reconciling object is deleted, these
  metrics associated with the object must also be deleted.
- Updated all the tests to use simple string cache.
- Get() now returns a pointer instead of a separate exists boolean. If
  the pointer is nil, the item is not found in the cache.
- Get() takes a key and returns a cached item for it. GetByKey() is
  removed as Get() does the same.

Signed-off-by: Sunny <github@darkowlzz.space>
Custom prefix allows using multiple instances of the cache in an
application with unique metrics.

Signed-off-by: Sunny <github@darkowlzz.space>
OCI auth token caching will be done in the OCI client. Remove the cache
from auth package and its use in the integration tests.

Since the cache related changes introduced the auth token expiry time to
be returned from the auth provider logins, a new function
LoginWithExpiry() is introduced which returns the expiry time. The
original Login() remains as it is for backwards compatibility. A higher
level client can use LoginWithExpiry() to obtain the TTL of the auth
token to use with the cache.

Signed-off-by: Sunny <github@darkowlzz.space>
Previously, when an item is not found in the cache, no error was
returned and instead the returned value was expected to be checked for
nil to determine if the object was found or not.

Considering that other cache functions like GetExpiration() and
SetExpiration() return ErrNotFound for items not in cache, it's more
consistent to return ErrNotFound for Get() as well.

Returning ErrNotFound from Get() makes it clear that the object was not
found, instead of needing to check the obtained value. There is no more
need to return a pointer to the item. Return the item value instead.
This changes the Store interface method signature for Get().

Signed-off-by: Sunny <github@darkowlzz.space>
@darkowlzz darkowlzz merged commit b83bd25 into main Dec 5, 2024
12 checks passed
@darkowlzz darkowlzz deleted the refactor-cache branch December 5, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants