Merge pull request #20274 from ashley-cui/cleanup

Machine: Teardown on init failure

pkg/machine/applehv/machine.go

@@ -192,7 +192,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
@@ -211,6 +219,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
@@ -220,6 +229,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 {
@@ -232,11 +243,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
 	}
 
@@ -252,7 +263,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
 	}
 
@@ -267,22 +278,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 {
@@ -294,9 +306,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 {

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()
+}

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())
+		}
+	})
 })

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())
-
 	})
 })

pkg/machine/hyperv/machine.go

@@ -203,11 +203,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)
 
@@ -233,13 +247,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{
@@ -258,6 +273,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
@@ -268,7 +284,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
@@ -277,6 +293,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 {