From 5569817c268e9134095fa381bdc85ceada07fd39 Mon Sep 17 00:00:00 2001
From: Priya Wadhwa <priyawadhwa@google.com>
Date: Tue, 10 Nov 2020 10:58:31 -0800
Subject: [PATCH] Address code review comments

---
 cmd/minikube/cmd/stop.go                   |  5 +-
 pkg/minikube/schedule/daemonize_unix.go    | 55 +++++++++++++---------
 pkg/minikube/schedule/daemonize_windows.go |  8 ++--
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/cmd/minikube/cmd/stop.go b/cmd/minikube/cmd/stop.go
index bf7bd2b7b3ed..a677051b66d4 100644
--- a/cmd/minikube/cmd/stop.go
+++ b/cmd/minikube/cmd/stop.go
@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"log"
 	"os"
 	"runtime"
 	"time"
@@ -97,9 +96,7 @@ func runStop(cmd *cobra.Command, args []string) {
 	}
 
 	// Kill any existing scheduled stops
-	if err := schedule.KillExisting(profilesToStop); err != nil {
-		log.Printf("error killing existing scheduled stops: %v", err)
-	}
+	schedule.KillExisting(profilesToStop)
 
 	if scheduledStopDuration != 0 {
 		if runtime.GOOS == "windows" {
diff --git a/pkg/minikube/schedule/daemonize_unix.go b/pkg/minikube/schedule/daemonize_unix.go
index 4f52a7621d6c..022b6de8a6d9 100644
--- a/pkg/minikube/schedule/daemonize_unix.go
+++ b/pkg/minikube/schedule/daemonize_unix.go
@@ -31,30 +31,41 @@ import (
 	"k8s.io/minikube/pkg/minikube/localpath"
 )
 
-// KillExisting kills existing scheduled stops
-func KillExisting(profiles []string) error {
+// KillExisting kills existing scheduled stops by looking up the PID
+// of the scheduled stop from the PID file saved for the profile and killing the process
+func KillExisting(profiles []string) {
 	for _, profile := range profiles {
-		file := localpath.PID(profile)
-		f, err := ioutil.ReadFile(file)
-		if os.IsNotExist(err) {
-			return nil
+		if err := killPIDForProfile(profile); err != nil {
+			klog.Errorf("error killng PID for profile %s: %v", profile, err)
 		}
-		defer os.Remove(file)
-		if err != nil {
-			return errors.Wrapf(err, "reading %s", file)
-		}
-		pid, err := strconv.Atoi(string(f))
-		if err != nil {
-			return errors.Wrapf(err, "converting %v to int", string(f))
-		}
-		p, err := os.FindProcess(pid)
-		if err != nil {
-			return errors.Wrap(err, "finding process")
-		}
-		klog.Infof("killing process %v as it is an old scheduled stop", pid)
-		if err := p.Kill(); err != nil {
-			return errors.Wrapf(err, "killing %v", pid)
+	}
+}
+
+func killPIDForProfile(profile string) error {
+	file := localpath.PID(profile)
+	f, err := ioutil.ReadFile(file)
+	if os.IsNotExist(err) {
+		return nil
+	}
+	defer func() {
+		if err := os.Remove(file); err != nil {
+			klog.Errorf("error deleting %s: %v, you may have to delete in manually", file, err)
 		}
+	}()
+	if err != nil {
+		return errors.Wrapf(err, "reading %s", file)
+	}
+	pid, err := strconv.Atoi(string(f))
+	if err != nil {
+		return errors.Wrapf(err, "converting %s to int", f)
+	}
+	p, err := os.FindProcess(pid)
+	if err != nil {
+		return errors.Wrap(err, "finding process")
+	}
+	klog.Infof("killing process %v as it is an old scheduled stop", pid)
+	if err := p.Kill(); err != nil {
+		return errors.Wrapf(err, "killing %v", pid)
 	}
 	return nil
 }
@@ -72,7 +83,7 @@ func daemonize(profiles []string, duration time.Duration) error {
 func savePIDs(pid int, profiles []string) error {
 	for _, p := range profiles {
 		file := localpath.PID(p)
-		if err := ioutil.WriteFile(file, []byte(fmt.Sprintf("%v", pid)), 0644); err != nil {
+		if err := ioutil.WriteFile(file, []byte(fmt.Sprintf("%v", pid)), 0600); err != nil {
 			return err
 		}
 	}
diff --git a/pkg/minikube/schedule/daemonize_windows.go b/pkg/minikube/schedule/daemonize_windows.go
index 7744c9d26fdf..ea3c7fa23856 100644
--- a/pkg/minikube/schedule/daemonize_windows.go
+++ b/pkg/minikube/schedule/daemonize_windows.go
@@ -21,11 +21,13 @@ package schedule
 import (
 	"fmt"
 	"time"
+
+	"k8s.io/klog/v2"
 )
 
-// KillExisting kills existing scheduled stops
-func KillExisting(profiles []string) error {
-	return fmt.Errorf("not yet implemented for windows")
+// KillExisting will kill existing scheduled stops
+func KillExisting(profiles []string) {
+	klog.Errorf("not yet implemented for windows")
 }
 
 func daemonize(profiles []string, duration time.Duration) error {