Skip to content

Commit

Permalink
Consider already selected packages during solve
Browse files Browse the repository at this point in the history 
We ran into a very very slow solve. I can't tell if it was actually slow
or infinite, but it didn't seem to be convering very quickly. I realized
that we essentially recurse the entire graph for every constraint, even
for things we've already solved. This was surprising, because I thought
we handled that already, but it seems like we didn't.

A major concern I have for this change is that I believe it affects the
order of the packages we return when we solve, which might affect how
the actual filesystem gets put together when we install them. This might
not actually be a problem given that in other places we _sort_ the
package list when locking the apko config, which would generate a
different install order, which would already be exercising my concern.
So... if this is a problem, we haven't noticed it yet.

Signed-off-by: Jon Johnson <[email protected]>
  • Loading branch information
@jonjohnsonjr
jonjohnsonjr committed Nov 15, 2024
1 parent cecb0d6 commit f870d65
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 50 deletions.
78 changes: 70 additions & 8 deletions pkg/apk/apk/repo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ type PkgResolver struct {
indexes []NamedIndex
nameMap map[string][]*repositoryPackage
installIfMap map[string][]*repositoryPackage // contains any package that should be installed if the named package is installed

// Short-circuit providers we have already selected.
selected map[string]*RepositoryPackage
}

// Clone returns a copy of PkgResolver.
Expand All @@ -211,6 +214,7 @@ func (p *PkgResolver) Clone() *PkgResolver {
indexes: p.indexes,
nameMap: maps.Clone(p.nameMap),
installIfMap: maps.Clone(p.installIfMap),
selected: map[string]*RepositoryPackage{},
}
}

Expand All @@ -234,7 +238,8 @@ func newPkgResolver(ctx context.Context, indexes []NamedIndex) *PkgResolver {
installIfMap = map[string][]*repositoryPackage{}
)
p := &PkgResolver{
indexes: indexes,
indexes: indexes,
selected: map[string]*RepositoryPackage{},
}

// create a map of every package by name and version to its RepositoryPackage
Expand Down Expand Up @@ -383,6 +388,29 @@ func (p *PkgResolver) disqualifyConflicts(pkg *RepositoryPackage, dq map[*Reposi
}
}

func (p *PkgResolver) pick(pkg *RepositoryPackage) error {
if conflict, ok := p.selected[pkg.Name]; ok {
// Trying to re-select the same thing is fine actually.
if conflict == pkg {
return nil
}

return fmt.Errorf("selecting package %s conflicts with %s on %q", pkg.Filename(), conflict.Filename(), pkg.Name)
}

p.selected[pkg.Name] = pkg

for _, prov := range pkg.Provides {
constraint := cachedResolvePackageNameVersionPin(prov)
if conflict, ok := p.selected[constraint.name]; ok {
return fmt.Errorf("selecting package %s conflicts with %s on %q", pkg.Filename(), conflict.Filename(), constraint.name)
}
p.selected[constraint.name] = pkg
}

return nil
}

