Skip to content

fix: do not run GC every update cycle #12695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RTann merged 2 commits into from
Sep 20, 2024
Merged

fix: do not run GC every update cycle #12695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ffaa26
fix: do not run GC every update cycle
RTann Sep 12, 2024
74f35f2
add GC skip log
RTann Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion scanner/matcher/matcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ func NewMatcher(ctx context.Context, cfg config.MatcherConfig) (Matcher, error)
vulnUpdater, err := vuln.New(ctx, vuln.Opts{
Store: store,
Locker: locker,
Pool: pool,
MetadataStore: metadataStore,
Client: client,
URL: cfg.VulnerabilitiesURL,
Expand Down
78 changes: 44 additions & 34 deletions scanner/matcher/updater/vuln/updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"time"

"github.com/google/uuid"
"github.com/jackc/pgx/v4/pgxpool"
"github.com/klauspost/compress/zstd"
"github.com/quay/claircore"
"github.com/quay/claircore/datastore"
Expand Down Expand Up @@ -62,12 +61,11 @@ var (

// Opts represents Updater options.
//
// Store, Locker, Pool, MetadataStore, and URL are required.
// Store, Locker, MetadataStore, and URL are required.
// The rest are optional.
type Opts struct {
Store datastore.MatcherStore
Locker *ctxlock.Locker
Pool *pgxpool.Pool
MetadataStore postgres.MatcherMetadataStore

Client *http.Client
Expand All @@ -91,7 +89,6 @@ type Updater struct {

store datastore.MatcherStore
locker updates.LockSource
pool *pgxpool.Pool
metadataStore postgres.MatcherMetadataStore

client *http.Client
Expand Down Expand Up @@ -128,7 +125,6 @@ func New(ctx context.Context, opts Opts) (*Updater, error) {

store: opts.Store,
locker: opts.Locker,
pool: opts.Pool,
metadataStore: opts.MetadataStore,

client: opts.Client,
Expand Down Expand Up @@ -186,8 +182,8 @@ func fillOpts(opts *Opts) error {
}

func validate(opts Opts) error {
if opts.Store == nil || opts.Locker == nil || opts.Pool == nil || opts.MetadataStore == nil {
return errors.New("must provide a Store, a Locker, a Pool, and a MetadataStore")
if opts.Store == nil || opts.Locker == nil || opts.MetadataStore == nil {
return errors.New("must provide a Store, a Locker, and a MetadataStore")
}
if _, err := url.Parse(opts.URL); err != nil {
return fmt.Errorf("invalid URL: %q", opts.URL)
Expand Down Expand Up @@ -353,23 +349,36 @@ func (u *Updater) Initialized(ctx context.Context) bool {
//
// Note: periodic full GC will not be started.
func (u *Updater) Update(ctx context.Context) error {
ctx = zlog.ContextWithValues(ctx, "component", "matcher/updater/vuln/Updater.Update")

var (
updated bool
err error
)
if features.ScannerV4MultiBundle.Enabled() {
if err := u.runMultiBundleUpdate(ctx); err != nil {
return err
}
updated, err = u.runMultiBundleUpdate(ctx)
} else {
if err := u.runSingleBundleUpdate(ctx); err != nil {
return err
}
updated, err = u.runSingleBundleUpdate(ctx)
}
if !u.skipGC {
if err != nil {
return err
}

// Only bother running the GC when it's not disabled
// and when the vulnerabilities have been updated.
if !u.skipGC && updated {
u.runGC(ctx)
} else if !u.skipGC {
// Only log if GC is enabled to reduce noise when GC is disabled.
zlog.Info(ctx).Msg("no vulnerability updates: skipping GC")
}

return nil
}

// runSingleBundleUpdate updates the vulnerability data with one single bundle.
func (u *Updater) runSingleBundleUpdate(ctx context.Context) error {
// runSingleBundleUpdate updates the vulnerability data with one single bundle and
// returns a bool indicating if any updates actually happened.
func (u *Updater) runSingleBundleUpdate(ctx context.Context) (bool, error) {
ctx = zlog.ContextWithValues(ctx, "component", "matcher/updater/vuln/Updater.runSingleBundleUpdate")

// Use TryLock instead of Lock to prevent simultaneous updates.
Expand All @@ -379,28 +388,28 @@ func (u *Updater) runSingleBundleUpdate(ctx context.Context) error {
zlog.Info(ctx).
Str("lock", updateName).
Msg("did not obtain lock, skipping update run")
return nil
return false, nil
}

prevTimestamp, err := u.metadataStore.GetLastVulnerabilityUpdate(ctx)
if err != nil {
zlog.Debug(ctx).
Err(err).
Msg("did not get previous vuln update timestamp")
return err
return false, err
}
zlog.Info(ctx).
Str("timestamp", prevTimestamp.Format(http.TimeFormat)).
Msg("previous vuln update")

f, timestamp, err := u.fetch(ctx, prevTimestamp)
if err != nil {
return err
return false, err
}
if f == nil {
// Nothing to update at this time.
zlog.Info(ctx).Msg("no new vulnerability update")
return nil
return false, nil
}
defer func() {
if err := f.Close(); err != nil {
Expand All @@ -413,48 +422,49 @@ func (u *Updater) runSingleBundleUpdate(ctx context.Context) error {

dec, err := zstd.NewReader(f)
if err != nil {
return fmt.Errorf("creating zstd reader: %w", err)
return false, fmt.Errorf("creating zstd reader: %w", err)
}
defer dec.Close()

if err := u.importFunc(ctx, dec); err != nil {
return err
return false, err
}

if err := u.metadataStore.SetLastVulnerabilityUpdate(ctx, postgres.SingleBundleUpdateKey, timestamp); err != nil {
return err
return false, err
}

if u.initialized.CompareAndSwap(false, true) {
zlog.Info(ctx).Msg("finished initial updater run: setting updater to initialized")
}

return nil
return true, nil
}

// runMultiBundleUpdate updates the vulnerability data with a multi-bundle.
func (u *Updater) runMultiBundleUpdate(ctx context.Context) error {
// runMultiBundleUpdate updates the vulnerability data with a multi-bundle and
// returns a bool indicating if any updates actually happened.
func (u *Updater) runMultiBundleUpdate(ctx context.Context) (bool, error) {
ctx = zlog.ContextWithValues(ctx, "component", "matcher/updater/vuln/Updater.runMultiBundleUpdate")

prevTime, err := u.metadataStore.GetLastVulnerabilityUpdate(ctx)
if err != nil {
zlog.Debug(ctx).
Err(err).
Msg("did not get previous vuln update timestamp")
return err
return false, err
}
zlog.Info(ctx).
Time("timestamp", prevTime).
Msg("previous vuln update")

zipFile, zipTime, err := u.fetch(ctx, prevTime)
if err != nil {
return err
return false, err
}
if zipFile == nil {
// Nothing to update at this time.
zlog.Info(ctx).Msg("no new vulnerability update")
return nil
return false, nil
}
defer func() {
if err := zipFile.Close(); err != nil {
Expand All @@ -467,11 +477,11 @@ func (u *Updater) runMultiBundleUpdate(ctx context.Context) error {

zipInfo, err := zipFile.Stat()
if err != nil {
return err
return false, err
}
zipReader, err := zip.NewReader(zipFile, zipInfo.Size())
if err != nil {
return err
return false, err
}

// Iterate through each vulnerability bundle in the .zip archive
Expand All @@ -483,7 +493,7 @@ func (u *Updater) runMultiBundleUpdate(ctx context.Context) error {
zlog.Info(ctx).Msg("starting bundle update")
if err := u.updateBundle(ctx, bundleF, zipTime, prevTime); err != nil {
zlog.Error(ctx).Err(err).Msg("updating bundle failed")
return fmt.Errorf("updating bundle %s: %w", bundleF.Name, err)
return false, fmt.Errorf("updating bundle %s: %w", bundleF.Name, err)
}
zlog.Info(ctx).Msg("completed bundle update")
}
Expand All @@ -492,12 +502,12 @@ func (u *Updater) runMultiBundleUpdate(ctx context.Context) error {
// Safe to be run concurrently.
err = u.metadataStore.GCVulnerabilityUpdates(ctx, names, zipTime)
if err != nil {
return fmt.Errorf("cleaning vuln updates: %w", err)
return false, fmt.Errorf("cleaning vuln updates: %w", err)
}

_ = u.Initialized(ctx)

return nil
return true, nil
}

func (u *Updater) updateBundle(ctx context.Context, zipF *zip.File, zipTime time.Time, prevTime time.Time) error {
Expand Down
51 changes: 35 additions & 16 deletions scanner/matcher/updater/vuln/updater_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"archive/zip"
"bytes"
"context"
"errors"
"fmt"
"io"
"net/http"
Expand All @@ -13,7 +14,6 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"github.com/quay/claircore"
"github.com/quay/claircore/datastore"
"github.com/quay/claircore/libvuln/driver"
Expand Down Expand Up @@ -67,23 +67,28 @@ func testHTTPServer(t *testing.T, content func(r *http.Request) io.ReadSeeker) (
func TestSingleBundleUpdate(t *testing.T) {
t.Setenv("ROX_SCANNER_V4_MULTI_BUNDLE", "false")

srv, now := testHTTPServer(t, func(_ *http.Request) io.ReadSeeker {
srv, now := testHTTPServer(t, func(r *http.Request) io.ReadSeeker {
accept := r.Header.Get("X-Scanner-V4-Accept")
if accept != "" {
t.Fatalf("X-Scanner-V4-Accept header should not be set for single-bundle")
}
return strings.NewReader("test")
})

locker := &testLocker{
locker: updates.NewLocalLockSource(),
fail: true,
}
store := mocks.NewMockMatcherStore(gomock.NewController(t))
metadataStore := mocks.NewMockMatcherMetadataStore(gomock.NewController(t))
u := &Updater{
locker: locker,
pool: nil,
store: store,
metadataStore: metadataStore,
client: srv.Client(),
url: srv.URL,
root: t.TempDir(),
skipGC: true,
skipGC: false,
importFunc: func(_ context.Context, _ io.Reader) error {
return nil
},
Expand All @@ -104,6 +109,9 @@ func TestSingleBundleUpdate(t *testing.T) {
metadataStore.EXPECT().
SetLastVulnerabilityUpdate(gomock.Any(), gomock.Eq(postgres.SingleBundleUpdateKey), now).
Return(nil)
store.EXPECT().
GC(gomock.Any(), gomock.Any()).
Return(int64(0), nil)
err = u.Update(context.Background())
assert.NoError(t, err)

Expand All @@ -118,50 +126,61 @@ func TestSingleBundleUpdate(t *testing.T) {
func TestMultiBundleUpdate(t *testing.T) {
t.Setenv("ROX_SCANNER_V4_MULTI_BUNDLE", "true")

// TODO(ROX-26236): Test with zst files, as a chunk of the updater function is currently untested.
srv, now := testHTTPServer(t, func(r *http.Request) io.ReadSeeker {
accept := r.Header.Get("X-Scanner-V4-Accept")
if accept != "application/vnd.stackrox.scanner-v4.multi-bundle+zip" {
return strings.NewReader("test")
t.Fatalf("X-Scanner-V4-Accept header should be set to application/vnd.stackrox.scanner-v4.multi-bundle+zip for multi-bundle")
}
var buf bytes.Buffer
zipWriter := zip.NewWriter(&buf)
err := zipWriter.Close()
if err != nil {
t.Fatalf("Failed to close zip writer: %v", err)
}
// Return an empty ZIP file.
return bytes.NewReader(buf.Bytes())
})

locker := &testLocker{
locker: updates.NewLocalLockSource(),
}
store := mocks.NewMockMatcherStore(gomock.NewController(t))
metadataStore := mocks.NewMockMatcherMetadataStore(gomock.NewController(t))
u := &Updater{
locker: locker,
pool: nil,
store: store,
metadataStore: metadataStore,
client: srv.Client(),
url: srv.URL,
root: t.TempDir(),
skipGC: true,
importFunc: func(_ context.Context, _ io.Reader) error {
return nil
},
retryDelay: 1 * time.Second,
retryMax: 1,
skipGC: false,
importFunc: func(_ context.Context, _ io.Reader) error { return nil },
retryDelay: 1 * time.Second,
retryMax: 1,
}

// Successful update.
// Skip update and error when unable to get previous update time.
metadataStore.EXPECT().
GetLastVulnerabilityUpdate(gomock.Any()).
Return(now.Add(-time.Minute), nil)
Return(time.Time{}, errors.New("err"))
err := u.Update(context.Background())
assert.Error(t, err)

// Successful update.
metadataStore.EXPECT().
GetLastVulnerabilityUpdate(gomock.Any()).
Return(now, nil)
Return(now.Add(-time.Minute), nil)
metadataStore.EXPECT().
GCVulnerabilityUpdates(gomock.Any(), gomock.Any(), now).
Return(nil)
err := u.Update(context.Background())
metadataStore.EXPECT().
GetLastVulnerabilityUpdate(gomock.Any()).
Return(now, nil)
store.EXPECT().
GC(gomock.Any(), gomock.Any()).
Return(int64(0), nil)
err = u.Update(context.Background())
assert.NoError(t, err)

// No update.
Expand Down
1 change: 0 additions & 1 deletion scanner/updater/import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ func Load(ctx context.Context, connString, vulnsURL string) error {
updater, err := vuln.New(ctx, vuln.Opts{
Store: store,
Locker: locker,
Pool: pool,
MetadataStore: metadataStore,
URL: vulnsURL,
SkipGC: true,
Expand Down