bug: fix remote build dependency variability (#12842)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Pawel Winogrodzki <pawelwi@microsoft.com>

Author: 3 people

toolkit/tools/grapher/grapher.go

@@ -12,6 +12,7 @@ import (
 	"github.com/microsoft/azurelinux/toolkit/tools/internal/packagerepo/repocloner/rpmrepocloner"
 	"github.com/microsoft/azurelinux/toolkit/tools/internal/pkggraph"
 	"github.com/microsoft/azurelinux/toolkit/tools/internal/pkgjson"
+	"github.com/microsoft/azurelinux/toolkit/tools/internal/sliceutils"
 	"github.com/microsoft/azurelinux/toolkit/tools/internal/timestamp"
 	"github.com/microsoft/azurelinux/toolkit/tools/pkg/profile"
 
@@ -23,14 +24,13 @@ var (
 	input  = exe.InputFlag(app, "Input json listing all local SRPMs")
 	output = exe.OutputFlag(app, "Output file to export the graph to")
 
-	logFlags              = exe.SetupLogFlags(app)
-	profFlags             = exe.SetupProfileFlags(app)
-	strictGoals           = app.Flag("strict-goals", "Don't allow missing goal packages").Bool()
-	strictUnresolved      = app.Flag("strict-unresolved", "Don't allow missing unresolved packages").Bool()
-	timestampFile         = app.Flag("timestamp-file", "File that stores timestamps for this program.").String()
-	usePMCtoResolveCycles = app.Flag("usePMCtoresolvecycles", "Cycles will be resolved by downloading rpm packages from PMC if locally unavailable").Bool()
-	tlsClientCert         = app.Flag("tls-cert", "TLS client certificate to use when downloading files.").String()
-	tlsClientKey          = app.Flag("tls-key", "TLS client key to use when downloading files.").String()
+	logFlags         = exe.SetupLogFlags(app)
+	profFlags        = exe.SetupProfileFlags(app)
+	strictGoals      = app.Flag("strict-goals", "Don't allow missing goal packages").Bool()
+	strictUnresolved = app.Flag("strict-unresolved", "Don't allow missing unresolved packages").Bool()
+	timestampFile    = app.Flag("timestamp-file", "File that stores timestamps for this program.").String()
+	tlsClientCert    = app.Flag("tls-cert", "TLS client certificate to use when downloading files.").String()
+	tlsClientKey     = app.Flag("tls-key", "TLS client key to use when downloading files.").String()
 
 	resolveCyclesFromUpstream     = app.Flag("resolve-cycles-from-upstream", "Let grapher resolve cycles by marking rpms available in repo as remote").Bool()
 	outDir                        = exe.OutputDirFlag(app, "Directory to download packages into.")
@@ -43,8 +43,6 @@ var (
 	disableDefaultRepos           = app.Flag("disable-default-repos", "Disable pulling packages from PMC repos").Bool()
 	ignoreVersionToResolveSelfDep = app.Flag("ignore-version-to-resolve-selfdep", "Ignore package version while downloading package from upstream when resolving cycle").Bool()
 	repoSnapshotTime              = app.Flag("repo-snapshot-time", "Optional: Repo time limit for tdnf virtual snapshot").String()
-
-	depGraph = pkggraph.NewPkgGraph()
 )
 
 func main() {
@@ -113,7 +111,7 @@ func main() {
 }
 
 // addUnresolvedPackage adds an unresolved node to the graph representing the
-// packged described in the PackgetVer structure. Returns an error if the node
+// package described in the PackageVer structure. Returns an error if the node
 // could not be created.
 func addUnresolvedPackage(g *pkggraph.PkgGraph, pkgVer *pkgjson.PackageVer) (newRunNode *pkggraph.PkgNode, err error) {
 	logger.Log.Debugf("Adding unresolved %s", pkgVer)
@@ -122,7 +120,9 @@ func addUnresolvedPackage(g *pkggraph.PkgGraph, pkgVer *pkgjson.PackageVer) (new
 		return
 	}
 
-	nodes, err := g.FindBestPkgNode(pkgVer)
+	// Double check that the package is not already in the graph. A previous check should have already found the node
+	// and no call to addUnresolvedPackage() should have been made.
+	nodes, err := g.FindExactPkgNodeFromPkg(pkgVer)
 	if err != nil {
 		return
 	}
@@ -205,6 +205,36 @@ func addNodesForPackage(g *pkggraph.PkgGraph, pkg *pkgjson.Package) (foundDuplic
 	return
 }
 
+// findOrAddExactRemoteDependency ensures that a remote node is available in the graph for every unresolved dependency.
+// 1. Check if the exact dependency is already in the graph. If it is, reuse it.
+// 2. If it is not, create a new unresolved node for the dependency.
+// It is important that we only match on the exact dependency name and version. If we don't, we may end up with
+// unpredictable behavior in the scheduler. If two different remote dependencies are added to two different build
+// nodes of a single SRPM, then the scheduler may queue that node twice.
+func findOrAddExactRemoteDependency(g *pkggraph.PkgGraph, dependency *pkgjson.PackageVer) (selectedRemoteNode *pkggraph.PkgNode, err error) {
+	existingRemoteNode, err := g.FindExactPkgNodeFromPkg(dependency)
+	if err != nil {
+		err = fmt.Errorf("failed to check lookup list for exact remote %+v:\n%w", dependency, err)
+		return nil, err
+	}
+
+	if existingRemoteNode == nil {
+		// No exact match, add a new one.
+		selectedRemoteNode, err = addUnresolvedPackage(g, dependency)
+		if err != nil {
+			err = fmt.Errorf("failed to add a remote node (%s):\n%w", dependency.Name, err)
+			return nil, err
+		}
+		logger.Log.Debugf("Added new node: '%s' for dependency %+v", selectedRemoteNode.FriendlyName(), dependency)
+	} else {
+		// This exact dependency is already in the graph, so reuse it.
+		selectedRemoteNode = existingRemoteNode.RunNode
+		logger.Log.Debugf("Found existing exact remote node: '%s' for dependency %+v", selectedRemoteNode.FriendlyName(), dependency)
+	}
+
+	return selectedRemoteNode, nil
+}
+
 // addSingleDependency will add an edge between packageNode and the "Run" node for the
 // dependency described in the PackageVer structure. Returns an error if the
 // addition failed.
@@ -217,15 +247,17 @@ func addSingleDependency(g *pkggraph.PkgGraph, packageNode *pkggraph.PkgNode, de
 		return err
 	}
 
-	if nodes == nil {
-		dependentNode, err = addUnresolvedPackage(g, dependency)
+	// If we can't find the dependency in the graph, or it is a remote dependency, we need to do a bit of extra validation.
+	if nodes == nil || nodes.RunNode.Type != pkggraph.TypeLocalRun {
+		dependentNode, err = findOrAddExactRemoteDependency(g, dependency)
 		if err != nil {
-			err = fmt.Errorf("failed to add a package (%s):\n%w", dependency.Name, err)
+			err = fmt.Errorf("failed to handle remote dependency from %+v to %+v:\n%w", packageNode.VersionedPkg, dependency, err)
 			return err
 		}
 	} else {
 		// All dependencies are assumed to be "Run" dependencies
 		dependentNode = nodes.RunNode
+		logger.Log.Debugf("Found existing node: '%s' for dependency %+v", dependentNode.FriendlyName(), dependency)
 	}
 
 	if packageNode == dependentNode {
@@ -335,10 +367,14 @@ func populateGraph(graph *pkggraph.PkgGraph, repo *pkgjson.PackageRepo) (err err
 	timestamp.StopEvent(nil) // add package nodes
 	timestamp.StartEvent("add dependencies", nil)
 
+	// Sort the map to ensure the order is deterministic
+	packageList := sliceutils.MapToSlice(uniquePackages)
+	pkgjson.SortPackageList(packageList)
+
 	// Rescan and add all the dependencies
 	logger.Log.Infof("Adding all dependencies from (%s)", *input)
 	dependenciesAdded := 0
-	for uniquePkg := range uniquePackages {
+	for _, uniquePkg := range packageList {
 		num, err := addPkgDependencies(graph, uniquePkg)
 		if err != nil {
 			err = fmt.Errorf("failed to add dependency %+v:\n%w", uniquePkg, err)

toolkit/tools/grapher/grapher_test.go

@@ -0,0 +1,147 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+package main
+
+import (
+	"os"
+	"testing"
+
+	"github.com/microsoft/azurelinux/toolkit/tools/internal/logger"
+	"github.com/microsoft/azurelinux/toolkit/tools/internal/pkggraph"
+	"github.com/microsoft/azurelinux/toolkit/tools/internal/pkgjson"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMain(m *testing.M) {
+	logger.InitStderrLog()
+	logger.Log.SetLevel(logrus.FatalLevel)
+	os.Exit(m.Run())
+}
+
+// Make a "local" package of the given name. Each package with the same pkgName will share the same
+// requires, srpmpath, etc. The subPkgName is used to differentiate multiple run-nodes for each
+// package. The buildRequires should be common across all packages with the same pkgName.
+func makePackage(pkgName, subPkgName string, buildRequires []*pkgjson.PackageVer) *pkgjson.Package {
+	// Create a package with the given name and version
+	pkg := &pkgjson.Package{
+		Provides: &pkgjson.PackageVer{
+			Name:    subPkgName,
+			Version: "1.0",
+		},
+		SrpmPath:      pkgName + ".src.rpm",
+		RpmPath:       pkgName + ".rpm",
+		SourceDir:     pkgName + "-src",
+		SpecPath:      pkgName + ".spec",
+		Architecture:  "x86_64",
+		IsToolchain:   false,
+		RunTests:      true,
+		BuildRequires: buildRequires,
+	}
+
+	return pkg
+}
+
+// A very simple test case to validate that the test framework is working
+func TestValidateFramework(t *testing.T) {
+	testRepo := &pkgjson.PackageRepo{
+		Repo: []*pkgjson.Package{
+			makePackage("pkg1", "pkg1-a", nil),
+			makePackage("pkg1", "pkg1-b", nil),
+		},
+	}
+	depGraph := pkggraph.NewPkgGraph()
+	err := populateGraph(depGraph, testRepo)
+	require.NoError(t, err)
+
+	// Validate that the graph is not empty
+	require.Equal(t, 2, len(depGraph.AllRunNodes()))
+	require.Equal(t, 2, len(depGraph.AllBuildNodes()))
+}
+
+// Load a specially crafted repo containing several sub-packages of a single SRPM, and two malicious
+// packages that try to pollute the available remote dependencies. The test will check if the various build
+// nodes of the sub-package SRPM are all using the same set of dependencies. The other packages will add
+// new remote nodes to the graph since they may not be satisfied by the already existing nodes. Once
+// this happens, the sub-package build nodes may start using the "better" dependencies, which is not what we want.
+func TestScenarioMultiRemoteProvides(t *testing.T) {
+	anyVerBr := []*pkgjson.PackageVer{
+		{
+			Name:       "build-req",
+			Version:    "",
+			Condition:  "",
+			SVersion:   "",
+			SCondition: "",
+		},
+	}
+	lowVerBr := []*pkgjson.PackageVer{
+		{
+			Name:       "build-req",
+			Version:    "1.0",
+			Condition:  "<",
+			SVersion:   "",
+			SCondition: "",
+		},
+	}
+	highVerBr := []*pkgjson.PackageVer{
+		{
+			Name:       "build-req",
+			Version:    "2.0",
+			Condition:  ">",
+			SVersion:   "",
+			SCondition: "",
+		},
+	}
+
+	// Define the test repo
+	testRepo := pkgjson.PackageRepo{
+		Repo: []*pkgjson.Package{
+			makePackage("pkg1", "pkg1-a", anyVerBr),
+			makePackage("other-pkg-1", "other-pkg-1", lowVerBr),
+			makePackage("pkg1", "pkg1-b", anyVerBr),
+			makePackage("other-pkg-2", "other-pkg-2", highVerBr),
+			makePackage("pkg1", "pkg1-c", anyVerBr),
+		},
+	}
+
+	// The graph library is non-deterministic (likely due to the use of maps), so to accurately catch the
+	// counter example we need to run the test multiple times. Experimentally, the test fails after about
+	// 10 runs (p=0.1), so with 1000 runs the probability of false negatives is 0.9^1000 ~= 1.7E-46 (ie very
+	// small).
+	const numRuns = 1000
+	for i := 0; i < numRuns; i++ {
+		// Create the dependency graph
+		depGraph := pkggraph.NewPkgGraph()
+		err := populateGraph(depGraph, &testRepo)
+		require.NoError(t, err)
+
+		// Check our invariants
+		actualDepsUsed := make(map[pkgjson.PackageVer]bool)
+		buildNodes := depGraph.AllBuildNodes()
+		for _, buildNode := range buildNodes {
+
+			// Only care about the pkg1 build nodes
+			if buildNode.SrpmPath != "pkg1.src.rpm" {
+				continue
+			}
+
+			dependencies := depGraph.From(buildNode.ID())
+
+			for dependencies.Next() {
+				dep := dependencies.Node().(*pkggraph.PkgNode)
+				// Add to the set
+				actualDepsUsed[*dep.VersionedPkg] = true
+			}
+		}
+		// Now check the map, if there is more than one entry something is wrong
+		if len(actualDepsUsed) > 1 {
+			t.Error("Build deps:")
+			for dep := range actualDepsUsed {
+				t.Logf("\t%s", dep)
+			}
+			assert.FailNowf(t, "Multiple dependencies used in a single build node", "Failed after %d runs", i+1)
+		}
+	}
+}

toolkit/tools/internal/pkgjson/pkgjson.go

@@ -6,6 +6,7 @@ package pkgjson
 import (
 	"fmt"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/microsoft/azurelinux/toolkit/tools/internal/jsonutils"
@@ -73,11 +74,49 @@ type Package struct {
 	RunTests      bool          `json:"RunTests"`      // Should we run tests for this package.
 }
 
+func packageLess(a, b PackageVer) bool {
+	if a.Name == b.Name {
+		v1 := versioncompare.New(a.Version)
+		v2 := versioncompare.New(b.Version)
+		return v1.Compare(v2) < 0
+	}
+
+	return a.Name < b.Name
+}
+
+// SortPackageList ensures the package list is sorted in a deterministic way. It will primarily sort by name, then
+// version. It will also sort the package lists (requires, buildRequires, testRequires) of each package.
+func SortPackageList(packages []*Package) {
+	sort.Slice(packages, func(i, j int) bool {
+		return packageLess(*packages[i].Provides, *packages[j].Provides)
+	})
+
+	// For each package, also sort the lists of its dependencies
+	for _, pkg := range packages {
+		sort.Slice(pkg.Requires, func(i, j int) bool {
+			return packageLess(*pkg.Requires[i], *pkg.Requires[j])
+		})
+		sort.Slice(pkg.BuildRequires, func(i, j int) bool {
+			return packageLess(*pkg.BuildRequires[i], *pkg.BuildRequires[j])
+		})
+		sort.Slice(pkg.TestRequires, func(i, j int) bool {
+			return packageLess(*pkg.TestRequires[i], *pkg.TestRequires[j])
+		})
+	}
+}
+
 // ParsePackageJSON reads a package list json file
 func (pkg *PackageRepo) ParsePackageJSON(path string) (err error) {
 	logger.Log.Infof("Opening %s", path)
 
-	return jsonutils.ReadJSONFile(path, &pkg)
+	if err = jsonutils.ReadJSONFile(path, &pkg); err != nil {
+		return err
+	}
+
+	// Ensure deterministic ordering of the package list
+	SortPackageList(pkg.Repo)
+
+	return nil
 }
 
 // IsImplicitPackage returns true if a PackageVer represents an implicit provide.