From 7a75914921d93fe63b43729fd8fa5ad3130cb576 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 7 Mar 2024 14:58:25 +0100 Subject: [PATCH] podman machine init: do not write config unlocked First make sure we check that a given VM exist when holding the VM lock for it. The check in cmd/podman/machine/init.go is a nice quick out but not enough to ensure that 2 processes to not create the same VM at the same time. The only way to ensure this is by holding the lock and checking if the VM config file exists. Signed-off-by: Paul Holzinger --- cmd/podman/machine/init.go | 7 +----- pkg/machine/shim/host.go | 42 +++++++++++++++++++------------- pkg/machine/vmconfigs/machine.go | 15 ++++++------ 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/cmd/podman/machine/init.go b/cmd/podman/machine/init.go index 3e97a7f942..bc3aac3dd3 100644 --- a/cmd/podman/machine/init.go +++ b/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") diff --git a/pkg/machine/shim/host.go b/pkg/machine/shim/host.go index 836b84cf1a..ddf4739980 100644 --- a/pkg/machine/shim/host.go +++ b/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 diff --git a/pkg/machine/vmconfigs/machine.go b/pkg/machine/vmconfigs/machine.go index 4cf9a27186..d5a3e41258 100644 --- a/pkg/machine/vmconfigs/machine.go +++ b/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 -func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIdentityPath string, vmtype define.VMType) (*MachineConfig, error) { +// NewMachineConfig creates the initial machine configuration file from cli options. +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)