pkg/machine/e2e: fix broken cleanup

Currently all podman machine rm errors in AfterEach were ignored. This means some leaked and caused issues later on, see containers#22844. To fix it first rework the logic to only remove machines when needed at the place were they are created using DeferCleanup(), however DeferCleanup() does not work well together with AfterEach() as it always run AfterEach() before DeferCleanup(). As AfterEach() deletes the dir the podman machine rm call can not be done afterwards. As such migrate all cleanup to use DeferCleanup() and while I have to touch this fix the code to remove the per file duplciation and define the setup/cleanup once in the global scope. Signed-off-by: Paul Holzinger <[email protected]>
Luap99 · Jul 1, 2024 · 3c0176b · 3c0176b
1 parent f84f4a9
commit 3c0176b
Show file tree

Hide file tree

Showing 16 changed files with 36 additions and 163 deletions.
diff --git a/pkg/machine/e2e/basic_test.go b/pkg/machine/e2e/basic_test.go
@@ -18,17 +18,6 @@ import (
 )
 
 var _ = Describe("run basic podman commands", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("Basic ops", func() {
 		// golangci-lint has trouble with actually skipping tests marked Skip

diff --git a/pkg/machine/e2e/config_init_test.go b/pkg/machine/e2e/config_init_test.go
@@ -2,6 +2,11 @@ package e2e_test
 
 import (
 	"strconv"
+	"strings"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gexec"
 )
 
 type initMachine struct {
@@ -71,7 +76,24 @@ func (i *initMachine) buildCmd(m *machineTestBuilder) []string {
 	if i.userModeNetworking {
 		cmd = append(cmd, "--user-mode-networking")
 	}
-	cmd = append(cmd, m.name)
+	name := m.name
+	cmd = append(cmd, name)
+
+	// when we create a new VM remove it again as cleanup
+	DeferCleanup(func() {
+		r := new(rmMachine)
+		session, err := m.setName(name).setCmd(r.withForce()).run()
+		Expect(err).ToNot(HaveOccurred(), "error occurred rm'ing machine")
+		// Some test create a invalid VM so the VM does not exists in this case we have to ignore the error.
+		// It would be much better if rm -f would behave like other commands and ignore not exists errors.
+		if session.ExitCode() == 125 {
+			if strings.Contains(session.errorToString(), "VM does not exist") {
+				return
+			}
+		}
+		Expect(session).To(Exit(0))
+	})
+
 	i.cmd = cmd
 	return cmd
 }

diff --git a/pkg/machine/e2e/info_test.go b/pkg/machine/e2e/info_test.go
@@ -11,17 +11,6 @@ import (
 )
 
 var _ = Describe("podman machine info", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("machine info", func() {
 		info := new(infoMachine)

diff --git a/pkg/machine/e2e/init_test.go b/pkg/machine/e2e/init_test.go
@@ -20,18 +20,6 @@ import (
 )
 
 var _ = Describe("podman machine init", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
-
 	cpus := runtime.NumCPU() / 2
 	if cpus == 0 {
 		cpus = 1

diff --git a/pkg/machine/e2e/init_windows_test.go b/pkg/machine/e2e/init_windows_test.go
@@ -15,17 +15,6 @@ import (
 )
 
 var _ = Describe("podman machine init - windows only", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("init with user mode networking", func() {
 		if testProvider.VMType() != define.WSLVirt {

diff --git a/pkg/machine/e2e/inspect_test.go b/pkg/machine/e2e/inspect_test.go
@@ -11,17 +11,6 @@ import (
 )
 
 var _ = Describe("podman inspect stop", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("inspect bad name", func() {
 		i := inspectMachine{}

diff --git a/pkg/machine/e2e/list_test.go b/pkg/machine/e2e/list_test.go
@@ -14,17 +14,6 @@ import (
 )
 
 var _ = Describe("podman machine list", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("list machine", func() {
 		list := new(listMachine)

diff --git a/pkg/machine/e2e/machine_test.go b/pkg/machine/e2e/machine_test.go
@@ -131,14 +131,7 @@ func setup() (string, *machineTestBuilder) {
 	return homeDir, mb
 }
 
-func teardown(origHomeDir string, testDir string, mb *machineTestBuilder) {
-	r := new(rmMachine)
-	for _, name := range mb.names {
-		if _, err := mb.setName(name).setCmd(r.withForce()).run(); err != nil {
-			GinkgoWriter.Printf("error occurred rm'ing machine: %q\n", err)
-		}
-	}
-
+func teardown(origHomeDir string, testDir string) {
 	if err := utils.GuardedRemoveAll(testDir); err != nil {
 		Fail(fmt.Sprintf("failed to remove test dir: %q", err))
 	}
@@ -153,6 +146,18 @@ func teardown(origHomeDir string, testDir string, mb *machineTestBuilder) {
 	}
 }
 
+var (
+	mb      *machineTestBuilder
+	testDir string
+)
+
+var _ = BeforeEach(func() {
+	testDir, mb = setup()
+	DeferCleanup(func() {
+		teardown(originalHomeDir, testDir)
+	})
+})
+
 func setTmpDir(value string) (string, error) {
 	switch {
 	case runtime.GOOS != "darwin":

diff --git a/pkg/machine/e2e/os_test.go b/pkg/machine/e2e/os_test.go
@@ -7,17 +7,6 @@ package e2e_test
 // )
 
 // var _ = Describe("podman machine os apply", func() {
-// 	var (
-// 		mb      *machineTestBuilder
-// 		testDir string
-// 	)
-
-// 	BeforeEach(func() {
-// 		testDir, mb = setup()
-// 	})
-// 	AfterEach(func() {
-// 		teardown(originalHomeDir, testDir, mb)
-// 	})
 
 // 	It("apply machine", func() {
 // 		i := new(initMachine)

diff --git a/pkg/machine/e2e/proxy_test.go b/pkg/machine/e2e/proxy_test.go
@@ -11,17 +11,6 @@ import (
 )
 
 var _ = Describe("podman machine proxy settings propagation", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("ssh to running machine and check proxy settings", func() {
 		defer func() {

diff --git a/pkg/machine/e2e/reset_test.go b/pkg/machine/e2e/reset_test.go
@@ -7,17 +7,6 @@ import (
 )
 
 var _ = Describe("podman machine reset", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("starting from scratch should not error", func() {
 		i := resetMachine{}

diff --git a/pkg/machine/e2e/rm_test.go b/pkg/machine/e2e/rm_test.go
@@ -12,17 +12,6 @@ import (
 )
 
 var _ = Describe("podman machine rm", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("bad init name", func() {
 		i := rmMachine{}

diff --git a/pkg/machine/e2e/set_test.go b/pkg/machine/e2e/set_test.go
@@ -13,17 +13,6 @@ import (
 )
 
 var _ = Describe("podman machine set", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("set machine cpus, disk, memory", func() {
 		skipIfWSL("WSL cannot change set properties of disk, processor, or memory")

diff --git a/pkg/machine/e2e/ssh_test.go b/pkg/machine/e2e/ssh_test.go
@@ -8,17 +8,6 @@ import (
 )
 
 var _ = Describe("podman machine ssh", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("bad machine name", func() {
 		name := randomString()

diff --git a/pkg/machine/e2e/start_test.go b/pkg/machine/e2e/start_test.go
@@ -14,16 +14,6 @@ import (
 )
 
 var _ = Describe("podman machine start", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("start simple machine", func() {
 		i := new(initMachine)

diff --git a/pkg/machine/e2e/stop_test.go b/pkg/machine/e2e/stop_test.go
@@ -10,17 +10,6 @@ import (
 )
 
 var _ = Describe("podman machine stop", func() {
-	var (
-		mb      *machineTestBuilder
-		testDir string
-	)
-
-	BeforeEach(func() {
-		testDir, mb = setup()
-	})
-	AfterEach(func() {
-		teardown(originalHomeDir, testDir, mb)
-	})
 
 	It("stop bad name", func() {
 		i := stopMachine{}