ephemeral-disk-utils: simplify and rename RemoveFile

We would like RemoveFile to remove a file that may already exists from a
former attemt to reconcile a resource. We do not want it to do anything
more. E.g. if it finds a whole directory tree there, it is probably a
programmatic error that should be reported as such.

This commit renames the function to RemoveFilesIfExist, to make the
intended functionality clear, and modify the implemenetation to be more
careful and return an error if the on-disk situation is unexpected.

It also extends the signature to allow this type of optional removal of
multiple files.

Signed-off-by: Dan Kenigsberg <[email protected]>

pkg/cloud-init/cloud-init.go

@@ -521,10 +521,10 @@ func GenerateLocalData(vmiName string, namespace string, data *CloudInitData) er
 		networkData = []byte(data.NetworkData)
 	}
 
-	diskutils.RemoveFile(userFile)
-	diskutils.RemoveFile(metaFile)
-	diskutils.RemoveFile(networkFile)
-	diskutils.RemoveFile(isoStaging)
+	diskutils.RemoveFilesIfExist(userFile)
+	diskutils.RemoveFilesIfExist(metaFile)
+	diskutils.RemoveFilesIfExist(networkFile)
+	diskutils.RemoveFilesIfExist(isoStaging)
 
 	err = ioutil.WriteFile(userFile, userData, 0644)
 	if err != nil {

pkg/ephemeral-disk-utils/BUILD.bazel

@@ -5,10 +5,7 @@ go_library(
     srcs = ["utils.go"],
     importpath = "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils",
     visibility = ["//visibility:public"],
-    deps = [
-        "//staging/src/kubevirt.io/client-go/api/v1:go_default_library",
-        "//staging/src/kubevirt.io/client-go/log:go_default_library",
-    ],
+    deps = ["//staging/src/kubevirt.io/client-go/api/v1:go_default_library"],
 )
 
 go_test(

pkg/ephemeral-disk-utils/utils.go

@@ -28,7 +28,6 @@ import (
 	"strings"
 
 	v1 "kubevirt.io/client-go/api/v1"
-	"kubevirt.io/client-go/log"
 )
 
 // TODO this should be part of structs, instead of a global
@@ -66,13 +65,13 @@ func (om *OwnershipManager) SetFileOwnership(file string) error {
 	return os.Chown(file, uid, gid)
 }
 
-func RemoveFile(path string) error {
-	err := os.RemoveAll(path)
-	if err != nil && os.IsNotExist(err) {
-		return nil
-	} else if err != nil {
-		log.Log.Reason(err).Errorf("failed to remove %s", path)
-		return err
+func RemoveFilesIfExist(paths ...string) error {
+	var err error
+	for _, path := range paths {
+		err = os.Remove(path)
+		if err != nil && !os.IsNotExist(err) {
+			return err
+		}
 	}
 	return nil
 }

pkg/ephemeral-disk-utils/utils_test.go

@@ -34,18 +34,27 @@ var _ = Describe("FileExists", func() {
 		Expect(FileExists("no one would ever have this file")).To(BeFalse())
 	})
 })
-var _ = Describe("RemoveFile", func() {
+var _ = Describe("RemoveFilesIfExist", func() {
 	It("silently ignores non-existing file", func() {
-		// Ingnoring missing files is typically a sloppy behavior. This test
-		// documents it, not aproves of its usage.
-		Expect(RemoveFile("no one would ever have this file")).To(BeNil())
+		Expect(RemoveFilesIfExist("no one would ever have this file")).To(BeNil())
 	})
 	It("removes a file", func() {
 		tmpfile, err := ioutil.TempFile("", "file_to_remove")
 		Expect(err).To(BeNil())
 		defer tmpfile.Close()
 		Expect(FileExists(tmpfile.Name())).To(BeTrue())
-		Expect(RemoveFile(tmpfile.Name())).To(BeNil())
+		Expect(RemoveFilesIfExist(tmpfile.Name())).To(BeNil())
 		Expect(FileExists(tmpfile.Name())).To(BeFalse())
 	})
+	It("removes multiple files", func() {
+		tmpfile1, err := ioutil.TempFile("", "file_to_remove1")
+		Expect(err).To(BeNil())
+		defer tmpfile1.Close()
+		tmpfile2, err := ioutil.TempFile("", "file_to_remove2")
+		Expect(err).To(BeNil())
+		defer tmpfile2.Close()
+		Expect(RemoveFilesIfExist(tmpfile1.Name(), tmpfile2.Name())).To(BeNil())
+		Expect(FileExists(tmpfile1.Name())).To(BeFalse())
+		Expect(FileExists(tmpfile2.Name())).To(BeFalse())
+	})
 })

pkg/virt-handler/vm.go

@@ -1259,7 +1259,7 @@ func gracefulShutdownTriggerFromNamespaceName(baseDir string, namespace string,
 // VMIs running with the old graceful shutdown trigger logic
 func vmGracefulShutdownTriggerClear(baseDir string, vmi *v1.VirtualMachineInstance) error {
 	triggerFile := gracefulShutdownTriggerFromNamespaceName(baseDir, vmi.Namespace, vmi.Name)
-	return diskutils.RemoveFile(triggerFile)
+	return diskutils.RemoveFilesIfExist(triggerFile)
 }
 
 // Legacy, remove once we're certain we are no longer supporting

pkg/watchdog/watchdog.go

@@ -65,7 +65,7 @@ func WatchdogFileRemove(baseDir string, vmi *v1.VirtualMachineInstance) error {
 	file := WatchdogFileFromNamespaceName(baseDir, namespace, domain)
 
 	log.Log.V(3).Infof("Remove watchdog file %s", file)
-	return diskutils.RemoveFile(file)
+	return diskutils.RemoveFilesIfExist(file)
 }
 
 func WatchdogFileUpdate(watchdogFile string, uid string) error {