Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/machine/e2e: fix broken cleanup #23154

Merged
merged 2 commits into from
Jul 2, 2024
Merged

pkg/machine/e2e: fix broken cleanup #23154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f84f4a9
pkg/machine/e2e: use tmp file for connections
Luap99 Jul 1, 2024
3c0176b
pkg/machine/e2e: fix broken cleanup
Luap99 Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions pkg/machine/e2e/basic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 23 additions & 1 deletion pkg/machine/e2e/config_init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Comment on lines +87 to +89
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of those tests! Would it make sense for those tests to set a SkipMachineCleanup or InvalidVM state flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not trusting test writers to remove this flag when it is no longer needed and it doesn't help if a bug causes the machine to be created all of the sudden then we leak machines.

IMO the reasonable fix to to make rm -f not error on non existing machine like our other commands do, i.e. podman rm -f blah

if strings.Contains(session.errorToString(), "VM does not exist") {
return
}
}
Expect(session).To(Exit(0))
})

i.cmd = cmd
return cmd
}
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 0 additions & 12 deletions pkg/machine/e2e/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/init_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/inspect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
24 changes: 16 additions & 8 deletions pkg/machine/e2e/machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of allowing CI tests to run locally, would it make sense to set this to a tempdir?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is already a tmpdir, see above. The entire home is overwritten but as windows is using APPDATA it does not help there. Of course I could overwrite APPDATA for windows but this seems simpler and more consistent to me

Fail("failed to set PODMAN_CONNECTIONS_CONF")
}
mb, err := newMB()
if err != nil {
Fail(fmt.Sprintf("failed to create machine test: %q", err))
Expand All @@ -128,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))
}
Expand All @@ -150,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":
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/os_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/reset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/rm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/ssh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
10 changes: 0 additions & 10 deletions pkg/machine/e2e/start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 0 additions & 11 deletions pkg/machine/e2e/stop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down