From f84f4a9cce614e6db1fd71e8900ead9a78763dc7 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 1 Jul 2024 13:04:03 +0200 Subject: [PATCH 1/2] pkg/machine/e2e: use tmp file for connections On linux and macos the connections are stored under the home dir by default so it is not a problem there but on windows we first check the APPDATA env and use this dir as config storage. This has the problem that it is not cleaned up after each test as such connections might leak into the following test causing failues there. Fixes #22844 Signed-off-by: Paul Holzinger --- pkg/machine/e2e/machine_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/machine/e2e/machine_test.go b/pkg/machine/e2e/machine_test.go index 7ff0fee2cd..a8ca6f2cac 100644 --- a/pkg/machine/e2e/machine_test.go +++ b/pkg/machine/e2e/machine_test.go @@ -111,6 +111,9 @@ func setup() (string, *machineTestBuilder) { if err := os.Unsetenv("SSH_AUTH_SOCK"); err != nil { Fail("unable to unset SSH_AUTH_SOCK") } + if err := os.Setenv("PODMAN_CONNECTIONS_CONF", filepath.Join(homeDir, "connections.json")); err != nil { + Fail("failed to set PODMAN_CONNECTIONS_CONF") + } mb, err := newMB() if err != nil { Fail(fmt.Sprintf("failed to create machine test: %q", err)) From 3c0176b2d0dd3d4dbd3713c5e7fedf5501e304c5 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 1 Jul 2024 14:23:11 +0200 Subject: [PATCH 2/2] 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 #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 --- pkg/machine/e2e/basic_test.go | 11 ----------- pkg/machine/e2e/config_init_test.go | 24 +++++++++++++++++++++++- pkg/machine/e2e/info_test.go | 11 ----------- pkg/machine/e2e/init_test.go | 12 ------------ pkg/machine/e2e/init_windows_test.go | 11 ----------- pkg/machine/e2e/inspect_test.go | 11 ----------- pkg/machine/e2e/list_test.go | 11 ----------- pkg/machine/e2e/machine_test.go | 21 +++++++++++++-------- pkg/machine/e2e/os_test.go | 11 ----------- pkg/machine/e2e/proxy_test.go | 11 ----------- pkg/machine/e2e/reset_test.go | 11 ----------- pkg/machine/e2e/rm_test.go | 11 ----------- pkg/machine/e2e/set_test.go | 11 ----------- pkg/machine/e2e/ssh_test.go | 11 ----------- pkg/machine/e2e/start_test.go | 10 ---------- pkg/machine/e2e/stop_test.go | 11 ----------- 16 files changed, 36 insertions(+), 163 deletions(-) diff --git a/pkg/machine/e2e/basic_test.go b/pkg/machine/e2e/basic_test.go index 2ddee67547..79d97eaa12 100644 --- 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 index fb2ec97c90..9d33e2d3d2 100644 --- 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 index 481cdb96e0..7f3af2516e 100644 --- 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 index 30e7b74f56..63c0336e97 100644 --- 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 index 79e41b6c5e..238fe8467f 100644 --- 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 index a655d3a276..1c6224d793 100644 --- 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 index fe6c48b100..4b2d738869 100644 --- 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 index a8ca6f2cac..5a8d13ad9b 100644 --- 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 index 6205a0c19e..9fd907dde9 100644 --- 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 index a8879dbfbb..d539e95fb9 100644 --- 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 index 6394f4c249..f997bf864c 100644 --- 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 index f74c28e4db..210e6ed09f 100644 --- 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 index 04855c8cdf..0becff0083 100644 --- 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 index 36a48345c6..d360fd48d0 100644 --- 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 index 5b2a6342ef..efd65311db 100644 --- 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 index e18d570458..780858344d 100644 --- 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{}