Merge pull request #1212 from anderbubble/syncuser-refactor

Refactor syncuser to reduce walks of the container fs

CHANGELOG.md

@@ -42,12 +42,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Changed
 
 - Replace reference to docusaurus with Sphinx
+- `wwctl container import` now only runs syncuser if explicitly requested. #1212
 
 ### Fixed
 
 - Block unprivileged requests for arbitrary overlays in secure mode. #1215
 - Fix installation docs to use github.com/warewulf instead of github.com/hpcng. #1219
 - Fix the issue that warewulf.conf parse does not support CIDR format. #1130
+- Reduce the number of times syncuser walks the container file system. #1209
 
 ### Security
 

internal/app/wwctl/container/exec/main.go

@@ -121,7 +121,7 @@ func CobraRunE(cmd *cobra.Command, args []string) error {
 	}
 	wwlog.Debug("group: %v", time.Unix(int64(unixStat.Ctim.Sec), int64(unixStat.Ctim.Nsec)))
 	if syncuids && SyncUser {
-		err = container.SyncUids(containerName, true)
+		err = container.SyncUids(containerName, false)
 		if err != nil {
 			wwlog.Error("Error in user sync, fix error and run 'syncuser' manually, but trying to build container: %s", err)
 		}

internal/app/wwctl/container/syncuser/main.go

@@ -16,7 +16,7 @@ func CobraRunE(cmd *cobra.Command, args []string) error {
 	if !container.ValidName(containerName) {
 		return fmt.Errorf("%s is not a valid container", containerName)
 	}
-	err := container.SyncUids(containerName, !write)
+	err := container.SyncUids(containerName, write)
 	if err != nil {
 		wwlog.Error("Error in synchronize: %s", err)
 		os.Exit(1)

internal/pkg/api/container/container.go

@@ -231,12 +231,11 @@ func ContainerImport(cip *wwapiv1.ContainerImportParameter) (containerName strin
 		return
 	}
 
-	SyncUserShowOnly := !cip.SyncUser
-	err = container.SyncUids(cip.Name, SyncUserShowOnly)
-	if err != nil {
-		err = fmt.Errorf("error in user sync, fix error and run 'syncuser' manually: %s", err)
-		wwlog.Error(err.Error())
-		if cip.SyncUser {
+	if cip.SyncUser {
+		err = container.SyncUids(cip.Name, true)
+		if err != nil {
+			err = fmt.Errorf("error in user sync, fix error and run 'syncuser' manually: %s", err)
+			wwlog.Error(err.Error())
 			return
 		}
 	}

internal/pkg/container/syncuids.go

@@ -25,7 +25,7 @@ const groupPath = "/etc/group"
 // container. Files in the container are updated to match the new
 // numeric id assignments.
 //
-// If showOnly is true, the container is not actually updated, but
+// If write is false, the container is not actually updated, but
 // relevant log entries and output are generated.
 //
 // Any I/O errors are returned unmodified.
@@ -33,7 +33,8 @@ const groupPath = "/etc/group"
 // A conflict arises if the container has an entry with the same id as
 // an entry in the host and the host does not have an entry with the
 // same name. In this case, an error is returned.
-func SyncUids(containerName string, showOnly bool) error {
+func SyncUids(containerName string, write bool) error {
+	wwlog.Debug("SyncUids(containerName=%v, write=%v)", containerName, write)
 	containerPath := RootFsDir(containerName)
 	containerPasswdPath := path.Join(containerPath, passwdPath)
 	containerGroupPath := path.Join(containerPath, groupPath)
@@ -60,22 +61,17 @@ func SyncUids(containerName string, showOnly bool) error {
 		return err
 	}
 
+	if err := passwdSync.findUserFiles(containerPath); err != nil {
+		return err
+	}
+	if err := groupSync.findGroupFiles(containerPath); err != nil {
+		return err
+	}
+
 	passwdSync.log("passwd")
 	groupSync.log("group")
 
-	if showOnly {
-		if passwdSync.needsSync() || groupSync.needsSync() {
-			wwlog.Info("uid/gid not synced: run `wwctl container syncuser --write %s`", containerName)
-		} else {
-			wwlog.Info("uid/gid already synced")
-		}
-	} else {
-		if err := passwdSync.findUserFiles(containerPath); err != nil {
-			return err
-		}
-		if err := groupSync.findGroupFiles(containerPath); err != nil {
-			return err
-		}
+	if write {
 		if err := passwdSync.chownUserFiles(); err != nil {
 			return err
 		}
@@ -89,6 +85,12 @@ func SyncUids(containerName string, showOnly bool) error {
 			return err
 		}
 		wwlog.Info("uid/gid synced for container %s", containerName)
+	} else {
+		if passwdSync.needsSync() || groupSync.needsSync() {
+			wwlog.Info("uid/gid not synced: run `wwctl container syncuser --write %s`", containerName)
+		} else {
+			wwlog.Info("uid/gid already synced")
+		}
 	}
 
 	return nil
@@ -224,16 +226,34 @@ func (db syncDB) readFromContainer(fileName string) error { return db.read(fileN
 // If byGid is true, files with a matching gid are
 // recorded. Otherwise, files with a matching uid are recorded.
 func (db syncDB) findFiles(containerPath string, byGid bool) error {
-	for name, ids := range db {
-		if err := ids.findFiles(containerPath, byGid); err != nil {
-			return err
+	wwlog.Debug("findFiles(containerPath=%v, byGid=%v)", containerPath, byGid)
+	syncIDs := make(map[int]string)
+	for name, info := range db {
+		if info.inHost() && info.inContainer() && !info.match() {
+			wwlog.Debug("syncID[%v] = %v", info.ContainerID, name)
+			syncIDs[info.ContainerID] = name
 		}
-		if len(ids.ContainerFiles) > 0 {
-			wwlog.Debug("files for %s (%v -> %v, gid: %v): %v", name, ids.ContainerID, ids.HostID, byGid, ids.ContainerFiles)
-		}
-		db[name] = ids
 	}
-	return nil
+
+	return filepath.Walk(containerPath, func(filePath string, fileInfo fs.FileInfo, err error) error {
+		if stat, ok := fileInfo.Sys().(*syscall.Stat_t); ok {
+			var id int
+			if byGid {
+				id = int(stat.Gid)
+			} else {
+				id = int(stat.Uid)
+			}
+			if name, ok := syncIDs[id]; ok {
+				info := db[name]
+				wwlog.Debug("findFiles: %s: (%v -> %v, gid: %v)", filePath, info.ContainerID, info.HostID, byGid)
+				info.ContainerFiles = append(info.ContainerFiles, filePath)
+				db[name] = info
+			} else {
+				wwlog.Debug("findFiles: %s", filePath)
+			}
+		}
+		return nil
+	})
 }
 
 // findUserFiles is equivalent to findFiles(containerPath, false)
@@ -378,36 +398,6 @@ func (info *syncInfo) differ() bool {
 	return info.inContainer() && info.inHost() && info.HostID != info.ContainerID
 }
 
-// findFiles walks containerPath to find files owned by the name
-// represented by info and records them in info.ContainerFiles.
-//
-// If byGid is true, the file's gid is checked; otherwise, the file's
-// uid is checked.
-func (info *syncInfo) findFiles(containerPath string, byGid bool) error {
-	var containerFiles []string
-	if info.inHost() && !info.match() {
-		if err := filepath.Walk(containerPath, func(filePath string, fileInfo fs.FileInfo, err error) error {
-			wwlog.Debug("findFiles: %s", filePath)
-			if stat, ok := fileInfo.Sys().(*syscall.Stat_t); ok {
-				var id int
-				if byGid {
-					id = int(stat.Gid)
-				} else {
-					id = int(stat.Uid)
-				}
-				if id == info.ContainerID {
-					containerFiles = append(containerFiles, filePath)
-				}
-			}
-			return nil
-		}); err != nil {
-			return err
-		}
-	}
-	info.ContainerFiles = containerFiles
-	return nil
-}
-
 // chownFiles updates the files recorded in info.ContainerFiles to use
 // the numerical IDs from the host.
 //