From 5860334a8969bf2b22dcb7c043ef87d4326619de Mon Sep 17 00:00:00 2001
From: Jonathon Anderson <janderson@ciq.com>
Date: Fri, 6 Sep 2024 12:23:58 -0600
Subject: [PATCH] Improve error handling during container file copy removal

Based on feedback at #1365

Signed-off-by: Jonathon Anderson <janderson@ciq.com>
---
 internal/app/wwctl/container/exec/main.go | 44 +++++++++++++++--------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/internal/app/wwctl/container/exec/main.go b/internal/app/wwctl/container/exec/main.go
index 5aed57fa..336218a9 100644
--- a/internal/app/wwctl/container/exec/main.go
+++ b/internal/app/wwctl/container/exec/main.go
@@ -82,8 +82,10 @@ func runContainedCmd(cmd *cobra.Command, containerName string, args []string) (e
 	wwlog.Verbose("Running contained command: %s", childArgs)
 	retVal := childCommandFunc(cmd, childArgs)
 	for _, cpyFile := range filesToCpy {
-		if err := cpyFile.removeFromContainer(containerName); err != nil {
-			wwlog.Warn("couldn't remove file: %s", err)
+		if cpyFile.shouldRemoveFromContainer(containerName) {
+			if err := cpyFile.removeFromContainer(containerName); err != nil {
+				wwlog.Warn("couldn't remove file: %s", err)
+			}
 		}
 	}
 	return retVal
@@ -183,37 +185,49 @@ type copyFile struct {
 	modTime  time.Time
 }
 
+func (this *copyFile) containerDest(containerName string) string {
+	return path.Join(container.RootFsDir(containerName), this.fileName)
+}
+
 func (this *copyFile) copyToContainer(containerName string) error {
-	containerDest := path.Join(container.RootFsDir(containerName), this.fileName)
+	containerDest := this.containerDest(containerName)
 	if _, err := os.Stat(path.Dir(containerDest)); err != nil {
-		return fmt.Errorf("destination directory doesn't exist: %s", err)
+		return err
 	} else if _, err := os.Stat(containerDest); err == nil {
-		return fmt.Errorf("file already exists in container: %s", this.fileName)
+		return err
 	} else if _, err := os.Stat(this.src); err != nil {
-		return fmt.Errorf("source doesn't exist: %s", err)
+		return err
 	} else if err := util.CopyFile(this.src, containerDest); err != nil {
-		return fmt.Errorf("couldn't copy files into container: %w", err)
+		return err
+	} else if stat, err := os.Stat(containerDest); err != nil {
+		return err
 	} else {
-		// we can ignore error as the file was copied
-		stat, _ := os.Stat(containerDest)
 		this.modTime = stat.ModTime()
 		return nil
 	}
 }
 
-func (this *copyFile) removeFromContainer(containerName string) error {
-	containerDest := path.Join(container.RootFsDir(containerName), this.fileName)
+func (this *copyFile) shouldRemoveFromContainer(containerName string) bool {
+	containerDest := this.containerDest(containerName)
 	if this.modTime.IsZero() {
-		return fmt.Errorf("not previously copied: %s", this.fileName)
+		wwlog.Debug("file was not previously copied: %s", this.fileName)
+		return false
 	} else if destStat, err := os.Stat(containerDest); err != nil {
-		return fmt.Errorf("no longer present: %s", err)
+		wwlog.Verbose("file is no longer present: %s (%s)", this.fileName, err)
+		return false
 	} else if destStat.ModTime() == this.modTime {
-		return os.Remove(containerDest)
+		wwlog.Verbose("don't remove modified file:", this.fileName)
+		return false
 	} else {
-		return fmt.Errorf("modified since copy: %s", this.fileName)
+		return true
 	}
 }
 
+func (this *copyFile) removeFromContainer(containerName string) error {
+	containerDest := this.containerDest(containerName)
+	return os.Remove(containerDest)
+}
+
 /*
 Check the objects we want to copy in, instead of mounting
 */