✨ 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 73.30% 73.45% +0.15%
==========================================
Files 42 42
Lines 3057 3063 +6
==========================================
+ Hits 2241 2250 +9
+ Misses 643 640 -3
Partials 173 173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
5 times, most recently
from
80763b1
to
482531d
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
|
||
| catalogFsys, err := r.CatalogCache.Get(existingCatalog.Name, existingCatalog.Status.ResolvedSource.Image.Ref) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("error retrieving cache for catalog %q: %v", existingCatalog.Name, err) |
There was a problem hiding this comment.
WDYT about propagating this error into the cache so that cache clients have more context about why the content is not available?
There was a problem hiding this comment.
@joelanford it is currently implemented how we discussed here: #1284 (comment)
Cache client caches content fetch errors and r.CatalogCache.Get(...) will return fetch error to the clients. So here we will have something like error retrieving cache for catalog "my-catalog": network request timeout or other error from actual network call
There was a problem hiding this comment.
Ah I see now. I didn't realize most of the plumbing had already been done! Beautiful!
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 <mradchuk@redhat.com>
m1kola
force-pushed
the
populate-cache-on-from-controller
branch
from
482531d
to
2ee4aa4
Compare
|
Updated the error wording. @joelanford please take another look. |
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