From 8997c7456b5db048a90ad373d0330e84bf00913a Mon Sep 17 00:00:00 2001 From: Jonathon Anderson Date: Thu, 2 May 2024 13:28:57 -0600 Subject: [PATCH] Refactor syncuser to reduce walking the container fs - Closes #1209 Signed-off-by: Jonathon Anderson --- CHANGELOG.md | 5 + internal/app/wwctl/container/exec/main.go | 2 +- internal/app/wwctl/container/syncuser/main.go | 2 +- internal/pkg/api/container/container.go | 11 +-- internal/pkg/container/syncuids.go | 96 +++++++++---------- 5 files changed, 55 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e68703c78..edd90a883 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,11 +39,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Allow specification of the ssh-keys to be to be created. #1185 +### Changed + +- `wwctl container import` now only runs syncuser if explicitly requested. #1212 + ### Fixed - Fix nightly release build failure issue. #1195 - Reorder dnsmasq config to put iPXE last. #1146 - Update a reference to `--addprofile` to be `--profile`. #1085 +- Reduce the number of times syncuser walks the container file system. #1209 ## v4.5.1, 2024-04-30 diff --git a/internal/app/wwctl/container/exec/main.go b/internal/app/wwctl/container/exec/main.go index a007b5a43..44e11e9f5 100644 --- a/internal/app/wwctl/container/exec/main.go +++ b/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) } diff --git a/internal/app/wwctl/container/syncuser/main.go b/internal/app/wwctl/container/syncuser/main.go index 67e2d296b..10e516202 100644 --- a/internal/app/wwctl/container/syncuser/main.go +++ b/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) diff --git a/internal/pkg/api/container/container.go b/internal/pkg/api/container/container.go index 480bbd57e..50a97e4ca 100644 --- a/internal/pkg/api/container/container.go +++ b/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 } } diff --git a/internal/pkg/container/syncuids.go b/internal/pkg/container/syncuids.go index c9e9d701a..211567e78 100644 --- a/internal/pkg/container/syncuids.go +++ b/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 - } - if len(ids.ContainerFiles) > 0 { - wwlog.Debug("files for %s (%v -> %v, gid: %v): %v", name, ids.ContainerID, ids.HostID, byGid, ids.ContainerFiles) + 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 } - 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. //