func (p *PkgResolver) disqualify(dq map[*RepositoryPackage]string, pkg *RepositoryPackage, reason string) {
dq[pkg] = reason

Expand Down Expand Up @@ -452,7 +480,7 @@ func (p *PkgResolver) constrain(constraints []string, dq map[*RepositoryPackage]
// GetPackagesWithDependencies get all of the dependencies for the given packages based on the
// indexes. Does not filter for installed already or not.
func (p *PkgResolver) GetPackagesWithDependencies(ctx context.Context, packages []string, allArchs map[string][]NamedIndex) (toInstall []*RepositoryPackage, conflicts []string, err error) {
_, span := otel.Tracer("go-apk").Start(ctx, "GetPackageWithDependencies")
_, span := otel.Tracer("go-apk").Start(ctx, "GetPackagesWithDependencies")
defer span.End()

// Tracks all the packages we have disqualified and the reason we disqualified them.
Expand Down Expand Up @@ -494,10 +522,11 @@ func (p *PkgResolver) GetPackagesWithDependencies(ctx context.Context, packages

// now get the dependencies for each package
for _, pkgName := range packages {
pkg, deps, confs, err := p.GetPackageWithDependencies(pkgName, dependenciesMap, dq)
pkg, deps, confs, err := p.GetPackageWithDependencies(ctx, pkgName, dependenciesMap, dq)
if err != nil {
return toInstall, nil, &ConstraintError{pkgName, err}
}

for _, dep := range deps {
if _, ok := installTracked[dep.Name]; !ok {
toInstall = append(toInstall, dep)
Expand Down Expand Up @@ -527,7 +556,7 @@ func (p *PkgResolver) GetPackagesWithDependencies(ctx context.Context, packages
// Requires the existing set because the logic for resolving dependencies between competing
// options may depend on whether or not one already is installed.
// Must not modify the existing map directly.
func (p *PkgResolver) GetPackageWithDependencies(pkgName string, existing map[string]*RepositoryPackage, dq map[*RepositoryPackage]string) (*RepositoryPackage, []*RepositoryPackage, []string, error) {
func (p *PkgResolver) GetPackageWithDependencies(ctx context.Context, pkgName string, existing map[string]*RepositoryPackage, dq map[*RepositoryPackage]string) (*RepositoryPackage, []*RepositoryPackage, []string, error) {
parents := make(map[string]bool)
localExisting := make(map[string]*RepositoryPackage, len(existing))
existingOrigins := map[string]bool{}
Expand All @@ -544,10 +573,11 @@ func (p *PkgResolver) GetPackageWithDependencies(pkgName string, existing map[st
}

pin := cachedResolvePackageNameVersionPin(pkgName).pin
deps, conflicts, err := p.getPackageDependencies(pkg, pin, true, parents, localExisting, existingOrigins, dq)
deps, conflicts, err := p.getPackageDependencies(ctx, pkg, pin, parents, localExisting, existingOrigins, dq)
if err != nil {
return nil, nil, nil, err
}

// eliminate duplication in dependencies
added := make(map[string]*RepositoryPackage, len(deps))
dependencies := make([]*RepositoryPackage, 0, len(deps))
Expand Down Expand Up @@ -672,7 +702,10 @@ func (p *PkgResolver) resolvePackage(pkgName string, dq map[*RepositoryPackage]s
// It might change the order of install.
// In other words, this _should_ be a DAG (acyclical), but because the packages
// are just listing dependencies in text, it might be cyclical. We need to be careful of that.
func (p *PkgResolver) getPackageDependencies(pkg *RepositoryPackage, allowPin string, allowSelfFulfill bool, parents map[string]bool, existing map[string]*RepositoryPackage, existingOrigins map[string]bool, dq map[*RepositoryPackage]string) (dependencies []*RepositoryPackage, conflicts []string, err error) {
func (p *PkgResolver) getPackageDependencies(ctx context.Context, pkg *RepositoryPackage, allowPin string, parents map[string]bool, existing map[string]*RepositoryPackage, existingOrigins map[string]bool, dq map[*RepositoryPackage]string) (dependencies []*RepositoryPackage, conflicts []string, err error) {
if err := ctx.Err(); err != nil {
return nil, nil, context.Cause(ctx)
}
// check if the package we are checking is one of our parents, avoid cyclical graphs
if _, ok := parents[pkg.Name]; ok {
return nil, nil, nil
Expand Down Expand Up @@ -715,7 +748,7 @@ func (p *PkgResolver) getPackageDependencies(pkg *RepositoryPackage, allowPin st
continue
}

if allowSelfFulfill && pkg.Name == name {
if pkg.Name == name {
var (
actualVersion, requiredVersion Version
err1, err2 error
Expand All @@ -733,6 +766,30 @@ func (p *PkgResolver) getPackageDependencies(pkg *RepositoryPackage, allowPin st
}
}

if picked, ok := p.selected[name]; ok {
if version == "" {
// If we don't care which version, and we've already selected something, fantastic.
continue
}

actualVersion, err := cachedParseVersion(picked.Version)
if err != nil {
return nil, nil, err
}
requiredVersion, err := cachedParseVersion(version)
if err != nil {
return nil, nil, err
}

// We do care which version and they match.
if compare.satisfies(actualVersion, requiredVersion) {
continue
}

// We already selected something to satisfy "name" and it does not match the "version" we need now.
return nil, nil, fmt.Errorf("we already selected %q=%q which conflicts with %q=%q", picked.Name, picked.Version, name, version)
}

// first see if it is a name of a package
depPkgWithVersions, ok := p.nameMap[name]
if !ok {
Expand Down Expand Up @@ -793,7 +850,12 @@ func (p *PkgResolver) getPackageDependencies(pkg *RepositoryPackage, allowPin st
childParents[k] = true
}
childParents[pkg.Name] = true
subDeps, confs, err := p.getPackageDependencies(depPkg, allowPin, true, childParents, existing, existingOrigins, dq)

if err := p.pick(pkg); err != nil {
return nil, nil, err
}

subDeps, confs, err := p.getPackageDependencies(ctx, depPkg, allowPin, childParents, existing, existingOrigins, dq)
if err != nil {
return nil, nil, &DepError{pkg, err}
}
Expand Down
56 changes: 14 additions & 42 deletions pkg/apk/apk/repo_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
a.SetClient(&http.Client{
Transport: &testLocalTransport{root: testPrimaryPkgDir, basenameOnly: true},
})
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
})
Expand All @@ -136,7 +136,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
a.SetClient(&http.Client{
Transport: &testLocalTransport{root: testRSA256IndexPkgDir, basenameOnly: true},
})
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
})
Expand All @@ -148,7 +148,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
a.SetClient(&http.Client{
Transport: &testLocalTransport{fail: true},
})
_, err := a.GetRepositoryIndexes(context.TODO(), false)
_, err := a.GetRepositoryIndexes(context.Background(), false)
require.Error(t, err, "should fail when no cache and no network")
})
t.Run("we can fetch, but do not cache indices without etag", func(t *testing.T) {
Expand All @@ -160,7 +160,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
a.SetClient(&http.Client{
Transport: &testLocalTransport{root: testPrimaryPkgDir, basenameOnly: true},
})
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")

Expand Down Expand Up @@ -188,7 +188,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
},
},
})
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
// check that the contents are the same
Expand Down Expand Up @@ -226,7 +226,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
Transport: &testLocalTransport{root: testPrimaryPkgDir, basenameOnly: true, headers: map[string][]string{http.CanonicalHeaderKey("etag"): {testEtag}}},
})
// Use the client to fill the cache.
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
// Capture the initial index.
Expand All @@ -237,7 +237,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
a.SetClient(&http.Client{
Transport: &testLocalTransport{root: testAlternatePkgDir, basenameOnly: true, headers: map[string][]string{http.CanonicalHeaderKey("etag"): {testEtag}}},
})
indexes, err = a.GetRepositoryIndexes(context.TODO(), false)
indexes, err = a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
// Capture the resulting index.
Expand All @@ -258,7 +258,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
Transport: &testLocalTransport{root: testPrimaryPkgDir, basenameOnly: true, headers: map[string][]string{http.CanonicalHeaderKey("etag"): {testEtag}}},
})
// Use the client to fill the cache.
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
// Capture the initial index.
Expand All @@ -271,7 +271,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
Transport: &testLocalTransport{root: testAlternatePkgDir, basenameOnly: true, headers: map[string][]string{http.CanonicalHeaderKey("etag"): {testEtag + "change"}}},
})

indexes, err = a.GetRepositoryIndexes(context.TODO(), false)
indexes, err = a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
// Capture the resulting index.
Expand All @@ -296,7 +296,7 @@ func TestGetRepositoryIndexes(t *testing.T) {
headers: map[string][]string{http.CanonicalHeaderKey("etag"): {fmt.Sprint(i)}},
},
})
indexes, err := a.GetRepositoryIndexes(context.TODO(), false)
indexes, err := a.GetRepositoryIndexes(context.Background(), false)
require.NoErrorf(t, err, "unable to get indexes")
require.Greater(t, len(indexes), 0, "no indexes found")
return nil
Expand Down Expand Up @@ -593,7 +593,7 @@ func TestGetPackageDependencies(t *testing.T) {
_, index := testGetPackagesAndIndex()

resolver := NewPkgResolver(context.Background(), testNamedRepositoryFromIndexes(index))
_, pkgs, _, err := resolver.GetPackageWithDependencies("package1", nil, map[*RepositoryPackage]string{})
_, pkgs, _, err := resolver.GetPackageWithDependencies(context.Background(), "package1", nil, map[*RepositoryPackage]string{})
require.NoErrorf(t, err, "unable to get dependencies")

actual := make([]string, 0, len(pkgs))
Expand All @@ -608,7 +608,7 @@ func TestGetPackageDependencies(t *testing.T) {
_, index := testGetPackagesAndIndex()

resolver := NewPkgResolver(context.Background(), testNamedRepositoryFromIndexes(index))
_, pkgs, _, err := resolver.GetPackageWithDependencies("package3", nil, map[*RepositoryPackage]string{})
_, pkgs, _, err := resolver.GetPackageWithDependencies(context.Background(), "package3", nil, map[*RepositoryPackage]string{})
require.NoErrorf(t, err, "unable to get dependencies")

actual := make([]string, 0, len(pkgs))
Expand All @@ -617,42 +617,14 @@ func TestGetPackageDependencies(t *testing.T) {
}
require.True(t, reflect.DeepEqual(expected, actual), "dependencies mismatch:\nactual %v\nexpect %v", actual, expected)
})
t.Run("self-fulfill", func(t *testing.T) {
_, index := testGetPackagesAndIndex()

resolver := NewPkgResolver(context.Background(), testNamedRepositoryFromIndexes(index))
pkg6, err := resolver.ResolvePackage("package6", map[*RepositoryPackage]string{})
require.NoErrorf(t, err, "unable to resolve package6")
require.GreaterOrEqual(t, len(pkg6), 1, "package6 should have at least one match")
tests := []struct {
name string
expected []string
allow bool
}{
{"allowed", []string{"package5"}, true},
{"not allowed", []string{"package6", "package5"}, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
deps, _, err := resolver.getPackageDependencies(pkg6[0], "", tt.allow, nil, nil, nil, map[*RepositoryPackage]string{})
require.NoErrorf(t, err, "unable to get dependencies")

actual := make([]string, 0, len(deps))
for _, p := range deps {
actual = append(actual, p.Name)
}
require.True(t, reflect.DeepEqual(tt.expected, actual), "dependencies mismatch:\nactual %v\nexpect %v", actual, tt.expected)
})
}
})
t.Run("existing dependency", func(t *testing.T) {
origPkgs, index := testGetPackagesAndIndex()
resolver := NewPkgResolver(context.Background(), testNamedRepositoryFromIndexes(index))

// start with regular resolution, just to compare
expectedName := "package5"
expectedVersion := "2.0.0" // highest version
_, pkgs, _, err := resolver.GetPackageWithDependencies("package9", nil, map[*RepositoryPackage]string{})
_, pkgs, _, err := resolver.GetPackageWithDependencies(context.Background(), "package9", nil, map[*RepositoryPackage]string{})
require.NoErrorf(t, err, "unable to get dependencies")
require.Len(t, pkgs, 1, "package9 should have one dependency, %s", expectedName)
require.Equal(t, expectedName, pkgs[0].Name)
Expand All @@ -668,7 +640,7 @@ func TestGetPackageDependencies(t *testing.T) {
break
}
}
_, pkgs, _, err = resolver.GetPackageWithDependencies("package9", existingPkgs, map[*RepositoryPackage]string{})
_, pkgs, _, err = resolver.GetPackageWithDependencies(context.Background(), "package9", existingPkgs, map[*RepositoryPackage]string{})
require.NoErrorf(t, err, "unable to get dependencies")
require.Len(t, pkgs, 1, "package9 should have one dependency, %s", expectedName)
require.Equal(t, expectedName, pkgs[0].Name)
Expand Down

0 comments on commit f870d65

Please sign in to comment.