Merge pull request #28228 from mtaufen/e2e-killcmd-cleanup

Automatic merge from submit-queue

Use slices of items to clean up after tests

Fixes #27582.

We used to maintain a pointer variable for each process to kill after the
tests finish. @lavalamp suggested using a slice instead, which is a much
cleaner solution. This implements @lavalamp's suggestion and also extends
the idea to tracking directories that need to be removed after the tests finish.

This also means that we should no longer check for nil `killCmd`s inside
`func (k *killCmd) Kill() error {...}` (see #27582 and #27589). If a nil
`killCmd` makes it in there, something is bad elsewhere and we want to see
the nil pointer exception immediately.

Mentioning @timstclair and @euank wrt the original issue/PR.

test/e2e_node/e2e_service.go

@@ -38,10 +38,10 @@ var serverStartTimeout = flag.Duration("server-start-timeout", time.Second*120,
 var reportDir = flag.String("report-dir", "", "Path to the directory where the JUnit XML reports should be saved. Default is empty, which doesn't generate these reports.")
 
 type e2eService struct {
-	etcdCmd             *killCmd
+	killCmds []*killCmd
+	rmDirs   []string
+
 	etcdDataDir         string
-	apiServerCmd        *killCmd
-	kubeletCmd          *killCmd
 	kubeletStaticPodDir string
 	nodeName            string
 	logFiles            map[string]logFileData
@@ -79,19 +79,21 @@ func (es *e2eService) start() error {
 	if err != nil {
 		return err
 	}
-	es.etcdCmd = cmd
+	es.killCmds = append(es.killCmds, cmd)
+	es.rmDirs = append(es.rmDirs, es.etcdDataDir)
 
 	cmd, err = es.startApiServer()
 	if err != nil {
 		return err
 	}
-	es.apiServerCmd = cmd
+	es.killCmds = append(es.killCmds, cmd)
 
 	cmd, err = es.startKubeletServer()
 	if err != nil {
 		return err
 	}
-	es.kubeletCmd = cmd
+	es.killCmds = append(es.killCmds, cmd)
+	es.rmDirs = append(es.rmDirs, es.kubeletStaticPodDir)
 
 	return nil
 }
@@ -152,25 +154,15 @@ func isJournaldAvailable() bool {
 }
 
 func (es *e2eService) stop() {
-	if err := es.stopService(es.kubeletCmd); err != nil {
+	for _, k := range es.killCmds {
-		glog.Errorf("Failed to stop kubelet: %v", err)
+		if err := k.Kill(); err != nil {
-	}
+			glog.Errorf("Failed to stop %v: %v", k.name, err)
-	if es.kubeletStaticPodDir != "" {
-		err := os.RemoveAll(es.kubeletStaticPodDir)
-		if err != nil {
-			glog.Errorf("Failed to delete kubelet static pod directory %s.\n%v", es.kubeletStaticPodDir, err)
 		}
 	}
-	if err := es.stopService(es.apiServerCmd); err != nil {
+	for _, d := range es.rmDirs {
-		glog.Errorf("Failed to stop kube-apiserver: %v", err)
+		err := os.RemoveAll(d)
-	}
-	if err := es.stopService(es.etcdCmd); err != nil {
-		glog.Errorf("Failed to stop etcd: %v", err)
-	}
-	if es.etcdDataDir != "" {
-		err := os.RemoveAll(es.etcdDataDir)
 		if err != nil {
-			glog.Errorf("Failed to delete etcd data directory %s.\n%v", es.etcdDataDir, err)
+			glog.Errorf("Failed to delete directory %s.\n%v", d, err)
 		}
 	}
 }
@@ -306,10 +298,6 @@ func (es *e2eService) startServer(cmd *healthCheckCommand) error {
 	return fmt.Errorf("Timeout waiting for service %s", cmd)
 }
 
-func (es *e2eService) stopService(cmd *killCmd) error {
-	return cmd.Kill()
-}
-
 // killCmd is a struct to kill a given cmd. The cmd member specifies a command
 // to find the pid of and attempt to kill.
 // If the override field is set, that will be used instead to kill the command.
@@ -321,19 +309,18 @@ type killCmd struct {
 }
 
 func (k *killCmd) Kill() error {
-	if k == nil {
+	name := k.name
-		glog.V(2).Infof("The killCmd is nil, nothing will be killed")
+	cmd := k.cmd
-		return nil
-	}
 
 	if k.override != nil {
 		return k.override.Run()
 	}
 
-	name := k.name
+	if cmd == nil {
-	cmd := k.cmd
+		return fmt.Errorf("Could not kill %s because both `override` and `cmd` are nil", name)
+	}
 
-	if cmd == nil || cmd.Process == nil {
+	if cmd.Process == nil {
 		glog.V(2).Infof("%s not running", name)
 		return nil
 	}