Quadlet - make sure the order of the UnitsDir is deterministic

Change getUnitDirs to maintain a slice in addition to the map and return the slice
Add helper functions to make the code more readable
Adjust unit tests
Restore system test

Signed-off-by: Ygal Blum <ygal.blum@gmail.com>

cmd/quadlet/main.go

@@ -103,16 +103,39 @@ func Debugf(format string, a ...interface{}) {
 	}
 }
 
+type searchPaths struct {
+	sorted []string
+	// map to store paths so we can quickly check if we saw them already and not loop in case of symlinks
+	visitedDirs map[string]struct{}
+}
+
+func newSearchPaths() *searchPaths {
+	return &searchPaths{
+		sorted:      make([]string, 0),
+		visitedDirs: make(map[string]struct{}, 0),
+	}
+}
+
+func (s *searchPaths) Add(path string) {
+	s.sorted = append(s.sorted, path)
+	s.visitedDirs[path] = struct{}{}
+}
+
+func (s *searchPaths) Visited(path string) bool {
+	_, visited := s.visitedDirs[path]
+	return visited
+}
+
 // This returns the directories where we read quadlet .container and .volumes from
 // For system generators these are in /usr/share/containers/systemd (for distro files)
 // and /etc/containers/systemd (for sysadmin files).
 // For user generators these can live in $XDG_RUNTIME_DIR/containers/systemd, /etc/containers/systemd/users, /etc/containers/systemd/users/$UID, and $XDG_CONFIG_HOME/containers/systemd
