Repository: osbuild/osbuild-composer
Pull request #2918 (2022-08-26 00:00:29)

Cache the results of dnf-json dump and search commands
This is a proposed change, so I'm making it a Draft for now. Discussion will be in #2887

This caches the results of dnf-json for 60s, the cache is based on the Request made, so different distros, repos, search terms are considered different. It returns the results for 60s and then makes a new request to dnf-json, and it cleans up the map when CleanCache() is called so that memory use doesn't go crazy.

The result of this is that when using cockpit-composer to page through lists of packages, or explore package dependencies, it speeds things up 'a whole bunch'. The only possible drawback I see is that every 60s it takes a bit longer to get results.

This pull request includes:

- [X] adequate testing for the new functionality or fixed issue
- [X] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

- [X] PR contains automated tests for new functionality and

teg: Nice speedup!

I wonder if we should limit the size (maybe in number of entries?) just in case? Given the short time out, I can't see anyone creating too big caches, but just in case someone does something imaginative...

I wonder if a further improvement is possible by somehow passing in an argument to dnf-json that says "if the metadata didn't change, do nothing, we will keep using the cache even after 60s".
bcl: I'm not sure if we need to do more to keep the cache size down. I'll see if I can add some diagnostics to it and then hit it with a pile of requests to get a better handle on the memory usage. Most of it should be the full listing result. And second will be dependency lists for packages being viewed in the UI. And they are all shared among all users of the instance. Anything older than 60s and not repeated will then be deleted when CleanCache() is called after a Depsolve so I think (without good evidence :)) that it should be ok.

Most of the slowdown is in dnf-json so I'd try to avoid calling it at all. I can try some kind of 'tell me metadata changed, I have the old version' query though, just to see if it makes any significant difference.


bcl: Ok, so I did a bunch of poking around at memory usage using pprof and a debug api route to dump details and I'm pretty sure this is ready for review.

I don't think there is any need to try and limit the memory usage other than with the timeout. It looks like it is 'naturally' limiting itself with something in the dnf metadata access causing the dnf-json calls to be serial, not parallel. If you hit the API with a bunch of depsolve requests and you can see them start and wait their turn. So it can only add new data to the cache as fast as it can run these requests. I have seen as many as 80 results cached in my tests, but never more than that.

For memory usage, I saw a max of around 90M when looking at it with pprof (you can't see the memory usage for the map itself, but the functions adding to the map seem to reflect the size) and with my debug output it hit about 100M. The largest part of the usage is 55M when you do a listing of all the packages. This is on Fedora 35 so the package list is really big, 63987 of them so on CentOS and RHEL this will be quite a bit less. With the giant listing timed out it was less than 1M for most requests.


Patch:
diff --git a/internal/dnfjson/cache.go b/internal/dnfjson/cache.go
index 225e45c9d..625013f98 100644
--- a/internal/dnfjson/cache.go
+++ b/internal/dnfjson/cache.go
@@ -9,6 +9,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/osbuild/osbuild-composer/internal/rpmmd"
+
 	"github.com/gobwas/glob"
 )
 
@@ -207,3 +209,60 @@ func dirSize(path string) (uint64, error) {
 	err := filepath.Walk(path, sizer)
 	return size, err
 }
