Merge pull request #21980 from Luap99/machine-locking-fixes

more machine locking fixes

cmd/podman/machine/init.go

@@ -208,16 +208,11 @@ func initMachine(cmd *cobra.Command, args []string) error {
 	// 	return err
 	// }
 
-	mc, err := shim.Init(initOpts, provider)
+	err = shim.Init(initOpts, provider)
 	if err != nil {
 		return err
 	}
 
-	// TODO callback needed for the configuration file
-	if err := mc.Write(); err != nil {
-		return err
-	}
-
 	newMachineEvent(events.Init, events.Event{Name: initOpts.Name})
 	fmt.Println("Machine init complete")
 

cmd/podman/machine/set.go

@@ -3,8 +3,6 @@
 package machine
 
 import (
-	"fmt"
-
 	"github.com/containers/common/pkg/completion"
 	"github.com/containers/common/pkg/strongunits"
 	"github.com/containers/podman/v5/cmd/podman/registry"
@@ -90,10 +88,6 @@ func init() {
 }
 
 func setMachine(cmd *cobra.Command, args []string) error {
-	var (
-		err error
-	)
-
 	vmName := defaultMachineName
 	if len(args) > 0 && len(args[0]) > 0 {
 		vmName = args[0]
@@ -113,20 +107,14 @@ func setMachine(cmd *cobra.Command, args []string) error {
 		setOpts.Rootful = &setFlags.Rootful
 	}
 	if cmd.Flags().Changed("cpus") {
-		mc.Resources.CPUs = setFlags.CPUs
+		setOpts.CPUs = &setFlags.CPUs
-		setOpts.CPUs = &mc.Resources.CPUs
 	}
 	if cmd.Flags().Changed("memory") {
-		mc.Resources.Memory = strongunits.MiB(setFlags.Memory)
+		newMemory := strongunits.MiB(setFlags.Memory)
-		setOpts.Memory = &mc.Resources.Memory
+		setOpts.Memory = &newMemory
 	}
 	if cmd.Flags().Changed("disk-size") {
 		newDiskSizeGB := strongunits.GiB(setFlags.DiskSize)
-		if newDiskSizeGB <= mc.Resources.DiskSize {
-			return fmt.Errorf("new disk size must be larger than %d GB", mc.Resources.DiskSize)
-		}
-		mc.Resources.DiskSize = newDiskSizeGB
-
 		setOpts.DiskSize = &newDiskSizeGB
 	}
 	if cmd.Flags().Changed("user-mode-networking") {

cmd/podman/machine/start.go

@@ -12,7 +12,6 @@ import (
 	"github.com/containers/podman/v5/pkg/machine/env"
 	"github.com/containers/podman/v5/pkg/machine/shim"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
-	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
 
@@ -82,19 +81,6 @@ func start(_ *cobra.Command, args []string) error {
 		fmt.Printf("Starting machine %q\n", vmName)
 	}
 
-	// Set starting to true
-	mc.Starting = true
-	if err := mc.Write(); err != nil {
-		logrus.Error(err)
-	}
-
-	// Set starting to false on exit
-	defer func() {
-		mc.Starting = false
-		if err := mc.Write(); err != nil {
-			logrus.Error(err)
-		}
-	}()
 	if err := shim.Start(mc, provider, dirs, startOpts); err != nil {
 		return err
 	}

cmd/podman/machine/stop.go

@@ -4,14 +4,12 @@ package machine
 
 import (
 	"fmt"
-	"time"
 
 	"github.com/containers/podman/v5/cmd/podman/registry"
 	"github.com/containers/podman/v5/libpod/events"
 	"github.com/containers/podman/v5/pkg/machine/env"
 	"github.com/containers/podman/v5/pkg/machine/shim"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
-	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
 
@@ -59,12 +57,6 @@ func stop(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	// Update last time up
-	mc.LastUp = time.Now()
-	if err := mc.Write(); err != nil {
-		logrus.Errorf("unable to write configuration file: %q", err)
-	}
-
 	fmt.Printf("Machine %q stopped successfully\n", vmName)
 	newMachineEvent(events.Stop, events.Event{Name: vmName})
 	return nil

pkg/machine/shim/host.go

@@ -15,6 +15,7 @@ import (
 	machineDefine "github.com/containers/podman/v5/pkg/machine/define"
 	"github.com/containers/podman/v5/pkg/machine/env"
 	"github.com/containers/podman/v5/pkg/machine/ignition"
+	"github.com/containers/podman/v5/pkg/machine/lock"
 	"github.com/containers/podman/v5/pkg/machine/proxyenv"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
 	"github.com/containers/podman/v5/utils"
@@ -66,7 +67,7 @@ func List(vmstubbers []vmconfigs.VMProvider, _ machine.ListOptions) ([]*machine.
 	return lrs, nil
 }
 
-func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.MachineConfig, error) {
+func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error {
 	var (
 		err            error
 		imageExtension string
@@ -79,21 +80,28 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	dirs, err := env.GetMachineDirs(mp.VMType())
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	sshIdentityPath, err := env.GetSSHIdentityPath(machineDefine.DefaultIdentityName)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	sshKey, err := machine.GetSSHKeys(sshIdentityPath)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	mc, err := vmconfigs.NewMachineConfig(opts, dirs, sshIdentityPath, mp.VMType())
+	machineLock, err := lock.GetMachineLock(opts.Name, dirs.ConfigDir.GetPath())
 	if err != nil {
-		return nil, err
+		return err
+	}
+	machineLock.Lock()
+	defer machineLock.Unlock()
+
+	mc, err := vmconfigs.NewMachineConfig(opts, dirs, sshIdentityPath, mp.VMType(), machineLock)
+	if err != nil {
+		return err
 	}
 
 	mc.Version = vmconfigs.MachineConfigVersion
@@ -128,7 +136,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	imagePath, err = dirs.DataDir.AppendToNewVMFile(fmt.Sprintf("%s-%s%s", opts.Name, runtime.GOARCH, imageExtension), nil)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	mc.ImagePath = imagePath
 
@@ -142,7 +150,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 	// "docker://quay.io/something/someManifest
 
 	if err := mp.GetDisk(opts.Image, dirs, mc); err != nil {
-		return nil, err
+		return err
 	}
 
 	callbackFuncs.Add(mc.ImagePath.Delete)
@@ -151,7 +159,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	ignitionFile, err := mc.IgnitionFile()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	uid := os.Getuid()
@@ -184,22 +192,22 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 	// copy it into the conf dir
 	if len(opts.IgnitionPath) > 0 {
 		err = ignBuilder.BuildWithIgnitionFile(opts.IgnitionPath)
-		return nil, err
+		return err
 	}
 
 	err = ignBuilder.GenerateIgnitionConfig()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	readyUnit := ignition.Unit{
@@ -216,7 +224,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	// TODO AddSSHConnectionToPodmanSocket could take an machineconfig instead
 	if err := connection.AddSSHConnectionsToPodmanSocket(mc.HostUser.UID, mc.SSH.Port, mc.SSH.IdentityPath, mc.Name, mc.SSH.RemoteUsername, opts); err != nil {
-		return nil, err
+		return err
 	}
 
 	cleanup := func() error {
@@ -226,15 +234,15 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	err = mp.CreateVM(createOpts, mc, &ignBuilder)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	err = ignBuilder.Build()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	return mc, err
+	return mc.Write()
 }
 
 // VMExists looks across given providers for a machine's existence.  returns the actual config and found bool
@@ -358,7 +366,9 @@ func stopLocked(mc *vmconfigs.MachineConfig, mp vmconfigs.VMProvider, dirs *mach
 		}
 	}
 
-	return nil
+	// Update last time up
+	mc.LastUp = time.Now()
+	return mc.Write()
 }
 
 func Start(mc *vmconfigs.MachineConfig, mp vmconfigs.VMProvider, dirs *machineDefine.MachineDirs, opts machine.StartOptions) error {
@@ -368,6 +378,19 @@ func Start(mc *vmconfigs.MachineConfig, mp vmconfigs.VMProvider, dirs *machineDe
 	mc.Lock()
 	defer mc.Unlock()
 
+	// Set starting to true
+	mc.Starting = true
+	if err := mc.Write(); err != nil {
+		logrus.Error(err)
+	}
+	// Set starting to false on exit
+	defer func() {
+		mc.Starting = false
+		if err := mc.Write(); err != nil {
+			logrus.Error(err)
+		}
+	}()
+
 	gvproxyPidFile, err := dirs.RuntimeDir.AppendToNewVMFile("gvproxy.pid", nil)
 	if err != nil {
 		return err
@@ -485,6 +508,25 @@ func Set(mc *vmconfigs.MachineConfig, mp vmconfigs.VMProvider, opts machineDefin
 	mc.Lock()
 	defer mc.Unlock()
 
+	if err := mc.Refresh(); err != nil {
+		return fmt.Errorf("reload config: %w", err)
+	}
+
+	if opts.CPUs != nil {
+		mc.Resources.CPUs = *opts.CPUs
+	}
+
+	if opts.Memory != nil {
+		mc.Resources.Memory = *opts.Memory
+	}
+
+	if opts.DiskSize != nil {
+		if *opts.DiskSize <= mc.Resources.DiskSize {
+			return fmt.Errorf("new disk size must be larger than %d GB", mc.Resources.DiskSize)
+		}
+		mc.Resources.DiskSize = *opts.DiskSize
+	}
+
 	if err := mp.SetProviderAttrs(mc, opts); err != nil {
 		return err
 	}

pkg/machine/vmconfigs/machine.go

@@ -18,6 +18,7 @@ import (
 	"github.com/containers/podman/v5/pkg/machine/lock"
 	"github.com/containers/podman/v5/pkg/machine/ports"
 	"github.com/containers/storage/pkg/ioutils"
+	"github.com/containers/storage/pkg/lockfile"
 	"github.com/sirupsen/logrus"
 )
 
@@ -42,16 +43,11 @@ var (
 
 type RemoteConnectionType string
 
-// NewMachineConfig creates the initial machine configuration file from cli options
+// NewMachineConfig creates the initial machine configuration file from cli options.
-func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIdentityPath string, vmtype define.VMType) (*MachineConfig, error) {
+func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIdentityPath string, vmtype define.VMType, machineLock *lockfile.LockFile) (*MachineConfig, error) {
 	mc := new(MachineConfig)
 	mc.Name = opts.Name
 	mc.dirs = dirs
-
-	machineLock, err := lock.GetMachineLock(opts.Name, dirs.ConfigDir.GetPath())
-	if err != nil {
-		return nil, err
-	}
 	mc.lock = machineLock
 
 	// Assign Dirs
@@ -60,6 +56,11 @@ func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIden
 		return nil, err
 	}
 	mc.configPath = cf
+	// Given that we are locked now and check again that the config file does not exists,
+	// if it does it means the VM was already created and we should error.
+	if _, err := os.Stat(cf.Path); err == nil {
+		return nil, fmt.Errorf("%s: %w", opts.Name, define.ErrVMAlreadyExists)
+	}
 
 	if vmtype != define.QemuVirt && len(opts.USBs) > 0 {
 		return nil, fmt.Errorf("USB host passthrough not supported for %s machines", vmtype)