Skip to content

Conversation

@joelanford
Copy link
Member

This commit improves maintainability and extensibility of the FBC caching used to serve the GRPC API.

It:

  • introduces a new cache subpackage and interface, which defines primitives for checking cache integrity, building the cache, and loading the cache.
  • moves the existing registry.Querier to an implementation of the cache interface, called cache.JSON

This refactor also resolves two outstanding issues:

  1. The current code contains a bug that causes panics when certain failures occur loading the cache.
  2. The current code is hardcoded to always rebuild the cache if it is unreadable or its digest doesn't contain the expected value.

This commit:

  1. Refactors and removes the cases the led to the panic situation
  2. Adds a new flag, --cache-enforce-integrity, that causes opm serve to exit with a failure if the cache is unreadable or its digest doesn't contain the expected value. This is useful in production contexts when one always expects the cache to be pre-populated correctly.

Closes #1049
Closes #1050

Signed-off-by: Joe Lanford joe.lanford@gmail.com

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2023
@joelanford joelanford force-pushed the serve-fail-cache-outdated branch 5 times, most recently from 4b7ab99 to 712627b Compare January 17, 2023 14:00
@operator-framework operator-framework deleted a comment from codecov bot Jan 17, 2023
@joelanford
Copy link
Member Author

Updated coverage report is here: https://app.codecov.io/github/operator-framework/operator-registry/commit/712627b1d32deeeb29773629724dbb785321d68d

Trying to track down what I did to make coverage go down... 🤔

@joelanford joelanford force-pushed the serve-fail-cache-outdated branch from 712627b to d957cbf Compare January 17, 2023 18:10
This commit improves maintainability and extensibility of the FBC
caching used to serve the GRPC API.

It:
- introduces a new cache package and interface, which defines
  primitives for checking cache integrity, building the cache, and
  loading the cache.
- moves the existing registry.Querier to an implementation of the cache
  interface, called cache.JSON

This refactor also resolves two outstanding issues:
1. The current code contains a bug that causes panics when certain
   failures occur loading the cache.
2. The current code is hardcoded to always rebuild the cache if it is
   unreadable or its digest doesn't contain the expected value.

This commit:
1. Refactors and removes the cases the led to the panic situation
2. Adds a new flag, --cache-enforce-integrity, that causes `opm serve`
   to exit with a failure if the cache is unreadable or its digest
   doesn't contain the expected value. This is useful in production
   contexts when one always expects the cache to be pre-populated
   correctly.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the serve-fail-cache-outdated branch from d957cbf to 174fad2 Compare January 17, 2023 18:11
@operator-framework operator-framework deleted a comment from codecov bot Jan 17, 2023
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #1051 (ca8b014) into master (0080ea0) will decrease coverage by 0.08%.
The diff coverage is 59.45%.

❗ Current head ca8b014 differs from pull request most recent head 174fad2. Consider uploading reports for the commit 174fad2 to get more accurate results

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   52.06%   51.98%   -0.08%     
==========================================
  Files         102      104       +2     
  Lines        9218     9216       -2     
==========================================
- Hits         4799     4791       -8     
- Misses       3505     3514       +9     
+ Partials      914      911       -3     
Impacted Files Coverage Δ
pkg/cache/tar.go 61.11% <ø> (ø)
pkg/cache/cache.go 32.35% <32.35%> (ø)
pkg/cache/json.go 51.40% <51.40%> (ø)
alpha/declcfg/load.go 76.52% <70.37%> (-2.13%) ⬇️
pkg/cache/pkgs.go 70.40% <70.40%> (ø)
pkg/sqlite/query.go 64.61% <100.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

These changes look good and seem reasonable to me. I like the approach of making the cache an interface which will make it nice if we ever feel we need to extend to support different types of caches.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, joelanford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@grokspawn
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2023
@openshift-merge-robot openshift-merge-robot merged commit 562cf9c into operator-framework:master Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RFE: enhance error handling leading to catalog cache rebuilding

4 participants