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 {

pkg/machine/qemu/machine.go

@@ -249,7 +249,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
 
@@ -262,12 +269,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
 	}
 
@@ -276,7 +284,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,
@@ -286,23 +294,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())
@@ -310,7 +320,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
 	}
 
@@ -329,9 +339,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

pkg/machine/wsl/machine.go

@@ -394,6 +394,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
@@ -405,20 +413,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 {
@@ -439,21 +449,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
@@ -1006,7 +1054,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 {