From 1fc82393323fcdca45d0291babae1df05e7b49ff Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 20 Nov 2019 15:43:15 +0100 Subject: [PATCH 1/6] config: use EventsLogger=file without systemd if systemd is not available, use the file events logger backend. Signed-off-by: Giuseppe Scrivano --- libpod/config/config.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/libpod/config/config.go b/libpod/config/config.go index 5b4b57f3a4..f1fa70fbc8 100644 --- a/libpod/config/config.go +++ b/libpod/config/config.go @@ -469,6 +469,9 @@ func NewConfig(userConfigPath string) (*Config, error) { if defaultConfig, err := defaultConfigFromMemory(); err != nil { return nil, errors.Wrapf(err, "error generating default config from memory") } else { + // Check if we need to switch to cgroupfs and logger=file on rootless. + defaultConfig.checkCgroupsAndLogger() + if err := config.mergeConfig(defaultConfig); err != nil { return nil, errors.Wrapf(err, "error merging default config from memory") } @@ -487,9 +490,6 @@ func NewConfig(userConfigPath string) (*Config, error) { return nil, errors.Wrapf(define.ErrInvalidArg, "volume path must be an absolute path - instead got %q", config.VolumePath) } - // Check if we need to switch to cgroupfs on rootless. - config.checkCgroupsAndAdjustConfig() - return config, nil } @@ -524,11 +524,13 @@ func systemConfigs() ([]string, error) { return configs, nil } -// checkCgroupsAndAdjustConfig checks if we're running rootless with the systemd +// checkCgroupsAndLogger checks if we're running rootless with the systemd // cgroup manager. In case the user session isn't available, we're switching the -// cgroup manager to cgroupfs. Note, this only applies to rootless. -func (c *Config) checkCgroupsAndAdjustConfig() { - if !rootless.IsRootless() || c.CgroupManager != define.SystemdCgroupsManager { +// cgroup manager to cgroupfs and the events logger backend to 'file'. +// Note, this only applies to rootless. +func (c *Config) checkCgroupsAndLogger() { + if !rootless.IsRootless() || (c.CgroupManager != + define.SystemdCgroupsManager && c.EventsLogger == "file") { return } @@ -543,7 +545,8 @@ func (c *Config) checkCgroupsAndAdjustConfig() { logrus.Warningf("The cgroups manager is set to systemd but there is no systemd user session available") logrus.Warningf("For using systemd, you may need to login using an user session") logrus.Warningf("Alternatively, you can enable lingering with: `loginctl enable-linger %d` (possibly as root)", rootless.GetRootlessUID()) - logrus.Warningf("Falling back to --cgroup-manager=cgroupfs") + logrus.Warningf("Falling back to --cgroup-manager=cgroupfs and --events-backend=file") c.CgroupManager = define.CgroupfsCgroupsManager + c.EventsLogger = "file" } } From 1368eaf1ba88b26c612f3fec809584c686cdd596 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 6 Nov 2019 17:14:44 +0100 Subject: [PATCH 2/6] events: make sure the write channel is always closed in case of errors, the channel is not closed, blocking the reader indefinitely. Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1767663 Signed-off-by: Giuseppe Scrivano --- libpod/events/journal_linux.go | 2 +- libpod/events/logfile.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libpod/events/journal_linux.go b/libpod/events/journal_linux.go index 470c76959c..9e6fffc296 100644 --- a/libpod/events/journal_linux.go +++ b/libpod/events/journal_linux.go @@ -54,6 +54,7 @@ func (e EventJournalD) Write(ee Event) error { // Read reads events from the journal and sends qualified events to the event channel func (e EventJournalD) Read(options ReadOptions) error { + defer close(options.EventChannel) eventOptions, err := generateEventOptions(options.Filters, options.Since, options.Until) if err != nil { return errors.Wrapf(err, "failed to generate event options") @@ -87,7 +88,6 @@ func (e EventJournalD) Read(options ReadOptions) error { if err != nil { return err } - defer close(options.EventChannel) for { if _, err := j.Next(); err != nil { return err diff --git a/libpod/events/logfile.go b/libpod/events/logfile.go index 4b65b0ad01..93e6fa3c9e 100644 --- a/libpod/events/logfile.go +++ b/libpod/events/logfile.go @@ -41,6 +41,7 @@ func (e EventLogFile) Write(ee Event) error { // Reads from the log file func (e EventLogFile) Read(options ReadOptions) error { + defer close(options.EventChannel) eventOptions, err := generateEventOptions(options.Filters, options.Since, options.Until) if err != nil { return errors.Wrapf(err, "unable to generate event options") @@ -68,7 +69,6 @@ func (e EventLogFile) Read(options ReadOptions) error { options.EventChannel <- event } } - close(options.EventChannel) return nil } From 0436cf29e57b3118c6fa22597240731913556a6f Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 2 Dec 2019 23:06:00 -0500 Subject: [PATCH 3/6] Ensure volumes reacquire locks on state refresh After a restart, pods and containers both run a refresh() function to prepare to run after a reboot. Until now, volumes have not had a similar function, because they had no per-boot setup to perform. Unfortunately, this was not noticed when in-memory locking was introduced to volumes. The refresh() routine is, among other things, responsible for ensuring that locks are reserved after a reboot, ensuring they cannot be taken by a freshly-created container, pod, or volume. If this reservation is not done, we can end up with two objects using the same lock, potentially needing to lock each other for some operations - classic recipe for deadlocks. Add a refresh() function to volumes to perform lock reservation and ensure it is called as part of overall refresh(). Fixes #4605 Fixes #4621 Signed-off-by: Matthew Heon --- libpod/runtime.go | 18 ++++++++++++++---- libpod/volume_internal.go | 12 ++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/libpod/runtime.go b/libpod/runtime.go index 42e6782e96..3873079ce0 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -625,7 +625,8 @@ func (r *Runtime) refresh(alivePath string) error { } // Next refresh the state of all containers to recreate dirs and - // namespaces, and all the pods to recreate cgroups + // namespaces, and all the pods to recreate cgroups. + // Containers, pods, and volumes must also reacquire their locks. ctrs, err := r.state.AllContainers() if err != nil { return errors.Wrapf(err, "error retrieving all containers from state") @@ -634,10 +635,14 @@ func (r *Runtime) refresh(alivePath string) error { if err != nil { return errors.Wrapf(err, "error retrieving all pods from state") } - // No locks are taken during pod and container refresh. - // Furthermore, the pod and container refresh() functions are not + vols, err := r.state.AllVolumes() + if err != nil { + return errors.Wrapf(err, "error retrieving all volumes from state") + } + // No locks are taken during pod, volume, and container refresh. + // Furthermore, the pod/volume/container refresh() functions are not // allowed to take locks themselves. - // We cannot assume that any pod or container has a valid lock until + // We cannot assume that any pod/volume/container has a valid lock until // after this function has returned. // The runtime alive lock should suffice to provide mutual exclusion // until this has run. @@ -651,6 +656,11 @@ func (r *Runtime) refresh(alivePath string) error { logrus.Errorf("Error refreshing pod %s: %v", pod.ID(), err) } } + for _, vol := range vols { + if err := vol.refresh(); err != nil { + logrus.Errorf("Error refreshing volume %s: %v", vol.Name(), err) + } + } // Create a file indicating the runtime is alive and ready file, err := os.OpenFile(alivePath, os.O_RDONLY|os.O_CREATE, 0644) diff --git a/libpod/volume_internal.go b/libpod/volume_internal.go index 42b935e7ca..e89b3484d9 100644 --- a/libpod/volume_internal.go +++ b/libpod/volume_internal.go @@ -5,6 +5,7 @@ import ( "path/filepath" "github.com/containers/libpod/libpod/define" + "github.com/pkg/errors" ) // Creates a new volume @@ -46,3 +47,14 @@ func (v *Volume) update() error { func (v *Volume) save() error { return v.runtime.state.SaveVolume(v) } + +// Refresh volume state after a restart. +func (v *Volume) refresh() error { + lock, err := v.runtime.lockManager.AllocateAndRetrieveLock(v.config.LockID) + if err != nil { + return errors.Wrapf(err, "error acquiring lock %d for volume %s", v.config.LockID, v.Name()) + } + v.lock = lock + + return nil +} From a05ee7e7e38accc86d3105742de91f0e29b4480d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 6 Nov 2019 08:52:10 +0100 Subject: [PATCH 4/6] help message: don't parse the config for cgroup-manager default Do not generate an entire `config.Config` for displaying the default value for the --cgroup-manager flag and just default to systemd. Not using the `config.Config` is okay as 1) the value may change at runtime in any case (rootless, DBUS access, etc.), 2) it avoids to redundantly parse the system config files and to generate the hard-coded default config, and 3) the log-level and other attributes are not yet set during init() causing undesirable side effects. Fixes: #4456 Signed-off-by: Valentin Rothberg --- cmd/podman/main_local.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cmd/podman/main_local.go b/cmd/podman/main_local.go index 6057eeec3b..f630f12104 100644 --- a/cmd/podman/main_local.go +++ b/cmd/podman/main_local.go @@ -16,7 +16,6 @@ import ( "github.com/containers/libpod/cmd/podman/cliconfig" "github.com/containers/libpod/cmd/podman/libpodruntime" - "github.com/containers/libpod/libpod/config" "github.com/containers/libpod/libpod/define" "github.com/containers/libpod/pkg/cgroups" "github.com/containers/libpod/pkg/rootless" @@ -34,9 +33,6 @@ const remote = false func init() { cgroupManager := define.SystemdCgroupsManager - if runtimeConfig, err := config.NewConfig(""); err == nil { - cgroupManager = runtimeConfig.CgroupManager - } cgroupHelp := "Cgroup manager to use (cgroupfs or systemd)" cgroupv2, _ := cgroups.IsCgroup2UnifiedMode() if rootless.IsRootless() && !cgroupv2 { From cc95786d06e2b9fa1c0bd93f23d65953cc7a57c8 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Fri, 15 Nov 2019 12:44:15 -0500 Subject: [PATCH 5/6] Also delete winsz fifo In conmon 2.0.3, we add another fifo to handle window resizing. This needs to be cleaned up for commands like restore, where the same path is used. Signed-off-by: Peter Hunt --- libpod/container_internal.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index c0a6960cd3..75bd90f364 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -652,6 +652,11 @@ func (c *Container) removeConmonFiles() error { return errors.Wrapf(err, "error removing container %s ctl file", c.ID()) } + winszFile := filepath.Join(c.bundlePath(), "winsz") + if err := os.Remove(winszFile); err != nil && !os.IsNotExist(err) { + return errors.Wrapf(err, "error removing container %s winsz file", c.ID()) + } + oomFile := filepath.Join(c.bundlePath(), "oom") if err := os.Remove(oomFile); err != nil && !os.IsNotExist(err) { return errors.Wrapf(err, "error removing container %s OOM file", c.ID()) From ba00eb5eb9113d120ea85771bf67c9dec2b1eca5 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 10 Dec 2019 11:15:37 -0500 Subject: [PATCH 6/6] Stop testing on F29 We disabled it upstream, so images likely don't exist anymore. Signed-off-by: Matthew Heon --- .cirrus.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index d5e221a4f5..b29d4269c2 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -32,7 +32,7 @@ env: ### _BUILT_IMAGE_SUFFIX: "libpod-5642998972416000" FEDORA_CACHE_IMAGE_NAME: "fedora-30-${_BUILT_IMAGE_SUFFIX}" - PRIOR_FEDORA_CACHE_IMAGE_NAME: "fedora-29-${_BUILT_IMAGE_SUFFIX}" + # PRIOR_FEDORA_CACHE_IMAGE_NAME: "fedora-29-${_BUILT_IMAGE_SUFFIX}" SPECIAL_FEDORA_CACHE_IMAGE_NAME: "xfedora-30-${_BUILT_IMAGE_SUFFIX}" UBUNTU_CACHE_IMAGE_NAME: "ubuntu-19-${_BUILT_IMAGE_SUFFIX}" PRIOR_UBUNTU_CACHE_IMAGE_NAME: "ubuntu-18-${_BUILT_IMAGE_SUFFIX}" @@ -273,7 +273,7 @@ meta_task: # Space-separated list of images used by this repository state IMGNAMES: >- ${FEDORA_CACHE_IMAGE_NAME} - ${PRIOR_FEDORA_CACHE_IMAGE_NAME} + # ${PRIOR_FEDORA_CACHE_IMAGE_NAME} ${SPECIAL_FEDORA_CACHE_IMAGE_NAME} ${UBUNTU_CACHE_IMAGE_NAME} ${PRIOR_UBUNTU_CACHE_IMAGE_NAME} @@ -332,7 +332,7 @@ testing_task: matrix: # Images are generated separately, from build_images_task (below) image_name: "${FEDORA_CACHE_IMAGE_NAME}" - image_name: "${PRIOR_FEDORA_CACHE_IMAGE_NAME}" + # image_name: "${PRIOR_FEDORA_CACHE_IMAGE_NAME}" # Multiple test failures on Ubuntu 19 - Fixes TBD in future PR # TODO: image_name: "${UBUNTU_CACHE_IMAGE_NAME}" image_name: "${PRIOR_UBUNTU_CACHE_IMAGE_NAME}"