+
+// dnfResults holds the results of a dnfjson request
+// expire is the time the request was made, used to expire the entry
+type dnfResults struct {
+	expire time.Time
+	pkgs   rpmmd.PackageList
+}
+
+// dnfCache is a cache of results from dnf-json requests
+type dnfCache struct {
+	results map[string]dnfResults
+	timeout time.Duration
+	*sync.RWMutex
+}
+
+// NewDNFCache returns a pointer to an initialized dnfCache struct
+func NewDNFCache(timeout time.Duration) *dnfCache {
+	return &dnfCache{
+		results: make(map[string]dnfResults),
+		timeout: timeout,
+		RWMutex: new(sync.RWMutex),
+	}
+}
+
+// CleanCache deletes unused cache entries
+// This prevents the cache from growing for longer than the timeout interval
+func (d *dnfCache) CleanCache() {
+	d.Lock()
+	defer d.Unlock()
+
+	// Delete expired resultCache entries
+	for k := range d.results {
+		if time.Since(d.results[k].expire) > d.timeout {
+			delete(d.results, k)
+		}
+	}
+}
+
+// Get returns the package list and true if cached
+// or an empty list and false if not cached or if cache is timed out
+func (d *dnfCache) Get(hash string) (rpmmd.PackageList, bool) {
+	d.RLock()
+	defer d.RUnlock()
+
+	result, ok := d.results[hash]
+	if !ok || time.Since(result.expire) >= d.timeout {
+		return rpmmd.PackageList{}, false
+	}
+	return result.pkgs, true
+}
+
+// Store saves the package list in the cache
+func (d *dnfCache) Store(hash string, pkgs rpmmd.PackageList) {
+	d.Lock()
+	defer d.Unlock()
+	d.results[hash] = dnfResults{expire: time.Now(), pkgs: pkgs}
+}
diff --git a/internal/dnfjson/dnfjson.go b/internal/dnfjson/dnfjson.go
index c3088f32d..fb9bfedda 100644
--- a/internal/dnfjson/dnfjson.go
+++ b/internal/dnfjson/dnfjson.go
@@ -15,6 +15,7 @@ package dnfjson
 
 import (
 	"bytes"
+	"crypto/sha256"
 	"encoding/json"
 	"fmt"
 	"os"
@@ -37,6 +38,8 @@ type BaseSolver struct {
 
 	// Path to the dnf-json binary and optional args (default: "/usr/libexec/osbuild-composer/dnf-json")
 	dnfJsonCmd []string
+
+	resultCache *dnfCache
 }
 
 // Create a new unconfigured BaseSolver (without platform information). It can
@@ -44,8 +47,9 @@ type BaseSolver struct {
 // method.
 func NewBaseSolver(cacheDir string) *BaseSolver {
 	return &BaseSolver{
-		cache:      newRPMCache(cacheDir, 524288000), // 500 MiB
-		dnfJsonCmd: []string{"/usr/libexec/osbuild-composer/dnf-json"},
+		cache:       newRPMCache(cacheDir, 524288000), // 500 MiB
+		dnfJsonCmd:  []string{"/usr/libexec/osbuild-composer/dnf-json"},
+		resultCache: NewDNFCache(60 * time.Second),
 	}
 }
 
@@ -79,6 +83,7 @@ func (bs *BaseSolver) NewWithConfig(modulePlatformID string, releaseVer string,
 // the total size of the cache falls below the configured maximum size (see
 // SetMaxCacheSize()).
 func (bs *BaseSolver) CleanCache() error {
+	bs.resultCache.CleanCache()
 	return bs.cache.shrink()
 }
 
@@ -152,6 +157,11 @@ func (s *Solver) FetchMetadata(repos []rpmmd.RepoConfig) (rpmmd.PackageList, err
 	s.cache.locker.RLock()
 	defer s.cache.locker.RUnlock()
 
+	// Is this cached?
+	if pkgs, ok := s.resultCache.Get(req.Hash()); ok {
+		return pkgs, nil
+	}
+
 	result, err := run(s.dnfJsonCmd, req)
 	if err != nil {
 		return nil, err
@@ -176,6 +186,9 @@ func (s *Solver) FetchMetadata(repos []rpmmd.RepoConfig) (rpmmd.PackageList, err
 	sort.Slice(pkgs, func(i, j int) bool {
 		return sortID(pkgs[i]) < sortID(pkgs[j])
 	})
+
+	// Cache the results
+	s.resultCache.Store(req.Hash(), pkgs)
 	return pkgs, nil
 }
 
@@ -190,6 +203,11 @@ func (s *Solver) SearchMetadata(repos []rpmmd.RepoConfig, packages []string) (rp
 	s.cache.locker.RLock()
 	defer s.cache.locker.RUnlock()
 
+	// Is this cached?
+	if pkgs, ok := s.resultCache.Get(req.Hash()); ok {
+		return pkgs, nil
+	}
+
 	result, err := run(s.dnfJsonCmd, req)
 	if err != nil {
 		return nil, err
@@ -214,6 +232,9 @@ func (s *Solver) SearchMetadata(repos []rpmmd.RepoConfig, packages []string) (rp
 	sort.Slice(pkgs, func(i, j int) bool {
 		return sortID(pkgs[i]) < sortID(pkgs[j])
 	})
+
+	// Cache the results
+	s.resultCache.Store(req.Hash(), pkgs)
 	return pkgs, nil
 }
 
@@ -263,6 +284,17 @@ type repoConfig struct {
 	MetadataExpire string `json:"metadata_expire,omitempty"`
 }
 
+// Hash calculates an ID string that uniquely represents a repository
+// configuration.  The Name and ImageTypeTags fields are not considered in the
+// calculation.
+// Copied from rpmmd/repository.go
+func (r *repoConfig) Hash() string {
+	bts := func(b bool) string {
+		return fmt.Sprintf("%T", b)
+	}
+	return fmt.Sprintf("%x", sha256.Sum256([]byte(r.BaseURL+r.Metalink+r.MirrorList+r.GPGKey+bts(r.IgnoreSSL)+r.MetadataExpire)))
+}
+
 // Helper function for creating a depsolve request payload.
 // The request defines a sequence of transactions, each depsolving one of the
 // elements of `pkgSets` in the order they appear.  The `repoConfigs` are used
@@ -422,6 +454,23 @@ type Request struct {
 	Arguments arguments `json:"arguments"`
 }
 
+// Hash returns a hash of the unique aspects of the Request
+//nolint:errcheck
+func (r *Request) Hash() string {
+	h := sha256.New()
+
+	h.Write([]byte(r.Command))
+	h.Write([]byte(r.ModulePlatformID))
+	h.Write([]byte(r.Arch))
+	for _, repo := range r.Arguments.Repos {
+		h.Write([]byte(repo.Hash()))
+	}
+	h.Write([]byte(fmt.Sprintf("%T", r.Arguments.Search.Latest)))
+	h.Write([]byte(strings.Join(r.Arguments.Search.Packages, "")))
+
+	return fmt.Sprintf("%x", h.Sum(nil))
+}
+
 // arguments for a dnf-json request
 type arguments struct {
 	// Repositories to use for depsolving


Test patch:
diff --git a/internal/dnfjson/cache_test.go b/internal/dnfjson/cache_test.go
index 5dfa05455..93b1be182 100644
--- a/internal/dnfjson/cache_test.go
+++ b/internal/dnfjson/cache_test.go
@@ -8,6 +8,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/osbuild/osbuild-composer/internal/rpmmd"
+
 	"github.com/stretchr/testify/assert"
 )
 
@@ -273,3 +275,51 @@ func TestCacheCleanup(t *testing.T) {
 		})
 	}
 }
+
+// Mock package list to use in testing
+var PackageList = rpmmd.PackageList{
+	rpmmd.Package{
+		Name:        "package0",
+		Summary:     "package summary",
+		Description: "package description",
+		URL:         "https://package-url/",
+		Epoch:       0,
+		Version:     "1.0.0",
+		Release:     "3",
+		Arch:        "x86_64",
+		License:     "MIT",
+	},
+}
+
+func TestDNFCacheStoreGet(t *testing.T) {
+	cache := NewDNFCache(1 * time.Second)
+	assert.Equal(t, cache.timeout, 1*time.Second)
+	assert.NotNil(t, cache.RWMutex)
+
+	cache.Store("notreallyahash", PackageList)
+	assert.Equal(t, 1, len(cache.results))
+	pkgs, ok := cache.Get("notreallyahash")
+	assert.True(t, ok)
+	assert.Equal(t, "package0", pkgs[0].Name)
+}
+
+func TestDNFCacheTimeout(t *testing.T) {
+	cache := NewDNFCache(1 * time.Second)
+	cache.Store("notreallyahash", PackageList)
+	_, ok := cache.Get("notreallyahash")
+	assert.True(t, ok)
+	time.Sleep(2 * time.Second)
+	_, ok = cache.Get("notreallyahash")
+	assert.False(t, ok)
+}
+
+func TestDNFCacheCleanup(t *testing.T) {
+	cache := NewDNFCache(1 * time.Second)
+	cache.Store("notreallyahash", PackageList)
+	time.Sleep(2 * time.Second)
+	assert.Equal(t, 1, len(cache.results))
+	cache.CleanCache()
+	assert.Equal(t, 0, len(cache.results))
+	_, ok := cache.Get("notreallyahash")
+	assert.False(t, ok)
+}
diff --git a/internal/dnfjson/dnfjson_test.go b/internal/dnfjson/dnfjson_test.go
index 812069591..705f1c8c2 100644
--- a/internal/dnfjson/dnfjson_test.go
+++ b/internal/dnfjson/dnfjson_test.go
@@ -548,3 +548,39 @@ func TestErrorRepoInfo(t *testing.T) {
 		})
 	}
 }
+
+func TestRepoConfigHash(t *testing.T) {
+	rc := repoConfig{
+		ID:        "repoid-1",
+		Name:      "A test repository",
+		BaseURL:   "https://arepourl/",
+		IgnoreSSL: false,
+	}
+
+	hash := rc.Hash()
+	assert.Equal(t, 64, len(hash))
+
+	rc.BaseURL = "https://adifferenturl/"
+	assert.NotEqual(t, hash, rc.Hash())
+}
+
+func TestRequestHash(t *testing.T) {
+	solver := NewSolver("f36", "36", "x86_64", "/tmp/cache")
+	repos := []rpmmd.RepoConfig{
+		rpmmd.RepoConfig{
+			Name:      "A test repository",
+			BaseURL:   "https://arepourl/",
+			IgnoreSSL: false,
+		},
+	}
+
+	req, err := solver.makeDumpRequest(repos)
+	assert.Nil(t, err)
+	hash := req.Hash()
+	assert.Equal(t, 64, len(hash))
+
+	req, err = solver.makeSearchRequest(repos, []string{"package0*"})
+	assert.Nil(t, err)
+	assert.Equal(t, 64, len(req.Hash()))
+	assert.NotEqual(t, hash, req.Hash())
+}