From ee7cf3cc2c212ff7a24bda49f95d494d99eb3cd7 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Mon, 14 Feb 2022 15:43:52 -0500 Subject: Don't log errors on removing volumes inuse, if container --volumes-from When removing a container created with a --volumes-from a container created with a built in volume, we complain if the original container still exists. Since this is an expected state, we should not complain about it. Fixes: https://github.com/containers/podman/issues/12808 Signed-off-by: Daniel J Walsh --- libpod/container_inspect.go | 11 +++++++++++ libpod/runtime_ctr.go | 8 ++++++++ 2 files changed, 19 insertions(+) (limited to 'libpod') diff --git a/libpod/container_inspect.go b/libpod/container_inspect.go index 1344fc659..0bbfe3b70 100644 --- a/libpod/container_inspect.go +++ b/libpod/container_inspect.go @@ -51,6 +51,17 @@ func (c *Container) Inspect(size bool) (*define.InspectContainerData, error) { return c.inspectLocked(size) } +func (c *Container) volumesFrom() ([]string, error) { + ctrSpec, err := c.specFromState() + if err != nil { + return nil, err + } + if ctrs, ok := ctrSpec.Annotations[define.InspectAnnotationVolumesFrom]; ok { + return strings.Split(ctrs, ","), nil + } + return nil, nil +} + func (c *Container) getContainerInspectData(size bool, driverData *define.DriverData) (*define.InspectContainerData, error) { config := c.config runtimeInfo := c.state diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 44364100e..aeea85f89 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -768,6 +768,14 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo continue } if err := runtime.removeVolume(ctx, volume, false, timeout); err != nil && errors.Cause(err) != define.ErrNoSuchVolume { + if errors.Cause(err) == define.ErrVolumeBeingUsed { + // Ignore error, since podman will report original error + volumesFrom, _ := c.volumesFrom() + if len(volumesFrom) > 0 { + logrus.Debugf("Cleanup volume not possible since volume is in use (%s)", v) + continue + } + } logrus.Errorf("Cleanup volume (%s): %v", v, err) } } -- cgit v1.2.3-54-g00ecf 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(-) (limited to 'libpod') 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 From 697dd2890e73db46c50bc891e94104c4b87c9542 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 19 Jan 2022 13:20:53 +1100 Subject: Propagate $CONTAINERS_CONF to conmon The CONTAINERS_CONF environment variable can be used to override the configuration file, which is useful for testing. However, at the moment this variable is not propagated to conmon. That means in particular, that conmon can't propagate it back to podman when invoking its --exit-command. The mismatch in configuration between the starting and cleaning up podman instances can cause a variety of errors. This patch also adds two related test cases. One checks explicitly that the correct CONTAINERS_CONF value appears in conmon's environment. The other checks for a possible specific impact of this bug: if we use a nonstandard name for the runtime (even if its path is just a regular crun), then the podman container cleanup invoked at container exit will fail. That has the effect of meaning that a container started with -d --rm won't be correctly removed once complete. Fixes #12917 Signed-off-by: David Gibson --- libpod/oci_conmon_linux.go | 4 +++ test/system/800-config.bats | 80 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 test/system/800-config.bats (limited to 'libpod') diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index 268a301fb..a328f7621 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -1318,6 +1318,10 @@ func (r *ConmonOCIRuntime) configureConmonEnv(ctr *Container, runtimeDir string) env = append(env, e) } } + conf, ok := os.LookupEnv("CONTAINERS_CONF") + if ok { + env = append(env, fmt.Sprintf("CONTAINERS_CONF=%s", conf)) + } env = append(env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir)) env = append(env, fmt.Sprintf("_CONTAINERS_USERNS_CONFIGURED=%s", os.Getenv("_CONTAINERS_USERNS_CONFIGURED"))) env = append(env, fmt.Sprintf("_CONTAINERS_ROOTLESS_UID=%s", os.Getenv("_CONTAINERS_ROOTLESS_UID"))) diff --git a/test/system/800-config.bats b/test/system/800-config.bats new file mode 100644 index 000000000..f5b4e9570 --- /dev/null +++ b/test/system/800-config.bats @@ -0,0 +1,80 @@ +#!/usr/bin/env bats -*- bats -*- +# +# Test specific configuration options and overrides +# + +load helpers + +@test "podman CONTAINERS_CONF - CONTAINERS_CONF in conmon" { + skip_if_remote "can't check conmon environment over remote" + + # Get the normal runtime for this host + run_podman info --format '{{ .Host.OCIRuntime.Name }}' + runtime="$output" + run_podman info --format "{{ .Host.OCIRuntime.Path }}" + ocipath="$output" + + # Make an innocuous containers.conf in a non-standard location + conf_tmp="$PODMAN_TMPDIR/containers.conf" + cat >$conf_tmp < $conf_tmp <