From 95566d04f81aaf0246d8d2f42e5c550a1c7f7904 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 7 Dec 2020 11:50:47 -0500 Subject: Change name of imageVolumes in container config JSON Podman pre-1.8 also included a field with this name, which was a String. Podman 2.2.0 added a new field reusing the name but as a Struct. This completely broke JSON decode for pre-1.8 containers in Podman 2.2, resulting in completely broken behavior. Re-name the JSON field and add a note that the old name should not be re-used to prevent this problem from re-occurring. This will still result in containers from 2.2.0 being broken (specifically, containers with image volumes will have them disappear) but this is the lesser of two evils. Fixes #8613 Signed-off-by: Matthew Heon --- libpod/container_config.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'libpod') diff --git a/libpod/container_config.go b/libpod/container_config.go index cc3ad25ea..c95be9b55 100644 --- a/libpod/container_config.go +++ b/libpod/container_config.go @@ -135,7 +135,13 @@ type ContainerRootFSConfig struct { // OverlayVolumes lists the overlay volumes to mount into the container. OverlayVolumes []*ContainerOverlayVolume `json:"overlayVolumes,omitempty"` // ImageVolumes lists the image volumes to mount into the container. - ImageVolumes []*ContainerImageVolume `json:"imageVolumes,omitempty"` + // Please note that this is named ctrImageVolumes in JSON to + // distinguish between these and the old `imageVolumes` field in Podman + // pre-1.8, which was used in very old Podman versions to determine how + // image volumes were handled in Libpod (support for these eventually + // moved out of Libpod into pkg/specgen). + // Please DO NOT re-use the `imageVolumes` name in container JSON again. + ImageVolumes []*ContainerImageVolume `json:"ctrImageVolumes,omitempty"` // CreateWorkingDir indicates that Libpod should create the container's // working directory if it does not exist. Some OCI runtimes do this by // default, but others do not. -- cgit v1.2.3-54-g00ecf From 4ceac05eacc37ee4cb0af03290c1c8a0f491e302 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 17 Nov 2020 16:46:11 +0100 Subject: container cgroup path Before querying for a container's cgroup path, make sure that the container is synced. Also make sure to error out if the container isn't running. Signed-off-by: Valentin Rothberg --- libpod/container.go | 25 ++++++++++++++++++++++--- libpod/stats.go | 2 +- 2 files changed, 23 insertions(+), 4 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index e954d84eb..c86625a4a 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -916,13 +916,33 @@ func (c *Container) CgroupManager() string { return cgroupManager } -// CGroupPath returns a cgroups "path" for a given container. +// CGroupPath returns a cgroups "path" for the given container. +// Note that the container must be running. Otherwise, an error +// is returned. func (c *Container) CGroupPath() (string, error) { + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + if err := c.syncContainer(); err != nil { + return "", errors.Wrapf(err, "error updating container %s state", c.ID()) + } + } + return c.cGroupPath() +} + +// cGroupPath returns a cgroups "path" for the given container. +// Note that the container must be running. Otherwise, an error +// is returned. +// NOTE: only call this when owning the container's lock. +func (c *Container) cGroupPath() (string, error) { if c.config.NoCgroups || c.config.CgroupsMode == "disabled" { return "", errors.Wrapf(define.ErrNoCgroups, "this container is not creating cgroups") } + if c.state.State != define.ContainerStateRunning && c.state.State != define.ContainerStatePaused { + return "", errors.Wrapf(define.ErrCtrStopped, "cannot get cgroup path unless container %s is running", c.ID()) + } - // Read /proc/[PID]/cgroup and find the *longest* cgroup entry. That's + // Read /proc/{PID}/cgroup and find the *longest* cgroup entry. That's // needed to account for hacks in cgroups v1, where each line in the // file could potentially point to a cgroup. The longest one, however, // is the libpod-specific one we're looking for. @@ -947,7 +967,6 @@ func (c *Container) CGroupPath() (string, error) { if len(path) > len(cgroupPath) { cgroupPath = path } - } if len(cgroupPath) == 0 { diff --git a/libpod/stats.go b/libpod/stats.go index e34739626..09d990017 100644 --- a/libpod/stats.go +++ b/libpod/stats.go @@ -34,7 +34,7 @@ func (c *Container) GetContainerStats(previousStats *define.ContainerStats) (*de return stats, define.ErrCtrStateInvalid } - cgroupPath, err := c.CGroupPath() + cgroupPath, err := c.cGroupPath() if err != nil { return nil, err } -- cgit v1.2.3-54-g00ecf From dca4444486a08b169862135ea5079856473f2c80 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 7 Dec 2020 10:00:36 -0500 Subject: Do not error on installing duplicate shutdown handler Installing a duplicate shutdown handler fails, but if a handler with the same name is already present, we should be set to go. There's no reason to print a user-facing error about it. This comes up almost nowhere because Podman never makes more than one Libpod runtime, but there is one exception (`system reset`) and the error messages, while harmless, were making people very confused (we got several bug reports that `system reset` was nonfunctional). Signed-off-by: Matthew Heon --- libpod/runtime.go | 2 +- libpod/shutdown/handler.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'libpod') diff --git a/libpod/runtime.go b/libpod/runtime.go index df3dfae2b..73d73fb5d 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -190,7 +190,7 @@ func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...R if err := shutdown.Register("libpod", func(sig os.Signal) error { os.Exit(1) return nil - }); err != nil { + }); err != nil && errors.Cause(err) != shutdown.ErrHandlerExists { logrus.Errorf("Error registering shutdown handler for libpod: %v", err) } diff --git a/libpod/shutdown/handler.go b/libpod/shutdown/handler.go index 87538dec9..f0f228b19 100644 --- a/libpod/shutdown/handler.go +++ b/libpod/shutdown/handler.go @@ -10,6 +10,10 @@ import ( "github.com/sirupsen/logrus" ) +var ( + ErrHandlerExists error = errors.New("handler with given name already exists") +) + var ( stopped bool sigChan chan os.Signal @@ -98,7 +102,7 @@ func Register(name string, handler func(os.Signal) error) error { } if _, ok := handlers[name]; ok { - return errors.Errorf("handler with name %s already exists", name) + return ErrHandlerExists } handlers[name] = handler -- cgit v1.2.3-54-g00ecf From fa9ad1671e9b3433c6b5fb0c393cebca70a0811b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 2 Dec 2020 10:05:34 -0500 Subject: Use Libpod tmpdir for pause path Previously, we always computed pause path from the Rootless runtime directory. Problem: this does not match the behavior of Libpod when the directory changes. Libpod will continue to use the previous directory, cached in the database; Pause pidfiles will swap to the new path. This is problematic when the directory needs to exist to write the pidfile, and Libpod is what creates the directory. There are two potential solutions - allow the pause pidfile to move and just make the directory when we want to write it, or use the cached Libpod paths for a guaranteed location. This patch does the second, because it seems safer - we will never miss a previously-existing pidfile because the location is now consistent. Fixes #8539 Signed-off-by: Matthew Heon --- libpod/reset.go | 2 +- libpod/runtime.go | 11 ++++++++++- libpod/runtime_migrate.go | 6 +++--- libpod/runtime_migrate_unsupported.go | 2 +- pkg/domain/infra/abi/system.go | 17 +++++++++++++---- pkg/util/utils_supported.go | 13 ++++++++++++- pkg/util/utils_windows.go | 6 ++++++ 7 files changed, 46 insertions(+), 11 deletions(-) (limited to 'libpod') diff --git a/libpod/reset.go b/libpod/reset.go index f8828fed4..6d2842723 100644 --- a/libpod/reset.go +++ b/libpod/reset.go @@ -46,7 +46,7 @@ func (r *Runtime) Reset(ctx context.Context) error { } } - if err := stopPauseProcess(); err != nil { + if err := r.stopPauseProcess(); err != nil { logrus.Errorf("Error stopping pause process: %v", err) } diff --git a/libpod/runtime.go b/libpod/runtime.go index 73d73fb5d..1f50f3407 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -472,7 +472,7 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (retErr error) { // we will need to access the storage. if os.Geteuid() != 0 { aliveLock.Unlock() // Unlock to avoid deadlock as BecomeRootInUserNS will reexec. - pausePid, err := util.GetRootlessPauseProcessPidPath() + pausePid, err := util.GetRootlessPauseProcessPidPathGivenDir(runtime.config.Engine.TmpDir) if err != nil { return errors.Wrapf(err, "could not get pause process pid file path") } @@ -538,6 +538,15 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (retErr error) { return nil } +// TmpDir gets the current Libpod temporary files directory. +func (r *Runtime) TmpDir() (string, error) { + if !r.valid { + return "", define.ErrRuntimeStopped + } + + return r.config.Engine.TmpDir, nil +} + // GetConfig returns a copy of the configuration used by the runtime func (r *Runtime) GetConfig() (*config.Config, error) { r.lock.RLock() diff --git a/libpod/runtime_migrate.go b/libpod/runtime_migrate.go index 1ad32fe9c..f0f800ef0 100644 --- a/libpod/runtime_migrate.go +++ b/libpod/runtime_migrate.go @@ -18,9 +18,9 @@ import ( "github.com/sirupsen/logrus" ) -func stopPauseProcess() error { +func (r *Runtime) stopPauseProcess() error { if rootless.IsRootless() { - pausePidPath, err := util.GetRootlessPauseProcessPidPath() + pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(r.config.Engine.TmpDir) if err != nil { return errors.Wrapf(err, "could not get pause process pid file path") } @@ -98,5 +98,5 @@ func (r *Runtime) migrate(ctx context.Context) error { } } - return stopPauseProcess() + return r.stopPauseProcess() } diff --git a/libpod/runtime_migrate_unsupported.go b/libpod/runtime_migrate_unsupported.go index e362cca63..a9d351318 100644 --- a/libpod/runtime_migrate_unsupported.go +++ b/libpod/runtime_migrate_unsupported.go @@ -10,6 +10,6 @@ func (r *Runtime) migrate(ctx context.Context) error { return nil } -func stopPauseProcess() error { +func (r *Runtime) stopPauseProcess() error { return nil } diff --git a/pkg/domain/infra/abi/system.go b/pkg/domain/infra/abi/system.go index 72fd98ac1..ec2532bea 100644 --- a/pkg/domain/infra/abi/system.go +++ b/pkg/domain/infra/abi/system.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/containers/common/pkg/config" + "github.com/containers/podman/v2/libpod" "github.com/containers/podman/v2/libpod/define" "github.com/containers/podman/v2/pkg/cgroups" "github.com/containers/podman/v2/pkg/domain/entities" @@ -86,7 +87,11 @@ func (ic *ContainerEngine) SetupRootless(_ context.Context, cmd *cobra.Command) return nil } - pausePidPath, err := util.GetRootlessPauseProcessPidPath() + tmpDir, err := ic.Libpod.TmpDir() + if err != nil { + return err + } + pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(tmpDir) if err != nil { return errors.Wrapf(err, "could not get pause process pid file path") } @@ -112,7 +117,7 @@ func (ic *ContainerEngine) SetupRootless(_ context.Context, cmd *cobra.Command) } became, ret, err = rootless.TryJoinFromFilePaths(pausePidPath, true, paths) - if err := movePauseProcessToScope(); err != nil { + if err := movePauseProcessToScope(ic.Libpod); err != nil { conf, err := ic.Config(context.Background()) if err != nil { return err @@ -133,8 +138,12 @@ func (ic *ContainerEngine) SetupRootless(_ context.Context, cmd *cobra.Command) return nil } -func movePauseProcessToScope() error { - pausePidPath, err := util.GetRootlessPauseProcessPidPath() +func movePauseProcessToScope(r *libpod.Runtime) error { + tmpDir, err := r.TmpDir() + if err != nil { + return err + } + pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(tmpDir) if err != nil { return errors.Wrapf(err, "could not get pause process pid file path") } diff --git a/pkg/util/utils_supported.go b/pkg/util/utils_supported.go index 2d636a7cb..a63c76415 100644 --- a/pkg/util/utils_supported.go +++ b/pkg/util/utils_supported.go @@ -99,7 +99,8 @@ func GetRootlessConfigHomeDir() (string, error) { } // GetRootlessPauseProcessPidPath returns the path to the file that holds the pid for -// the pause process +// the pause process. +// DEPRECATED - switch to GetRootlessPauseProcessPidPathGivenDir func GetRootlessPauseProcessPidPath() (string, error) { runtimeDir, err := GetRuntimeDir() if err != nil { @@ -107,3 +108,13 @@ func GetRootlessPauseProcessPidPath() (string, error) { } return filepath.Join(runtimeDir, "libpod", "pause.pid"), nil } + +// GetRootlessPauseProcessPidPathGivenDir returns the path to the file that +// holds the PID of the pause process, given the location of Libpod's temporary +// files. +func GetRootlessPauseProcessPidPathGivenDir(libpodTmpDir string) (string, error) { + if libpodTmpDir == "" { + return "", errors.Errorf("must provide non-empty tmporary directory") + } + return filepath.Join(libpodTmpDir, "pause.pid"), nil +} diff --git a/pkg/util/utils_windows.go b/pkg/util/utils_windows.go index 9bba2d1ee..46ca5e7f1 100644 --- a/pkg/util/utils_windows.go +++ b/pkg/util/utils_windows.go @@ -25,6 +25,12 @@ func GetRootlessPauseProcessPidPath() (string, error) { return "", errors.Wrap(errNotImplemented, "GetRootlessPauseProcessPidPath") } +// GetRootlessPauseProcessPidPath returns the path to the file that holds the pid for +// the pause process +func GetRootlessPauseProcessPidPathGivenDir(unused string) (string, error) { + return "", errors.Wrap(errNotImplemented, "GetRootlessPauseProcessPidPath") +} + // GetRuntimeDir returns the runtime directory func GetRuntimeDir() (string, error) { return "", errors.New("this function is not implemented for windows") -- cgit v1.2.3-54-g00ecf