From 61e0b64b91d5b2709fd5f1b4b10525b2b9d2c545 Mon Sep 17 00:00:00 2001 From: Ashley Cui Date: Thu, 5 Oct 2023 02:44:27 -0400 Subject: [PATCH] Machine: Teardown on init failure If init fails, or if a SIGINT is sent during init, podman machine should remove all files and configs created during the init. This includes config jsons, image files, ssh id's, and system connections. On Windows, the VM instances are also unregistered. Signed-off-by: Ashley Cui --- pkg/machine/applehv/machine.go | 37 +++++++++++++++--- pkg/machine/cleanup.go | 68 ++++++++++++++++++++++++++++++++++ pkg/machine/e2e/init_test.go | 56 +++++++++++++++++++++++++++- pkg/machine/e2e/rm_test.go | 2 - pkg/machine/hyperv/machine.go | 42 +++++++++++++++++++-- pkg/machine/qemu/machine.go | 35 ++++++++++++++--- pkg/machine/wsl/machine.go | 60 +++++++++++++++++++++++++++--- 7 files changed, 276 insertions(+), 24 deletions(-) create mode 100644 pkg/machine/cleanup.go diff --git a/pkg/machine/applehv/machine.go b/pkg/machine/applehv/machine.go index da81d79c72..1868a8537a 100644 --- a/pkg/machine/applehv/machine.go +++ b/pkg/machine/applehv/machine.go @@ -188,7 +188,15 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { var ( key string virtiofsMnts []machine.VirtIoFs + err error ) + + // cleanup half-baked files if init fails at any point + callbackFuncs := machine.InitCleanup() + defer callbackFuncs.CleanIfErr(&err) + go callbackFuncs.CleanOnSignal() + + callbackFuncs.Add(m.ConfigPath.Delete) dataDir, err := machine.GetDataDir(machine.AppleHvVirt) if err != nil { return false, err @@ -207,6 +215,7 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { if err != nil { return false, err } + callbackFuncs.Add(imagePath.Delete) // Set the values for imagePath and strm m.ImagePath = *imagePath @@ -216,6 +225,8 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { if err != nil { return false, err } + callbackFuncs.Add(logPath.Delete) + m.LogPath = *logPath runtimeDir, err := m.getRuntimeDir() if err != nil { @@ -228,11 +239,11 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { } m.ReadySocket = *readySocket - if err := m.setGVProxyInfo(runtimeDir); err != nil { + if err = m.setGVProxyInfo(runtimeDir); err != nil { return false, err } - if err := m.setVfkitInfo(cfg, m.ReadySocket); err != nil { + if err = m.setVfkitInfo(cfg, m.ReadySocket); err != nil { return false, err } @@ -248,7 +259,7 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { } m.Port = sshPort - if err := m.addMountsToVM(opts, &virtiofsMnts); err != nil { + if err = m.addMountsToVM(opts, &virtiofsMnts); err != nil { return false, err } @@ -263,22 +274,23 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { if err != nil { return false, err } + callbackFuncs.Add(m.removeSystemConnections) logrus.Debugf("resizing disk to %d GiB", opts.DiskSize) - if err := m.resizeDisk(strongunits.GiB(opts.DiskSize)); err != nil { + if err = m.resizeDisk(strongunits.GiB(opts.DiskSize)); err != nil { return false, err } - if err := m.writeConfig(); err != nil { + if err = m.writeConfig(); err != nil { return false, err } if len(opts.IgnitionPath) < 1 { - var err error key, err = machine.CreateSSHKeys(m.IdentityPath) if err != nil { return false, err } + callbackFuncs.Add(m.removeSSHKeys) } if len(opts.IgnitionPath) > 0 { @@ -290,9 +302,22 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) { } // TODO Ignition stuff goes here err = m.writeIgnitionConfigFile(opts, key, &virtiofsMnts) + callbackFuncs.Add(m.IgnitionFile.Delete) + return err == nil, err } +func (m *MacMachine) removeSSHKeys() error { + if err := os.Remove(fmt.Sprintf("%s.pub", m.IdentityPath)); err != nil { + logrus.Error(err) + } + return os.Remove(m.IdentityPath) +} + +func (m *MacMachine) removeSystemConnections() error { + return machine.RemoveConnections(m.Name, fmt.Sprintf("%s-root", m.Name)) +} + func (m *MacMachine) Inspect() (*machine.InspectInfo, error) { vmState, err := m.Vfkit.state() if err != nil { diff --git a/pkg/machine/cleanup.go b/pkg/machine/cleanup.go new file mode 100644 index 0000000000..ff23846cf7 --- /dev/null +++ b/pkg/machine/cleanup.go @@ -0,0 +1,68 @@ +package machine + +import ( + "os" + "os/signal" + "sync" + "syscall" + + "github.com/sirupsen/logrus" +) + +type CleanupCallback struct { + Funcs []func() error + mu sync.Mutex +} + +func (c *CleanupCallback) CleanIfErr(err *error) { + // Do not remove created files if the init is successful + if *err == nil { + return + } + c.clean() +} + +func (c *CleanupCallback) CleanOnSignal() { + ch := make(chan os.Signal, 1) + signal.Notify(ch, os.Interrupt, syscall.SIGTERM) + + _, ok := <-ch + if !ok { + return + } + + c.clean() + os.Exit(1) +} + +func (c *CleanupCallback) clean() { + c.mu.Lock() + // Claim exclusive usage by copy and resetting to nil + funcs := c.Funcs + c.Funcs = nil + c.mu.Unlock() + + // Already claimed or none set + if funcs == nil { + return + } + + // Cleanup functions can now exclusively be run + for _, cleanfunc := range funcs { + if err := cleanfunc(); err != nil { + logrus.Error(err) + } + } +} + +func InitCleanup() CleanupCallback { + return CleanupCallback{ + Funcs: []func() error{}, + } +} + +func (c *CleanupCallback) Add(anotherfunc func() error) { + c.mu.Lock() + c.Funcs = append(c.Funcs, anotherfunc) + c.mu.Unlock() +} diff --git a/pkg/machine/e2e/init_test.go b/pkg/machine/e2e/init_test.go index 4ea370dff0..948d67a836 100644 --- a/pkg/machine/e2e/init_test.go +++ b/pkg/machine/e2e/init_test.go @@ -232,7 +232,7 @@ var _ = Describe("podman machine init", func() { Expect(output).To(Equal("/run/podman/podman.sock")) }) - It("init with user mode networking ", func() { + It("init with user mode networking", func() { if testProvider.VMType() != machine.WSLVirt { Skip("test is only supported by WSL") } @@ -249,4 +249,58 @@ var _ = Describe("podman machine init", func() { Expect(inspectSession).To(Exit(0)) Expect(inspectSession.outputToString()).To(Equal("true")) }) + + It("init should cleanup on failure", func() { + i := new(initMachine) + name := randomString() + session, err := mb.setName(name).setCmd(i.withImagePath(mb.imagePath)).run() + + Expect(err).ToNot(HaveOccurred()) + Expect(session).To(Exit(0)) + + inspect := new(inspectMachine) + inspect = inspect.withFormat("{{.ConfigPath.Path}}") + inspectSession, err := mb.setCmd(inspect).run() + Expect(err).ToNot(HaveOccurred()) + cfgpth := inspectSession.outputToString() + + inspect = inspect.withFormat("{{.Image.IgnitionFile.Path}}") + inspectSession, err = mb.setCmd(inspect).run() + Expect(err).ToNot(HaveOccurred()) + ign := inspectSession.outputToString() + + inspect = inspect.withFormat("{{.Image.ImagePath.Path}}") + inspectSession, err = mb.setCmd(inspect).run() + Expect(err).ToNot(HaveOccurred()) + img := inspectSession.outputToString() + + rm := rmMachine{} + removeSession, err := mb.setCmd(rm.withForce().withSaveKeys()).run() + Expect(err).ToNot(HaveOccurred()) + Expect(removeSession).To(Exit(0)) + + // Inspecting a non-existent machine should fail + // which means it is gone + _, ec, err := mb.toQemuInspectInfo() + Expect(err).ToNot(HaveOccurred()) + Expect(ec).To(Equal(125)) + + // Clashing keys - init fails + i = new(initMachine) + session, err = mb.setName(name).setCmd(i.withImagePath(mb.imagePath)).run() + Expect(err).ToNot(HaveOccurred()) + Expect(session).To(Exit(125)) + + // ensure files created by init are cleaned up on init failure + _, err = os.Stat(img) + Expect(err).To(HaveOccurred()) + _, err = os.Stat(cfgpth) + Expect(err).To(HaveOccurred()) + + // WSL does not use ignition + if testProvider.VMType() != machine.WSLVirt { + _, err = os.Stat(ign) + Expect(err).To(HaveOccurred()) + } + }) }) diff --git a/pkg/machine/e2e/rm_test.go b/pkg/machine/e2e/rm_test.go index 8439632f47..f074961c14 100644 --- a/pkg/machine/e2e/rm_test.go +++ b/pkg/machine/e2e/rm_test.go @@ -74,7 +74,6 @@ var _ = Describe("podman machine rm", func() { }) It("machine rm --save-keys, --save-ignition, --save-image", func() { - i := new(initMachine) session, err := mb.setCmd(i.withImagePath(mb.imagePath)).run() Expect(err).ToNot(HaveOccurred()) @@ -120,6 +119,5 @@ var _ = Describe("podman machine rm", func() { } _, err = os.Stat(img) Expect(err).ToNot(HaveOccurred()) - }) }) diff --git a/pkg/machine/hyperv/machine.go b/pkg/machine/hyperv/machine.go index b6f091cdfc..0e06a2d813 100644 --- a/pkg/machine/hyperv/machine.go +++ b/pkg/machine/hyperv/machine.go @@ -200,11 +200,25 @@ func (m *HyperVMachine) readAndSplitIgnition() error { func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) { var ( key string + err error ) - if err := m.addNetworkAndReadySocketsToRegistry(); err != nil { + // cleanup half-baked files if init fails at any point + callbackFuncs := machine.InitCleanup() + defer callbackFuncs.CleanIfErr(&err) + go callbackFuncs.CleanOnSignal() + + callbackFuncs.Add(m.ImagePath.Delete) + callbackFuncs.Add(m.ConfigPath.Delete) + callbackFuncs.Add(m.unregisterMachine) + + if err = m.addNetworkAndReadySocketsToRegistry(); err != nil { return false, err } + callbackFuncs.Add(func() error { + m.removeNetworkAndReadySocketsFromRegistry() + return nil + }) m.IdentityPath = util.GetIdentityPath(m.Name) @@ -230,13 +244,14 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) { if err != nil { return false, err } + callbackFuncs.Add(m.removeSystemConnections) if len(opts.IgnitionPath) < 1 { - var err error key, err = machine.CreateSSHKeys(m.IdentityPath) if err != nil { return false, err } + callbackFuncs.Add(m.removeSSHKeys) } m.ResourceConfig = machine.ResourceConfig{ @@ -255,6 +270,7 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) { } return false, os.WriteFile(m.IgnitionFile.GetPath(), inputIgnition, 0644) } + callbackFuncs.Add(m.IgnitionFile.Delete) if err := m.writeConfig(); err != nil { return false, err @@ -265,7 +281,7 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) { return false, err } - if err := m.resizeDisk(strongunits.GiB(opts.DiskSize)); err != nil { + if err = m.resizeDisk(strongunits.GiB(opts.DiskSize)); err != nil { return false, err } // The ignition file has been written. We now need to @@ -274,6 +290,26 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) { return err == nil, err } +func (m *HyperVMachine) unregisterMachine() error { + vmm := hypervctl.NewVirtualMachineManager() + vm, err := vmm.GetMachine(m.Name) + if err != nil { + logrus.Error(err) + } + return vm.Remove("") +} + +func (m *HyperVMachine) removeSSHKeys() error { + if err := os.Remove(fmt.Sprintf("%s.pub", m.IdentityPath)); err != nil { + logrus.Error(err) + } + return os.Remove(m.IdentityPath) +} + +func (m *HyperVMachine) removeSystemConnections() error { + return machine.RemoveConnections(m.Name, fmt.Sprintf("%s-root", m.Name)) +} + func (m *HyperVMachine) Inspect() (*machine.InspectInfo, error) { vm, err := hypervctl.NewVirtualMachineManager().GetMachine(m.Name) if err != nil { diff --git a/pkg/machine/qemu/machine.go b/pkg/machine/qemu/machine.go index 7d25892521..751025c187 100644 --- a/pkg/machine/qemu/machine.go +++ b/pkg/machine/qemu/machine.go @@ -272,7 +272,14 @@ RequiredBy=default.target func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { var ( key string + err error ) + + // cleanup half-baked files if init fails at any point + callbackFuncs := machine.InitCleanup() + defer callbackFuncs.CleanIfErr(&err) + go callbackFuncs.CleanOnSignal() + v.IdentityPath = util.GetIdentityPath(v.Name) v.Rootful = opts.Rootful @@ -285,12 +292,13 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { if err != nil { return false, err } + callbackFuncs.Add(imagePath.Delete) // Assign values about the download v.ImagePath = *imagePath v.ImageStream = strm.String() - if err := v.addMountsToVM(opts); err != nil { + if err = v.addMountsToVM(opts); err != nil { return false, err } @@ -299,7 +307,7 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { // Add location of bootable image v.CmdLine.SetBootableImage(v.getImageFile()) - if err := machine.AddSSHConnectionsToPodmanSocket( + if err = machine.AddSSHConnectionsToPodmanSocket( v.UID, v.Port, v.IdentityPath, @@ -309,23 +317,25 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { ); err != nil { return false, err } + callbackFuncs.Add(v.removeSystemConnections) // Write the JSON file - if err := v.writeConfig(); err != nil { + if err = v.writeConfig(); err != nil { return false, fmt.Errorf("writing JSON file: %w", err) } + callbackFuncs.Add(v.ConfigPath.Delete) // User has provided ignition file so keygen // will be skipped. if len(opts.IgnitionPath) < 1 { - var err error key, err = machine.CreateSSHKeys(v.IdentityPath) if err != nil { return false, err } + callbackFuncs.Add(v.removeSSHKeys) } // Run arch specific things that need to be done - if err := v.prepare(); err != nil { + if err = v.prepare(); err != nil { return false, err } originalDiskSize, err := getDiskSize(v.getImageFile()) @@ -333,7 +343,7 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { return false, err } - if err := v.resizeDisk(opts.DiskSize, originalDiskSize>>(10*3)); err != nil { + if err = v.resizeDisk(opts.DiskSize, originalDiskSize>>(10*3)); err != nil { return false, err } @@ -352,9 +362,22 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { } err = v.writeIgnitionConfigFile(opts, key) + callbackFuncs.Add(v.IgnitionFile.Delete) + return err == nil, err } +func (v *MachineVM) removeSSHKeys() error { + if err := os.Remove(fmt.Sprintf("%s.pub", v.IdentityPath)); err != nil { + logrus.Error(err) + } + return os.Remove(v.IdentityPath) +} + +func (v *MachineVM) removeSystemConnections() error { + return machine.RemoveConnections(v.Name, fmt.Sprintf("%s-root", v.Name)) +} + func (v *MachineVM) Set(_ string, opts machine.SetOptions) ([]error, error) { // If one setting fails to be applied, the others settings will not fail and still be applied. // The setting(s) that failed to be applied will have its errors returned in setErrors diff --git a/pkg/machine/wsl/machine.go b/pkg/machine/wsl/machine.go index 47ce27aa92..d58f6a4351 100644 --- a/pkg/machine/wsl/machine.go +++ b/pkg/machine/wsl/machine.go @@ -391,6 +391,14 @@ func getLegacyLastStart(vm *MachineVM) time.Time { // Init writes the json configuration file to the filesystem for // other verbs (start, stop) func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { + var ( + err error + ) + // cleanup half-baked files if init fails at any point + callbackFuncs := machine.InitCleanup() + defer callbackFuncs.CleanIfErr(&err) + go callbackFuncs.CleanOnSignal() + if cont, err := checkAndInstallWSL(opts); !cont { appendOutputIfError(opts.ReExec, err) return cont, err @@ -402,20 +410,22 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { v.Version = currentMachineVersion if v.UserModeNetworking { - if err := verifyWSLUserModeCompat(); err != nil { + if err = verifyWSLUserModeCompat(); err != nil { return false, err } } - if err := downloadDistro(v, opts); err != nil { + if err = downloadDistro(v, opts); err != nil { return false, err } + callbackFuncs.Add(v.removeMachineImage) const prompt = "Importing operating system into WSL (this may take a few minutes on a new WSL install)..." dist, err := provisionWSLDist(v.Name, v.ImagePath, prompt) if err != nil { return false, err } + callbackFuncs.Add(v.unprovisionWSL) if v.UserModeNetworking { if err = installUserModeDist(dist, v.ImagePath); err != nil { @@ -436,21 +446,59 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) { if err = createKeys(v, dist); err != nil { return false, err } + callbackFuncs.Add(v.removeSSHKeys) // Cycle so that user change goes into effect _ = terminateDist(dist) - if err := v.writeConfig(); err != nil { + if err = v.writeConfig(); err != nil { return false, err } + callbackFuncs.Add(v.removeMachineConfig) - if err := setupConnections(v, opts); err != nil { + if err = setupConnections(v, opts); err != nil { return false, err } - + callbackFuncs.Add(v.removeSystemConnections) return true, nil } +func (v *MachineVM) unprovisionWSL() error { + if err := terminateDist(toDist(v.Name)); err != nil { + logrus.Error(err) + } + if err := unregisterDist(toDist(v.Name)); err != nil { + logrus.Error(err) + } + + vmDataDir, err := machine.GetDataDir(vmtype) + if err != nil { + return err + } + distDir := filepath.Join(vmDataDir, "wsldist") + distTarget := filepath.Join(distDir, v.Name) + return machine.GuardedRemoveAll(distTarget) +} + +func (v *MachineVM) removeMachineConfig() error { + return machine.GuardedRemoveAll(v.ConfigPath) +} + +func (v *MachineVM) removeMachineImage() error { + return machine.GuardedRemoveAll(v.ImagePath) +} + +func (v *MachineVM) removeSSHKeys() error { + if err := machine.GuardedRemoveAll(fmt.Sprintf("%s.pub", v.IdentityPath)); err != nil { + logrus.Error(err) + } + return machine.GuardedRemoveAll(v.IdentityPath) +} + +func (v *MachineVM) removeSystemConnections() error { + return machine.RemoveConnections(v.Name, fmt.Sprintf("%s-root", v.Name)) +} + func downloadDistro(v *MachineVM, opts machine.InitOptions) error { var ( dd machine.DistributionDownload @@ -1003,7 +1051,7 @@ func wslPipe(input string, dist string, arg ...string) error { } func wslCreateKeys(identityPath string, dist string) (string, error) { - return machine.CreateSSHKeysPrefix(identityPath, true, true, "wsl", "-u", "root", "-d", dist) + return machine.CreateSSHKeysPrefix(identityPath, true, false, "wsl", "-u", "root", "-d", dist) } func runCmdPassThrough(name string, arg ...string) error {