-func getUnitDirs(rootless bool) map[string]struct{} {
+func getUnitDirs(rootless bool) []string {
-	dirs := make(map[string]struct{}, 0)
+	paths := newSearchPaths()
 
 	// Allow overriding source dir, this is mainly for the CI tests
-	if getDirsFromEnv(dirs) {
+	if getDirsFromEnv(paths) {
-		return dirs
+		return paths.sorted
 	}
 
 	resolvedUnitDirAdminUser := resolveUnitDirAdminUser()
@@ -121,14 +144,14 @@ func getUnitDirs(rootless bool) map[string]struct{} {
 	if rootless {
 		systemUserDirLevel := len(strings.Split(resolvedUnitDirAdminUser, string(os.PathSeparator)))
 		nonNumericFilter := getNonNumericFilter(resolvedUnitDirAdminUser, systemUserDirLevel)
-		getRootlessDirs(dirs, nonNumericFilter, userLevelFilter)
+		getRootlessDirs(paths, nonNumericFilter, userLevelFilter)
 	} else {
-		getRootDirs(dirs, userLevelFilter)
+		getRootDirs(paths, userLevelFilter)
 	}
-	return dirs
+	return paths.sorted
 }
 
-func getDirsFromEnv(dirs map[string]struct{}) bool {
+func getDirsFromEnv(paths *searchPaths) bool {
 	unitDirsEnv := os.Getenv("QUADLET_UNIT_DIRS")
 	if len(unitDirsEnv) == 0 {
 		return false
@@ -139,15 +162,15 @@ func getDirsFromEnv(dirs map[string]struct{}) bool {
 			Logf("%s not a valid file path", eachUnitDir)
 			break
 		}
-		appendSubPaths(dirs, eachUnitDir, false, nil)
+		appendSubPaths(paths, eachUnitDir, false, nil)
 	}
 	return true
 }
 
-func getRootlessDirs(dirs map[string]struct{}, nonNumericFilter, userLevelFilter func(string, bool) bool) {
+func getRootlessDirs(paths *searchPaths, nonNumericFilter, userLevelFilter func(string, bool) bool) {
 	runtimeDir, found := os.LookupEnv("XDG_RUNTIME_DIR")
 	if found {
-		appendSubPaths(dirs, path.Join(runtimeDir, "containers/systemd"), false, nil)
+		appendSubPaths(paths, path.Join(runtimeDir, "containers/systemd"), false, nil)
 	}
 
 	configDir, err := os.UserConfigDir()
@@ -155,23 +178,23 @@ func getRootlessDirs(dirs map[string]struct{}, nonNumericFilter, userLevelFilter
 		fmt.Fprintf(os.Stderr, "Warning: %v", err)
 		return
 	}
-	appendSubPaths(dirs, path.Join(configDir, "containers/systemd"), false, nil)
+	appendSubPaths(paths, path.Join(configDir, "containers/systemd"), false, nil)
 
 	u, err := user.Current()
 	if err == nil {
-		appendSubPaths(dirs, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
+		appendSubPaths(paths, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
-		appendSubPaths(dirs, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
+		appendSubPaths(paths, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
 	} else {
 		fmt.Fprintf(os.Stderr, "Warning: %v", err)
 	}
 
-	dirs[filepath.Join(quadlet.UnitDirAdmin, "users")] = struct{}{}
+	paths.Add(filepath.Join(quadlet.UnitDirAdmin, "users"))
 }
 
-func getRootDirs(dirs map[string]struct{}, userLevelFilter func(string, bool) bool) {
+func getRootDirs(paths *searchPaths, userLevelFilter func(string, bool) bool) {
-	appendSubPaths(dirs, quadlet.UnitDirTemp, false, userLevelFilter)
+	appendSubPaths(paths, quadlet.UnitDirTemp, false, userLevelFilter)
-	appendSubPaths(dirs, quadlet.UnitDirAdmin, false, userLevelFilter)
+	appendSubPaths(paths, quadlet.UnitDirAdmin, false, userLevelFilter)
-	appendSubPaths(dirs, quadlet.UnitDirDistro, false, nil)
+	appendSubPaths(paths, quadlet.UnitDirDistro, false, nil)
 }
 
 func resolveUnitDirAdminUser() string {
@@ -187,7 +210,7 @@ func resolveUnitDirAdminUser() string {
 	return resolvedUnitDirAdminUser
 }
 
-func appendSubPaths(dirs map[string]struct{}, path string, isUserFlag bool, filterPtr func(string, bool) bool) {
+func appendSubPaths(paths *searchPaths, path string, isUserFlag bool, filterPtr func(string, bool) bool) {
 	resolvedPath, err := filepath.EvalSymlinks(path)
 	if err != nil {
 		if !errors.Is(err, fs.ErrNotExist) {
@@ -195,40 +218,16 @@ func appendSubPaths(dirs map[string]struct{}, path string, isUserFlag bool, filt
 		}
 		// Despite the failure add the path to the list for logging purposes
 		// This is the equivalent of adding the path when info==nil below
-		dirs[path] = struct{}{}
+		paths.Add(path)
 		return
 	}
 
-	// If the resolvedPath is already in the map no need to read it again
+	if skipPath(paths, resolvedPath, isUserFlag, filterPtr) {
-	if _, already := dirs[resolvedPath]; already {
-		return
-	}
-
-	// Don't traverse drop-in directories
-	if strings.HasSuffix(resolvedPath, ".d") {
-		return
-	}
-
-	// Check if the directory should be filtered out
-	if filterPtr != nil && !filterPtr(resolvedPath, isUserFlag) {
-		return
-	}
-
-	stat, err := os.Stat(resolvedPath)
-	if err != nil {
-		if !errors.Is(err, fs.ErrNotExist) {
-			Debugf("Error occurred resolving path %q: %s", path, err)
-		}
-		return
-	}
-
-	// Not a directory nothing to add
-	if !stat.IsDir() {
 		return
 	}
 
 	// Add the current directory
-	dirs[resolvedPath] = struct{}{}
+	paths.Add(resolvedPath)
 
 	// Read the contents of the directory
 	entries, err := os.ReadDir(resolvedPath)
@@ -242,10 +241,38 @@ func appendSubPaths(dirs map[string]struct{}, path string, isUserFlag bool, filt
 	// Recursively run through the contents of the directory
 	for _, entry := range entries {
 		fullPath := filepath.Join(resolvedPath, entry.Name())
-		appendSubPaths(dirs, fullPath, isUserFlag, filterPtr)
+		appendSubPaths(paths, fullPath, isUserFlag, filterPtr)
 	}
 }
 
+func skipPath(paths *searchPaths, path string, isUserFlag bool, filterPtr func(string, bool) bool) bool {
+	// If the path is already in the map no need to read it again
+	if paths.Visited(path) {
+		return true
+	}
+
+	// Don't traverse drop-in directories
+	if strings.HasSuffix(path, ".d") {
+		return true
+	}
+
+	// Check if the directory should be filtered out
+	if filterPtr != nil && !filterPtr(path, isUserFlag) {
+		return true
+	}
+
+	stat, err := os.Stat(path)
+	if err != nil {
+		if !errors.Is(err, fs.ErrNotExist) {
+			Debugf("Error occurred resolving path %q: %s", path, err)
+		}
+		return true
+	}
+
+	// Not a directory nothing to add
+	return !stat.IsDir()
+}
+
 func getNonNumericFilter(resolvedUnitDirAdminUser string, systemUserDirLevel int) func(string, bool) bool {
 	return func(path string, isUserFlag bool) bool {
 		// when running in rootless, recursive walk directories that are non numeric
@@ -323,7 +350,7 @@ func loadUnitsFromDir(sourcePath string) ([]*parser.UnitFile, error) {
 	return units, prevError
 }
 
-func loadUnitDropins(unit *parser.UnitFile, sourcePaths map[string]struct{}) error {
+func loadUnitDropins(unit *parser.UnitFile, sourcePaths []string) error {
 	var prevError error
 	reportError := func(err error) {
 		if prevError != nil {
@@ -335,7 +362,7 @@ func loadUnitDropins(unit *parser.UnitFile, sourcePaths map[string]struct{}) err
 	dropinDirs := []string{}
 	unitDropinPaths := unit.GetUnitDropinPaths()
 
-	for sourcePath := range sourcePaths {
+	for _, sourcePath := range sourcePaths {
 		for _, dropinPath := range unitDropinPaths {
 			dropinDirs = append(dropinDirs, path.Join(sourcePath, dropinPath))
 		}
@@ -649,7 +676,7 @@ func process() error {
 	sourcePathsMap := getUnitDirs(isUserFlag)
 
 	var units []*parser.UnitFile
-	for d := range sourcePathsMap {
+	for _, d := range sourcePathsMap {
 		if result, err := loadUnitsFromDir(d); err != nil {
 			reportError(err)
 		} else {

cmd/quadlet/main_test.go

@@ -64,36 +64,36 @@ func TestUnitDirs(t *testing.T) {
 
 		resolvedUnitDirAdminUser := resolveUnitDirAdminUser()
 		userLevelFilter := getUserLevelFilter(resolvedUnitDirAdminUser)
-		rootDirs := make(map[string]struct{}, 0)
+		rootfulPaths := newSearchPaths()
-		appendSubPaths(rootDirs, quadlet.UnitDirTemp, false, userLevelFilter)
+		appendSubPaths(rootfulPaths, quadlet.UnitDirTemp, false, userLevelFilter)
-		appendSubPaths(rootDirs, quadlet.UnitDirAdmin, false, userLevelFilter)
+		appendSubPaths(rootfulPaths, quadlet.UnitDirAdmin, false, userLevelFilter)
-		appendSubPaths(rootDirs, quadlet.UnitDirDistro, false, userLevelFilter)
+		appendSubPaths(rootfulPaths, quadlet.UnitDirDistro, false, userLevelFilter)
-		assert.Equal(t, rootDirs, unitDirs, "rootful unit dirs should match")
+		assert.Equal(t, rootfulPaths.sorted, unitDirs, "rootful unit dirs should match")
 
 		configDir, err := os.UserConfigDir()
 		assert.Nil(t, err)
 
-		rootlessDirs := make(map[string]struct{}, 0)
+		rootlessPaths := newSearchPaths()
 
 		systemUserDirLevel := len(strings.Split(resolvedUnitDirAdminUser, string(os.PathSeparator)))
 		nonNumericFilter := getNonNumericFilter(resolvedUnitDirAdminUser, systemUserDirLevel)
 
 		runtimeDir, found := os.LookupEnv("XDG_RUNTIME_DIR")
 		if found {
-			appendSubPaths(rootlessDirs, path.Join(runtimeDir, "containers/systemd"), false, nil)
+			appendSubPaths(rootlessPaths, path.Join(runtimeDir, "containers/systemd"), false, nil)
 		}
-		appendSubPaths(rootlessDirs, path.Join(configDir, "containers/systemd"), false, nil)
+		appendSubPaths(rootlessPaths, path.Join(configDir, "containers/systemd"), false, nil)
-		appendSubPaths(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
+		appendSubPaths(rootlessPaths, filepath.Join(quadlet.UnitDirAdmin, "users"), true, nonNumericFilter)
-		appendSubPaths(rootlessDirs, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
+		appendSubPaths(rootlessPaths, filepath.Join(quadlet.UnitDirAdmin, "users", u.Uid), true, userLevelFilter)
-		rootlessDirs[filepath.Join(quadlet.UnitDirAdmin, "users")] = struct{}{}
+		rootlessPaths.Add(filepath.Join(quadlet.UnitDirAdmin, "users"))
 
 		unitDirs = getUnitDirs(true)
-		assert.Equal(t, rootlessDirs, unitDirs, "rootless unit dirs should match")
+		assert.Equal(t, rootlessPaths.sorted, unitDirs, "rootless unit dirs should match")
 
 		// Test that relative path returns an empty list
 		t.Setenv("QUADLET_UNIT_DIRS", "./relative/path")
 		unitDirs = getUnitDirs(false)
-		assert.Equal(t, map[string]struct{}{}, unitDirs)
+		assert.Equal(t, []string{}, unitDirs)
 
 		name, err := os.MkdirTemp("", "dir")
 		assert.Nil(t, err)
@@ -102,10 +102,10 @@ func TestUnitDirs(t *testing.T) {
 
 		t.Setenv("QUADLET_UNIT_DIRS", name)
 		unitDirs = getUnitDirs(false)
-		assert.Equal(t, map[string]struct{}{name: {}}, unitDirs, "rootful should use environment variable")
+		assert.Equal(t, []string{name}, unitDirs, "rootful should use environment variable")
 
 		unitDirs = getUnitDirs(true)
-		assert.Equal(t, map[string]struct{}{name: {}}, unitDirs, "rootless should use environment variable")
+		assert.Equal(t, []string{name}, unitDirs, "rootless should use environment variable")
 
 		symLinkTestBaseDir, err := os.MkdirTemp("", "podman-symlinktest")
 		assert.Nil(t, err)
@@ -123,7 +123,7 @@ func TestUnitDirs(t *testing.T) {
 		assert.Nil(t, err)
 		t.Setenv("QUADLET_UNIT_DIRS", symlink)
 		unitDirs = getUnitDirs(true)
-		assert.Equal(t, map[string]struct{}{actualDir: {}, innerDir: {}}, unitDirs, "directory resolution should follow symlink")
+		assert.Equal(t, []string{actualDir, innerDir}, unitDirs, "directory resolution should follow symlink")
 
 		// Make a more elborate test with the following structure:
 		// <BASE>/linkToDir - real directory to link to
@@ -137,44 +137,46 @@ func TestUnitDirs(t *testing.T) {
 		// <BASE>/unitDir/b/a - real directory
 		// <BASE>/unitDir/b/b - link to <BASE>/unitDir/a/a should be ignored
 		// <BASE>/unitDir/c - link to <BASE>/linkToDir
-		symLinkRecursiveTestBaseDir, err := os.MkdirTemp("", "podman-symlink-recursive-test")
+		createDir := func(path, name string, dirs []string) (string, []string) {
-		assert.Nil(t, err)
-		// remove the temporary directory at the end of the program
-		defer os.RemoveAll(symLinkRecursiveTestBaseDir)
-
-		createDir := func(path, name string, dirs map[string]struct{}) string {
 			dirName := filepath.Join(path, name)
 			assert.NotContains(t, dirs, dirName)
 			err = os.Mkdir(dirName, 0755)
 			assert.Nil(t, err)
-			dirs[dirName] = struct{}{}
+			dirs = append(dirs, dirName)
-			return dirName
+			return dirName, dirs
 		}
-		expectedDirs := make(map[string]struct{}, 0)
-		// Create <BASE>/linkToDir
-		linkToDirPath := createDir(symLinkRecursiveTestBaseDir, "linkToDir", expectedDirs)
-		// Create <BASE>/linkToDir/a
-		createDir(linkToDirPath, "a", expectedDirs)
-		// Create <BASE>/unitDir
-		unitsDirPath := createDir(symLinkRecursiveTestBaseDir, "unitsDir", expectedDirs)
-		// Create <BASE>/unitDir/a
-		aDirPath := createDir(unitsDirPath, "a", expectedDirs)
-		// Create <BASE>/unitDir/a/a
-		aaDirPath := createDir(aDirPath, "a", expectedDirs)
-		// Create <BASE>/unitDir/a/b
-		createDir(aDirPath, "b", expectedDirs)
-		// Create <BASE>/unitDir/a/a/a
-		createDir(aaDirPath, "a", expectedDirs)
-		// Create <BASE>/unitDir/b
-		bDirPath := createDir(unitsDirPath, "b", expectedDirs)
-		// Create <BASE>/unitDir/b/a
-		baDirPath := createDir(bDirPath, "a", expectedDirs)
 
 		linkDir := func(path, name, target string) {
 			linkName := filepath.Join(path, name)
 			err = os.Symlink(target, linkName)
 			assert.Nil(t, err)
 		}
+
+		symLinkRecursiveTestBaseDir, err := os.MkdirTemp("", "podman-symlink-recursive-test")
+		assert.Nil(t, err)
+		// remove the temporary directory at the end of the program
+		defer os.RemoveAll(symLinkRecursiveTestBaseDir)
+
+		expectedDirs := make([]string, 0)
+		// Create <BASE>/unitDir
+		unitsDirPath, expectedDirs := createDir(symLinkRecursiveTestBaseDir, "unitsDir", expectedDirs)
+		// Create <BASE>/unitDir/a
+		aDirPath, expectedDirs := createDir(unitsDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir/a/a
+		aaDirPath, expectedDirs := createDir(aDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir/a/a/a
+		_, expectedDirs = createDir(aaDirPath, "a", expectedDirs)
+		// Create <BASE>/unitDir/a/b
+		_, expectedDirs = createDir(aDirPath, "b", expectedDirs)
+		// Create <BASE>/unitDir/b
+		bDirPath, expectedDirs := createDir(unitsDirPath, "b", expectedDirs)
+		// Create <BASE>/unitDir/b/a
+		baDirPath, expectedDirs := createDir(bDirPath, "a", expectedDirs)
+		// Create <BASE>/linkToDir
+		linkToDirPath, expectedDirs := createDir(symLinkRecursiveTestBaseDir, "linkToDir", expectedDirs)
+		// Create <BASE>/linkToDir/a
+		_, expectedDirs = createDir(linkToDirPath, "a", expectedDirs)
+
 		// Link <BASE>/unitDir/b/b to <BASE>/unitDir/a/a
 		linkDir(bDirPath, "b", aaDirPath)
 		// Link <BASE>/linkToDir/b to <BASE>/unitDir/b/a

test/e2e/quadlet_test.go

@@ -713,7 +713,7 @@ var _ = Describe("quadlet system generator", func() {
 			Expect(session).Should(Exit(0))
 
 			current := session.ErrorToStringArray()
-			expected := "No files parsed from map[/something:{}]"
+			expected := "No files parsed from [/something]"
 
 			found := false
 			for _, line := range current {

test/system/252-quadlet.bats

@@ -221,8 +221,6 @@ EOF
 }
 
 @test "quadlet conflict names" {
-    skip "FIXME: #24047, temporary skip because this is an intense flake"
-
     # If two directories in the search have files with the same name, quadlet should
     # only process the first name
     dir1=$PODMAN_TMPDIR/$(random_string)
@@ -232,13 +230,13 @@ EOF
 
     cat > $dir1/$quadlet_file <<EOF
 [Container]
-Image=$IMAGE
+Image=quay.io/libpod/this-is-the-one:wewant
 Notify=yes
 EOF
 
     cat > $dir2/$quadlet_file <<EOF
 [Container]
-Image=$IMAGE
+Image=quay.io/libpod/bad-bad-bad:nonono
 Notify=no
 EOF
     QUADLET_UNIT_DIRS="$dir1:$dir2" run \