✨ Populate/update cache on ClusterCatalog reconcile #1284
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1284 +/- ##
==========================================
+ Coverage 74.67% 74.96% +0.28%
==========================================
Files 42 42
Lines 2515 2516 +1
==========================================
+ Hits 1878 1886 +8
+ Misses 451 446 -5
+ Partials 186 184 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| _, err = r.Cache.FetchCatalogContents(ctx, existingCatalog) | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
One benefit of the previous approach is that catalog pull errors will show up in ClusterExtension status.
If we move catalog cache populate to a separate controller, do users lose all visibility into the specific details of the reason the catalog caches aren't populated?
I wonder if we should store these errors somewhere that is available to the ClusterExtension reconciler so that we can still propagate them into ClusterExtension status?
There was a problem hiding this comment.
I'm considering keeping on-demand cache population. If cache fetch fails here - we will keep re-queueing until it gets populated. If it doesn't and it is something permanent - cache population will fail during ClusterExtension itself and it will be propagated to ClusterExtension status as it currently works.
It should work, but I don't like it very much because it is not straightforward to understand.
I wonder if we should store these errors somewhere that is available to the ClusterExtension reconciler so that we can still propagate them into ClusterExtension status?
Any ideas? We can probably store somewhere in filesystemCache (e.g. in cacheDataByCatalogName)?
There was a problem hiding this comment.
High level idea would be for the interface that ClusterExtension uses to remain the same (e.g. it can return an error).
Yeah cacheDataByCatalogName seems like the obvious place. We could have cacheData include an Err field?
There was a problem hiding this comment.
@joelanford could you please take a look at #1318?
I refactored the client and cache: the client is now responsible for http requests and cache is responsible for caching only (previously it was making request). And I also added error propagation.
The idea is that here instead of r.Cache.FetchCatalogContents we will be calling client's PopulateCache.
Once it is merged I'll be able to rebase this PR on top of the changes.
openshift-merge-robot
added
the
needs-rebase
m1kola
force-pushed
the
populate-cache-on-from-controller
branch
from
28b3312
to
72e1586
Compare
openshift-merge-robot
removed
the
needs-rebase
m1kola
force-pushed
the
populate-cache-on-from-controller
branch
from
72e1586
to
4b76985
Compare
m1kola
force-pushed
the
populate-cache-on-from-controller
branch
4 times, most recently
from
71c2382
to
80763b1
Compare
Currently we fetch catalog data and populate cache on demand during ClusterExtension reconciliation. This works but the first reconciliation after ClusterCatalog creation or update is slow due to the need to fetch data. With this change we proactively populate cache on ClusterCatalog creation and check if cache needs to be updated on ClusterCatalog update. Signed-off-by: Mikalai Radchuk <[email protected]>
m1kola
force-pushed
the
populate-cache-on-from-controller
branch
from
80763b1
to
482531d
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Description
Currently we fetch catalog data and populate cache on demand during
ClusterExtensionreconciliation. This works but the first reconciliation afterClusterCatalogcreation or update is slow due to the need to fetch data.With this change we proactively populate cache on
ClusterCatalogcreation and check if cache needs to be updated onClusterCatalogupdate.Reviewer Checklist