From 987dc2b8bbd892e1e53d6c3eb27ceb54e5e88998 Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Tue, 10 Oct 2023 09:51:17 -0400 Subject: [PATCH] SetLock for all virt providers Implements a shared `GetLock` function for virtualization providers. Returns a pointer to a lockfile used for serializing write operations. [NO NEW TESTS NEEDED] Signed-off-by: Jake Correnti --- pkg/machine/applehv/machine.go | 24 +++++++++++++++++++++++ pkg/machine/config.go | 19 ++++++++++++++++++ pkg/machine/hyperv/machine.go | 23 ++++++++++++++++++++++ pkg/machine/qemu/config.go | 6 ++++++ pkg/machine/qemu/machine.go | 35 ---------------------------------- pkg/machine/wsl/config.go | 10 ++++++++++ pkg/machine/wsl/machine.go | 15 +++++++++++++++ 7 files changed, 97 insertions(+), 35 deletions(-) diff --git a/pkg/machine/applehv/machine.go b/pkg/machine/applehv/machine.go index da81d79c72..6541ee8dc7 100644 --- a/pkg/machine/applehv/machine.go +++ b/pkg/machine/applehv/machine.go @@ -24,6 +24,7 @@ import ( "github.com/containers/podman/v4/pkg/strongunits" "github.com/containers/podman/v4/pkg/util" "github.com/containers/podman/v4/utils" + "github.com/containers/storage/pkg/lockfile" vfConfig "github.com/crc-org/vfkit/pkg/config" vfRest "github.com/crc-org/vfkit/pkg/rest" "github.com/docker/go-units" @@ -77,6 +78,9 @@ type MacMachine struct { LogPath machine.VMFile GvProxyPid machine.VMFile GvProxySock machine.VMFile + + // Used at runtime for serializing write operations + lock *lockfile.LockFile } // setGVProxyInfo sets the VM's gvproxy pid and socket files @@ -353,6 +357,9 @@ func (m *MacMachine) Remove(name string, opts machine.RemoveOptions) (string, fu files []string ) + m.lock.Lock() + defer m.lock.Unlock() + vmState, err := m.Vfkit.state() if err != nil { return "", nil, err @@ -403,6 +410,10 @@ func (m *MacMachine) writeConfig() error { func (m *MacMachine) Set(name string, opts machine.SetOptions) ([]error, error) { var setErrors []error + + m.lock.Lock() + defer m.lock.Unlock() + vmState, err := m.State(false) if err != nil { return nil, err @@ -527,6 +538,9 @@ func (m *MacMachine) addVolumesToVfKit() error { func (m *MacMachine) Start(name string, opts machine.StartOptions) error { var ignitionSocket *machine.VMFile + m.lock.Lock() + defer m.lock.Unlock() + st, err := m.State(false) if err != nil { return err @@ -680,6 +694,9 @@ func (m *MacMachine) State(_ bool) (machine.Status, error) { } func (m *MacMachine) Stop(name string, opts machine.StopOptions) error { + m.lock.Lock() + defer m.lock.Unlock() + vmState, err := m.State(false) if err != nil { return err @@ -718,6 +735,13 @@ func (m *MacMachine) loadFromFile() (*MacMachine, error) { if err := loadMacMachineFromJSON(jsonPath, &mm); err != nil { return nil, err } + + lock, err := machine.GetLock(mm.Name, vmtype) + if err != nil { + return nil, err + } + mm.lock = lock + return &mm, nil } diff --git a/pkg/machine/config.go b/pkg/machine/config.go index da2df61dd9..5acab5c3f8 100644 --- a/pkg/machine/config.go +++ b/pkg/machine/config.go @@ -16,6 +16,7 @@ import ( "time" "github.com/containers/storage/pkg/homedir" + "github.com/containers/storage/pkg/lockfile" "github.com/sirupsen/logrus" ) @@ -137,6 +138,24 @@ type VM interface { Stop(name string, opts StopOptions) error } +func GetLock(name string, vmtype VMType) (*lockfile.LockFile, error) { + // FIXME: there's a painful amount of `GetConfDir` calls scattered + // across the code base. This should be done once and stored + // somewhere instead. + vmConfigDir, err := GetConfDir(vmtype) + if err != nil { + return nil, err + } + + lockPath := filepath.Join(vmConfigDir, name+".lock") + lock, err := lockfile.GetLockFile(lockPath) + if err != nil { + return nil, fmt.Errorf("creating lockfile for VM: %w", err) + } + + return lock, nil +} + type DistributionDownload interface { HasUsableCache() (bool, error) Get() *Download diff --git a/pkg/machine/hyperv/machine.go b/pkg/machine/hyperv/machine.go index b6f091cdfc..b8ffa3bae4 100644 --- a/pkg/machine/hyperv/machine.go +++ b/pkg/machine/hyperv/machine.go @@ -21,6 +21,7 @@ import ( "github.com/containers/podman/v4/pkg/strongunits" "github.com/containers/podman/v4/pkg/util" "github.com/containers/podman/v4/utils" + "github.com/containers/storage/pkg/lockfile" "github.com/sirupsen/logrus" ) @@ -66,6 +67,8 @@ type HyperVMachine struct { LastUp time.Time // GVProxy will write its PID here GvProxyPid machine.VMFile + // Used at runtime for serializing write operations + lock *lockfile.LockFile } // addNetworkAndReadySocketsToRegistry adds the Network and Ready sockets to the @@ -358,6 +361,9 @@ func (m *HyperVMachine) Remove(_ string, opts machine.RemoveOptions) (string, fu files []string diskPath string ) + m.lock.Lock() + defer m.lock.Unlock() + vmm := hypervctl.NewVirtualMachineManager() vm, err := vmm.GetMachine(m.Name) if err != nil { @@ -400,6 +406,10 @@ func (m *HyperVMachine) Set(name string, opts machine.SetOptions) ([]error, erro cpuChanged, memoryChanged bool setErrors []error ) + + m.lock.Lock() + defer m.lock.Unlock() + vmm := hypervctl.NewVirtualMachineManager() // Considering this a hard return if we cannot lookup the machine vm, err := vmm.GetMachine(m.Name) @@ -476,6 +486,9 @@ func (m *HyperVMachine) SSH(name string, opts machine.SSHOptions) error { } func (m *HyperVMachine) Start(name string, opts machine.StartOptions) error { + m.lock.Lock() + defer m.lock.Unlock() + vmm := hypervctl.NewVirtualMachineManager() vm, err := vmm.GetMachine(m.Name) if err != nil { @@ -536,6 +549,9 @@ func (m *HyperVMachine) State(_ bool) (machine.Status, error) { } func (m *HyperVMachine) Stop(name string, opts machine.StopOptions) error { + m.lock.Lock() + defer m.lock.Unlock() + vmm := hypervctl.NewVirtualMachineManager() vm, err := vmm.GetMachine(m.Name) if err != nil { @@ -582,6 +598,13 @@ func (m *HyperVMachine) loadFromFile() (*HyperVMachine, error) { } return nil, err } + + lock, err := machine.GetLock(mm.Name, vmtype) + if err != nil { + return nil, err + } + mm.lock = lock + vmm := hypervctl.NewVirtualMachineManager() vm, err := vmm.GetMachine(m.Name) if err != nil { diff --git a/pkg/machine/qemu/config.go b/pkg/machine/qemu/config.go index 965daa9b24..90ebb057f4 100644 --- a/pkg/machine/qemu/config.go +++ b/pkg/machine/qemu/config.go @@ -125,6 +125,12 @@ func (p *QEMUVirtualization) LoadVMByName(name string) (machine.VM, error) { return nil, err } + lock, err := machine.GetLock(vm.Name, vmtype) + if err != nil { + return nil, err + } + vm.lock = lock + return vm, nil } diff --git a/pkg/machine/qemu/machine.go b/pkg/machine/qemu/machine.go index 7d25892521..e90eacf478 100644 --- a/pkg/machine/qemu/machine.go +++ b/pkg/machine/qemu/machine.go @@ -80,29 +80,6 @@ type MachineVM struct { lock *lockfile.LockFile } -func (v *MachineVM) setLock() error { - if v.lock != nil { - return nil - } - - // FIXME: there's a painful amount of `GetConfDir` calls scattered - // across the code base. This should be done once and stored - // somewhere instead. - vmConfigDir, err := machine.GetConfDir(vmtype) - if err != nil { - return err - } - - lockPath := filepath.Join(vmConfigDir, v.Name+".lock") - lock, err := lockfile.GetLockFile(lockPath) - if err != nil { - return fmt.Errorf("creating lockfile for VM: %w", err) - } - - v.lock = lock - return nil -} - type Monitor struct { // Address portion of the qmp monitor (/tmp/tmp.sock) Address machine.VMFile @@ -360,9 +337,6 @@ func (v *MachineVM) Set(_ string, opts machine.SetOptions) ([]error, error) { // The setting(s) that failed to be applied will have its errors returned in setErrors var setErrors []error - if err := v.setLock(); err != nil { - return nil, err - } v.lock.Lock() defer v.lock.Unlock() @@ -596,9 +570,6 @@ func (v *MachineVM) Start(name string, opts machine.StartOptions) error { defaultBackoff := 500 * time.Millisecond maxBackoffs := 6 - if err := v.setLock(); err != nil { - return err - } v.lock.Lock() defer v.lock.Unlock() @@ -929,9 +900,6 @@ func (v *MachineVM) VMPid() (int, error) { // Stop uses the qmp monitor to call a system_powerdown func (v *MachineVM) Stop(_ string, _ machine.StopOptions) error { - if err := v.setLock(); err != nil { - return err - } v.lock.Lock() defer v.lock.Unlock() @@ -1154,9 +1122,6 @@ func (v *MachineVM) Remove(_ string, opts machine.RemoveOptions) (string, func() files []string ) - if err := v.setLock(); err != nil { - return "", nil, err - } v.lock.Lock() defer v.lock.Unlock() diff --git a/pkg/machine/wsl/config.go b/pkg/machine/wsl/config.go index 5e158a2fe2..e20be7b8d3 100644 --- a/pkg/machine/wsl/config.go +++ b/pkg/machine/wsl/config.go @@ -71,6 +71,16 @@ func (p *WSLVirtualization) LoadVMByName(name string) (machine.VM, error) { } vm, err := readAndMigrate(configPath, name) + if err != nil { + return nil, err + } + + lock, err := machine.GetLock(vm.Name, vmtype) + if err != nil { + return nil, err + } + vm.lock = lock + return vm, err } diff --git a/pkg/machine/wsl/machine.go b/pkg/machine/wsl/machine.go index 47ce27aa92..cf1048b2e5 100644 --- a/pkg/machine/wsl/machine.go +++ b/pkg/machine/wsl/machine.go @@ -22,6 +22,7 @@ import ( "github.com/containers/podman/v4/pkg/machine/wsl/wutil" "github.com/containers/podman/v4/pkg/util" "github.com/containers/storage/pkg/homedir" + "github.com/containers/storage/pkg/lockfile" "github.com/sirupsen/logrus" "golang.org/x/text/encoding/unicode" "golang.org/x/text/transform" @@ -300,6 +301,8 @@ type MachineVM struct { Version int // Whether to use user-mode networking UserModeNetworking bool + // Used at runtime for serializing write operations + lock *lockfile.LockFile } type ExitCodeError struct { @@ -1061,6 +1064,9 @@ func (v *MachineVM) Set(_ string, opts machine.SetOptions) ([]error, error) { // The setting(s) that failed to be applied will have its errors returned in setErrors var setErrors []error + v.lock.Lock() + defer v.lock.Unlock() + if opts.Rootful != nil && v.Rootful != *opts.Rootful { err := v.setRootful(*opts.Rootful) if err != nil { @@ -1118,6 +1124,9 @@ func (v *MachineVM) Set(_ string, opts machine.SetOptions) ([]error, error) { } func (v *MachineVM) Start(name string, opts machine.StartOptions) error { + v.lock.Lock() + defer v.lock.Unlock() + if v.isRunning() { return machine.ErrVMAlreadyRunning } @@ -1406,6 +1415,9 @@ func isSystemdRunning(dist string) (bool, error) { } func (v *MachineVM) Stop(name string, _ machine.StopOptions) error { + v.lock.Lock() + defer v.lock.Unlock() + dist := toDist(v.Name) wsl, err := isWSLRunning(dist) @@ -1529,6 +1541,9 @@ func readWinProxyTid(v *MachineVM) (uint32, uint32, string, error) { func (v *MachineVM) Remove(name string, opts machine.RemoveOptions) (string, func() error, error) { var files []string + v.lock.Lock() + defer v.lock.Unlock() + if v.isRunning() { if !opts.Force { return "", nil, &machine.ErrVMRunningCannotDestroyed{Name: v.Name}