settings: Move cache implementation go-micro/store #6264
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
rhafer
changed the title
Share the same instance between the HTTP and the GRPC service. This is in preparation for moving the cache of the metadata storage backend to a go-micro/store based implementation. By sharing the same service instance in the HTTP and GRPC services we can avoid the usage of global variables for the caches, which will make the move to the go-micro/store implementation simpler.
The previous implementation was using an unlimited TTL which would cause problems in scale out deployments where multiple instances of the settings service are running. Fixes: owncloud#5067
This includes these two fixes: micro/plugins#109 micro/plugins#110
|
Kudos, SonarCloud Quality Gate passed! |
|
@micbar This would be interesting for a current project, that you also know. Because if a user would be unassigned from the "local admin" role, without this fix, the user would still stay a local admin until we restart the settings services. From what @rhafer told me, this needs no deployment changes. |
| type Cache struct { | ||
| Store string `yaml:"store" env:"OCIS_CACHE_STORE;SETTINGS_CACHE_STORE" desc:"The type of the cache store. Supported values are: 'memory', 'ocmem', 'etcd', 'redis', 'redis-sentinel', 'nats-js', 'noop'. See the text description for details."` | ||
| Nodes []string `yaml:"addresses" env:"OCIS_CACHE_STORE_NODES;SETTINGS_CACHE_STORE_NODES" desc:"A comma separated list of nodes to access the configured store. This has no effect when 'memory' or 'ocmem' stores are configured. Note that the behaviour how nodes are used is dependent on the library of the configured store."` | ||
| Database string `yaml:"database" env:"OCIS_CACHE_DATABASE" desc:"The database name the configured store should use."` |
There was a problem hiding this comment.
| Database string `yaml:"database" env:"OCIS_CACHE_DATABASE" desc:"The database name the configured store should use."` | |
| Database string `yaml:"database" env:"SETTINGS_CACHE_DATABASE" desc:"The database name the configured store should use."` |
afaik there is no global var for that
There was a problem hiding this comment.
@kobergj I was also wondering. But frontend, gateway, proxy, storage-system and storage-users all use OCIS_CACHE_DATABASE (without any local override).
There was a problem hiding this comment.
Funny. userlog, eventhistory, graph and postprocessing all use local Envvars without global override 😄
There was a problem hiding this comment.
Hm, those are STORE_DATABASE settings as oposed to CACHE_DATABASE. Well except GRAPH_CACHE_STORE_DATABASE, which .... 🤔 🤯
There was a problem hiding this comment.
I think the env vars try to distinguish between stores (used for persisting data, currently eventhistory and userlog iirc) and caches (used for volatile caches just used to improve performance). Caches also use stores to store their data, though, which is why we have these CACHE_STORE and CACHE_STORE_NODES vars.
Where it gets inconsisten is with the CACHE_STORE_DATABASE and GRAPH_CACHE_STORE_DATABASE, they should be CACHE_DATABASE and GRAPH_CACHE_DATABASE to be consistent with the other caches imo :|
That being said the whole database configuration is pretty annoying anyway because not all stores will consider it which can potentially lead to clashes between services with shared stores 🙈
There was a problem hiding this comment.
Fine for me to keep as is for now
This moves the settings cache to use a go-micro/store based caching implementation, similar to what we're using in many other services meanwhile.
This also switches to a different fork of go-micro/v4/store/redis as we need this fix: micro/plugins#110 for making the cacheinvalidation (https://github.com/owncloud/ocis/pull/6264/files#diff-9426c2b35f285118038505973e5852f93b8a31c9240420ebfd2057d3a3aaa9d9R162) work.