indexer: align affectedManifests call with matcher definitions by crozzy · Pull Request #1514 · quay/claircore

crozzy · 2025-04-09T17:42:48Z

This change attempts to make the matching process consistent across both the vulnerability matching and the manifest matching that is performed by the affectedManifests logic. It reduces the number of queries to 1 per matcher per vulnerability.

matchconstraint.go

datastore/postgres/migrations/indexer/09-versionrange.sql

pkg/omnimatcher/omnimatcher.go

datastore/postgres/affectedmanifest.go

datastore/postgres/affectedmanifest_test.go

indexer/store.go

crozzy · 2025-04-17T17:34:44Z

Execution Time not representative as it's from a small instance, but the output is still useful

Nested Loop  (cost=9.03..29.56 rows=1 width=528) (actual time=0.034..0.048 rows=4 loops=1)
  ->  Nested Loop Left Join  (cost=8.88..29.34 rows=1 width=504) (actual time=0.030..0.041 rows=4 loops=1)
        ->  Nested Loop  (cost=8.73..29.13 rows=1 width=256) (actual time=0.027..0.035 rows=4 loops=1)
              ->  Nested Loop  (cost=8.59..28.44 rows=3 width=136) (actual time=0.022..0.027 rows=6 loops=1)
                    ->  Bitmap Heap Scan on package  (cost=4.30..9.77 rows=2 width=120) (actual time=0.014..0.015 rows=2 loops=1)
                          Recheck Cond: ((name = 'pcre2'::text) AND (module = ''::text))
                          Heap Blocks: exact=1
                          ->  Bitmap Index Scan on package_unique_idx  (cost=0.00..4.29 rows=2 width=0) (actual time=0.011..0.012 rows=2 loops=1)
                                Index Cond: ((name = 'pcre2'::text) AND (module = ''::text))
                    ->  Bitmap Heap Scan on manifest_index  (cost=4.29..9.32 rows=2 width=32) (actual time=0.003..0.004 rows=3 loops=2)
                          Recheck Cond: (package_id = package.id)
                          Heap Blocks: exact=2
                          ->  Bitmap Index Scan on manifest_index_unique  (cost=0.00..4.29 rows=2 width=0) (actual time=0.001..0.001 rows=3 loops=2)
                                Index Cond: (package_id = package.id)
              ->  Index Scan using repo_pkey on repo  (cost=0.15..0.21 rows=1 width=136) (actual time=0.001..0.001 rows=1 loops=6)
                    Index Cond: (id = manifest_index.repo_id)
                    Filter: (key = 'rhel-cpe-repository'::text)
        ->  Index Scan using dist_pkey on dist  (cost=0.15..0.21 rows=1 width=264) (actual time=0.001..0.001 rows=1 loops=4)
              Index Cond: (id = manifest_index.dist_id)
  ->  Index Scan using manifest_pkey on manifest  (cost=0.15..0.22 rows=1 width=40) (actual time=0.001..0.001 rows=1 loops=4)
        Index Cond: (id = manifest_index.manifest_id)
Planning Time: 0.401 ms
Execution Time: 0.101 ms

hdonnay · 2025-04-25T18:10:39Z

What's the thinking on splitting the tests across two different packages?

crozzy · 2025-04-25T18:23:53Z

What's the thinking on splitting the tests across two different packages?

The idea is that any tests that call "meta" code should be at the root level /test/ directory. These are the kind of tests (that have been know as e2e tests) which aren't directly running the code in their packages, but indirectly running it by using parent functions (i.e. calling github.com/quay/claircore/internal/matcher.Match() instead of for example using the debian matcher directly). This was already done for the rhcc package so this commit was following that pattern.

datastore/postgres/migrations/indexer/09-versionrange.sql

test/rhel/rhcc_matcher_integration_test.go

test/mock/datastore/mocks.go

crozzy · 2026-03-20T16:19:41Z

test/mock/indexer/mocks.go


 // AffectedManifests mocks base method.
-func (m *MockStore) AffectedManifests(arg0 context.Context, arg1 claircore.Vulnerability, arg2 claircore.CheckVulnernableFunc) ([]claircore.Digest, error) {
+func (m *MockStore) AffectedManifests(ctx context.Context, v claircore.Vulnerability) ([]claircore.Digest, error) {


This signature has also changed

Which IIRC is why the mocks were recreated

hdonnay

LGTM

One comment about the metrics, just to make sure it's on purpose.

datastore/postgres/affectedmanifest.go

This change attempts to make the matching process consistent across both the vulnerability matching and the manifest matching that is performed by the affectedManifests logic. It reduces the number of queries to 1 per matcher per vulnerability. Note: the metric indexer.affectedmanifests_duration_seconds is now called datastore.getaffectedmanifests_duration_seconds and indexer.affectedmanifests_total is now called datastore.getaffectedmanifests_total. indexer.protorecord_total and indexer.protorecord_duration_seconds are removed. Signed-off-by: crozzy <joseph.crosland@gmail.com>

Update mocks to align with AffectedManifest() changes. This is also using a newer version of mockgen. Signed-off-by: crozzy <joseph.crosland@gmail.com>

Tests that do integration like behaviour should be in the root /test directory. Signed-off-by: crozzy <joseph.crosland@gmail.com>

Incidental import formats. Signed-off-by: crozzy <joseph.crosland@gmail.com>

crozzy · 2026-03-20T20:02:33Z

/fast-forward

crozzy added the not-review-ready This PR is currently in a state of disarray label Apr 9, 2025

crozzy force-pushed the affected-manifests-rewrite branch 2 times, most recently from 5a04fb9 to 8e13b8b Compare April 9, 2025 17:51