From 71b0909f2d71e542176a3e88111bf99c73d1f5cd Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 22 Feb 2022 11:05:26 -0500 Subject: Remove the runtime lock This primarily served to protect us against shutting down the Libpod runtime while operations (like creating a container) were happening. However, it was very inconsistently implemented (a lot of our longer-lived functions, like pulling images, just didn't implement it at all...) and I'm not sure how much we really care about this very-specific error case? Removing it also removes a lot of potential deadlocks, which is nice. [NO NEW TESTS NEEDED] Signed-off-by: Matthew Heon --- libpod/networking_linux.go | 2 +- libpod/runtime.go | 11 ----------- libpod/runtime_cstorage.go | 12 ------------ libpod/runtime_ctr.go | 32 +------------------------------- libpod/runtime_img.go | 3 --- libpod/runtime_pod.go | 18 ------------------ libpod/runtime_pod_linux.go | 9 --------- libpod/runtime_volume.go | 18 ------------------ libpod/runtime_volume_linux.go | 3 --- 9 files changed, 2 insertions(+), 106 deletions(-) diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index 19d5c7f76..29b9941fe 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -320,7 +320,7 @@ func (r *RootlessNetNS) Cleanup(runtime *Runtime) error { // only if the netns is empty we know that we do not need cleanup return c.state.NetNS != nil } - ctrs, err := runtime.GetContainersWithoutLock(activeNetns) + ctrs, err := runtime.GetContainers(activeNetns) if err != nil { return err } diff --git a/libpod/runtime.go b/libpod/runtime.go index dcf8c83f1..d19997709 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -11,7 +11,6 @@ import ( "regexp" "strconv" "strings" - "sync" "syscall" "time" @@ -109,7 +108,6 @@ type Runtime struct { // and remains true until the runtime is shut down (rendering its // storage unusable). When valid is false, the runtime cannot be used. valid bool - lock sync.RWMutex // mechanism to read and write even logs eventer events.Eventer @@ -713,9 +711,6 @@ func (r *Runtime) TmpDir() (string, error) { // Note that the returned value is not a copy and must hence // only be used in a reading fashion. func (r *Runtime) GetConfigNoCopy() (*config.Config, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -810,9 +805,6 @@ func (r *Runtime) DeferredShutdown(force bool) { // cleaning up; if force is false, an error will be returned if there are // still containers running or mounted func (r *Runtime) Shutdown(force bool) error { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return define.ErrRuntimeStopped } @@ -1016,9 +1008,6 @@ func (r *Runtime) RunRoot() string { // If the given ID does not correspond to any existing Pod or Container, // ErrNoSuchCtr is returned. func (r *Runtime) GetName(id string) (string, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return "", define.ErrRuntimeStopped } diff --git a/libpod/runtime_cstorage.go b/libpod/runtime_cstorage.go index 026cab3c5..1c528e1b8 100644 --- a/libpod/runtime_cstorage.go +++ b/libpod/runtime_cstorage.go @@ -21,9 +21,6 @@ type StorageContainer struct { // ListStorageContainers lists all containers visible to c/storage. func (r *Runtime) ListStorageContainers() ([]*StorageContainer, error) { - r.lock.RLock() - defer r.lock.RUnlock() - finalCtrs := []*StorageContainer{} ctrs, err := r.store.Containers() @@ -61,15 +58,6 @@ func (r *Runtime) StorageContainer(idOrName string) (*storage.Container, error) // Accepts ID or full name of container. // If force is set, the container will be unmounted first to ensure removal. func (r *Runtime) RemoveStorageContainer(idOrName string, force bool) error { - r.lock.Lock() - defer r.lock.Unlock() - - return r.removeStorageContainer(idOrName, force) -} - -// Internal function to remove the container storage without -// locking the runtime. -func (r *Runtime) removeStorageContainer(idOrName string, force bool) error { targetID, err := r.store.Lookup(idOrName) if err != nil { if errors.Cause(err) == storage.ErrLayerUnknown { diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index aeea85f89..fc1a688fb 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -42,8 +42,6 @@ type ContainerFilter func(*Container) bool // NewContainer creates a new container from a given OCI config. func (r *Runtime) NewContainer(ctx context.Context, rSpec *spec.Spec, spec *specgen.SpecGenerator, infra bool, options ...CtrCreateOption) (*Container, error) { - r.lock.Lock() - defer r.lock.Unlock() if !r.valid { return nil, define.ErrRuntimeStopped } @@ -81,8 +79,6 @@ func (r *Runtime) PrepareVolumeOnCreateContainer(ctx context.Context, ctr *Conta // RestoreContainer re-creates a container from an imported checkpoint func (r *Runtime) RestoreContainer(ctx context.Context, rSpec *spec.Spec, config *ContainerConfig) (*Container, error) { - r.lock.Lock() - defer r.lock.Unlock() if !r.valid { return nil, define.ErrRuntimeStopped } @@ -545,8 +541,6 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai // be removed also if and only if the container is the sole user // Otherwise, RemoveContainer will return an error if the container is running func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) error { - r.lock.Lock() - defer r.lock.Unlock() return r.removeContainer(ctx, c, force, removeVolume, false, timeout) } @@ -792,8 +786,6 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // If removeVolume is specified, named volumes used by the container will // be removed also if and only if the container is the sole user. func (r *Runtime) EvictContainer(ctx context.Context, idOrName string, removeVolume bool) (string, error) { - r.lock.RLock() - defer r.lock.RUnlock() return r.evictContainer(ctx, idOrName, removeVolume) } @@ -902,7 +894,7 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol } // Remove container from c/storage - if err := r.removeStorageContainer(id, true); err != nil { + if err := r.RemoveStorageContainer(id, true); err != nil { if cleanupErr == nil { cleanupErr = err } @@ -980,9 +972,6 @@ func (r *Runtime) RemoveDepend(ctx context.Context, rmCtr *Container, force bool // GetContainer retrieves a container by its ID func (r *Runtime) GetContainer(id string) (*Container, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -992,9 +981,6 @@ func (r *Runtime) GetContainer(id string) (*Container, error) { // HasContainer checks if a container with the given ID is present func (r *Runtime) HasContainer(id string) (bool, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return false, define.ErrRuntimeStopped } @@ -1005,9 +991,6 @@ func (r *Runtime) HasContainer(id string) (bool, error) { // LookupContainer looks up a container by its name or a partial ID // If a partial ID is not unique, an error will be returned func (r *Runtime) LookupContainer(idOrName string) (*Container, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -1017,9 +1000,6 @@ func (r *Runtime) LookupContainer(idOrName string) (*Container, error) { // LookupContainerId looks up a container id by its name or a partial ID // If a partial ID is not unique, an error will be returned func (r *Runtime) LookupContainerID(idOrName string) (string, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return "", define.ErrRuntimeStopped } @@ -1031,13 +1011,6 @@ func (r *Runtime) LookupContainerID(idOrName string) (string, error) { // the output. Multiple filters are handled by ANDing their output, so only // containers matching all filters are returned func (r *Runtime) GetContainers(filters ...ContainerFilter) ([]*Container, error) { - r.lock.RLock() - defer r.lock.RUnlock() - return r.GetContainersWithoutLock(filters...) -} - -// GetContainersWithoutLock is same as GetContainers but without lock -func (r *Runtime) GetContainersWithoutLock(filters ...ContainerFilter) ([]*Container, error) { if !r.valid { return nil, define.ErrRuntimeStopped } @@ -1115,9 +1088,6 @@ func (r *Runtime) GetLatestContainer() (*Container, error) { // GetExecSessionContainer gets the container that a given exec session ID is // attached to. func (r *Runtime) GetExecSessionContainer(id string) (*Container, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index e3b439dd1..54eadf6b8 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -25,9 +25,6 @@ import ( // we can use the libpod-internal removal logic. func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage.RemoveContainerFunc { return func(imageID string) error { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return define.ErrRuntimeStopped } diff --git a/libpod/runtime_pod.go b/libpod/runtime_pod.go index 11891630a..dca0ffc8a 100644 --- a/libpod/runtime_pod.go +++ b/libpod/runtime_pod.go @@ -27,9 +27,6 @@ type PodFilter func(*Pod) bool // being removed // Otherwise, the pod will not be removed if any containers are running func (r *Runtime) RemovePod(ctx context.Context, p *Pod, removeCtrs, force bool, timeout *uint) error { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return define.ErrRuntimeStopped } @@ -50,9 +47,6 @@ func (r *Runtime) RemovePod(ctx context.Context, p *Pod, removeCtrs, force bool, // GetPod retrieves a pod by its ID func (r *Runtime) GetPod(id string) (*Pod, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -62,9 +56,6 @@ func (r *Runtime) GetPod(id string) (*Pod, error) { // HasPod checks to see if a pod with the given ID exists func (r *Runtime) HasPod(id string) (bool, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return false, define.ErrRuntimeStopped } @@ -75,9 +66,6 @@ func (r *Runtime) HasPod(id string) (bool, error) { // LookupPod retrieves a pod by its name or a partial ID // If a partial ID is not unique, an error will be returned func (r *Runtime) LookupPod(idOrName string) (*Pod, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -111,9 +99,6 @@ func (r *Runtime) Pods(filters ...PodFilter) ([]*Pod, error) { // GetAllPods retrieves all pods func (r *Runtime) GetAllPods() ([]*Pod, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -148,9 +133,6 @@ func (r *Runtime) GetRunningPods() ([]*Pod, error) { pods []string runningPods []*Pod ) - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } diff --git a/libpod/runtime_pod_linux.go b/libpod/runtime_pod_linux.go index 7bc675af7..155ad5c2d 100644 --- a/libpod/runtime_pod_linux.go +++ b/libpod/runtime_pod_linux.go @@ -22,9 +22,6 @@ import ( // NewPod makes a new, empty pod func (r *Runtime) NewPod(ctx context.Context, p specgen.PodSpecGenerator, options ...PodCreateOption) (_ *Pod, deferredErr error) { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -151,9 +148,6 @@ func (r *Runtime) NewPod(ctx context.Context, p specgen.PodSpecGenerator, option // AddInfra adds the created infra container to the pod state func (r *Runtime) AddInfra(ctx context.Context, pod *Pod, infraCtr *Container) (*Pod, error) { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -167,9 +161,6 @@ func (r *Runtime) AddInfra(ctx context.Context, pod *Pod, infraCtr *Container) ( // SavePod is a helper function to save the pod state from outside of libpod func (r *Runtime) SavePod(pod *Pod) error { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return define.ErrRuntimeStopped } diff --git a/libpod/runtime_volume.go b/libpod/runtime_volume.go index a3be0ff5b..21bf8aefc 100644 --- a/libpod/runtime_volume.go +++ b/libpod/runtime_volume.go @@ -22,9 +22,6 @@ type VolumeFilter func(*Volume) bool // RemoveVolume removes a volumes func (r *Runtime) RemoveVolume(ctx context.Context, v *Volume, force bool, timeout *uint) error { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return define.ErrRuntimeStopped } @@ -41,9 +38,6 @@ func (r *Runtime) RemoveVolume(ctx context.Context, v *Volume, force bool, timeo // GetVolume retrieves a volume given its full name. func (r *Runtime) GetVolume(name string) (*Volume, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -58,9 +52,6 @@ func (r *Runtime) GetVolume(name string) (*Volume, error) { // LookupVolume retrieves a volume by unambiguous partial name. func (r *Runtime) LookupVolume(name string) (*Volume, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -75,9 +66,6 @@ func (r *Runtime) LookupVolume(name string) (*Volume, error) { // HasVolume checks to see if a volume with the given name exists func (r *Runtime) HasVolume(name string) (bool, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return false, define.ErrRuntimeStopped } @@ -90,9 +78,6 @@ func (r *Runtime) HasVolume(name string) (bool, error) { // output. If multiple filters are used, a volume will be returned if // any of the filters are matched func (r *Runtime) Volumes(filters ...VolumeFilter) ([]*Volume, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } @@ -123,9 +108,6 @@ func (r *Runtime) Volumes(filters ...VolumeFilter) ([]*Volume, error) { // GetAllVolumes retrieves all the volumes func (r *Runtime) GetAllVolumes() ([]*Volume, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if !r.valid { return nil, define.ErrRuntimeStopped } diff --git a/libpod/runtime_volume_linux.go b/libpod/runtime_volume_linux.go index 5fd68fffb..c4fe3db90 100644 --- a/libpod/runtime_volume_linux.go +++ b/libpod/runtime_volume_linux.go @@ -21,9 +21,6 @@ import ( // NewVolume creates a new empty volume func (r *Runtime) NewVolume(ctx context.Context, options ...VolumeCreateOption) (*Volume, error) { - r.lock.Lock() - defer r.lock.Unlock() - if !r.valid { return nil, define.ErrRuntimeStopped } -- cgit v1.2.3-54-g00ecf