Add support for containers.conf volume timeouts

Also, do a general cleanup of all the timeout code. Changes
include:
- Convert from int to *uint where possible. Timeouts cannot be
  negative, hence the uint change; and a timeout of 0 is valid,
  so we need a new way to detect that the user set a timeout
  (hence, pointer).
- Change name in the database to avoid conflicts between new data
  type and old one. This will cause timeouts set with 4.2.0 to be
  lost, but considering nobody is using the feature at present
  (and the lack of validation means we could have invalid,
  negative timeouts in the DB) this feels safe.
- Ensure volume plugin timeouts can only be used with volumes
  created using a plugin. Timeouts on the local driver are
  nonsensical.
- Remove the existing test, as it did not use a volume plugin.
  Write a new test that does.

The actual plumbing of the containers.conf timeout in is one line
in volume_api.go; the remainder are the above-described cleanups.

Backported to v4.2.0-rhel per RHBZ 2125241

Signed-off-by: Matthew Heon <mheon@redhat.com>

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/containernetworking/cni v1.1.2
 	github.com/containernetworking/plugins v1.1.1
 	github.com/containers/buildah v1.27.1
-	github.com/containers/common v0.49.1
+	github.com/containers/common v0.49.2
 	github.com/containers/conmon v2.0.20+incompatible
 	github.com/containers/image/v5 v5.22.0
 	github.com/containers/ocicrypt v1.1.5

go.sum

@@ -394,8 +394,9 @@ github.com/containernetworking/plugins v1.1.1 h1:+AGfFigZ5TiQH00vhR8qPeSatj53eNG
 github.com/containernetworking/plugins v1.1.1/go.mod h1:Sr5TH/eBsGLXK/h71HeLfX19sZPp3ry5uHSkI4LPxV8=
 github.com/containers/buildah v1.27.1 h1:i5yP3uJBq9mKANOP4WA+5x9cBuEQ4FJIAzEPoPzRrXQ=
 github.com/containers/buildah v1.27.1/go.mod h1:anH3ExvDXRNP9zLQCrOc1vWb5CrhqLF/aYFim4tslvA=
-github.com/containers/common v0.49.1 h1:6y4/s2WwYxrv+Cox7fotOo316wuZI+iKKPUQweCYv50=
 github.com/containers/common v0.49.1/go.mod h1:ueM5hT0itKqCQvVJDs+EtjornAQtrHYxQJzP2gxeGIg=
+github.com/containers/common v0.49.2 h1:gmMScREi9yXItb8Za47FMXol09Gx3hyzw7+ETPX7rao=
+github.com/containers/common v0.49.2/go.mod h1:ueM5hT0itKqCQvVJDs+EtjornAQtrHYxQJzP2gxeGIg=
 github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg=
 github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I=
 github.com/containers/image/v5 v5.22.0 h1:KemxPmD4D2YYOFZN2SgoTk7nBFcnwPiPW0MqjYtknSE=

libpod/define/volume_inspect.go

@@ -57,7 +57,7 @@ type InspectVolumeData struct {
 	// UID/GID.
 	NeedsChown bool `json:"NeedsChown,omitempty"`
 	// Timeout is the specified driver timeout if given
-	Timeout int `json:"Timeout,omitempty"`
+	Timeout uint `json:"Timeout,omitempty"`
 }
 
 type VolumeReload struct {

libpod/options.go

@@ -1704,14 +1704,22 @@ func withSetAnon() VolumeCreateOption {
 	}
 }
 
-// WithVolumeDriverTimeout sets the volume creation timeout period
-func WithVolumeDriverTimeout(timeout int) VolumeCreateOption {
+// WithVolumeDriverTimeout sets the volume creation timeout period.
+// Only usable if a non-local volume driver is in use.
+func WithVolumeDriverTimeout(timeout uint) VolumeCreateOption {
 	return func(volume *Volume) error {
 		if volume.valid {
 			return define.ErrVolumeFinalized
 		}
 
-		volume.config.Timeout = timeout
+		if volume.config.Driver == "" || volume.config.Driver == define.VolumeDriverLocal {
+			return fmt.Errorf("Volume driver timeout can only be used with non-local volume drivers: %w", define.ErrInvalidArg)
+		}
+
+		tm := timeout
+
+		volume.config.Timeout = &tm
+
 		return nil
 	}
 }

libpod/plugin/volume_api.go

@@ -3,6 +3,7 @@ package plugin
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -13,8 +14,7 @@ import (
 	"sync"
 	"time"
 
-	"errors"
-
+	"github.com/containers/common/pkg/config"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/docker/go-plugins-helpers/sdk"
 	"github.com/docker/go-plugins-helpers/volume"
@@ -40,7 +40,6 @@ var (
 )
 
 const (
-	defaultTimeout   = 5 * time.Second
 	volumePluginType = "VolumeDriver"
 )
 
@@ -129,7 +128,7 @@ func validatePlugin(newPlugin *VolumePlugin) error {
 
 // GetVolumePlugin gets a single volume plugin, with the given name, at the
 // given path.
-func GetVolumePlugin(name string, path string, timeout int) (*VolumePlugin, error) {
+func GetVolumePlugin(name string, path string, timeout *uint, cfg *config.Config) (*VolumePlugin, error) {
 	pluginsLock.Lock()
 	defer pluginsLock.Unlock()
 
@@ -152,13 +151,11 @@ func GetVolumePlugin(name string, path string, timeout int) (*VolumePlugin, erro
 	// Need an HTTP client to force a Unix connection.
 	// And since we can reuse it, might as well cache it.
 	client := new(http.Client)
-	client.Timeout = defaultTimeout
-	// if the user specified a non-zero timeout, use their value. Else, keep the default.
-	if timeout != 0 {
-		if time.Duration(timeout)*time.Second < defaultTimeout {
-			logrus.Warnf("the default timeout for volume creation is %d seconds, setting a time less than that may break this feature.", defaultTimeout)
-		}
-		client.Timeout = time.Duration(timeout) * time.Second
+	client.Timeout = 5 * time.Second
+	if timeout != nil {
+		client.Timeout = time.Duration(*timeout) * time.Second
+	} else if cfg != nil {
+		client.Timeout = time.Duration(cfg.Engine.VolumePluginTimeout) * time.Second
 	}
 	// This bit borrowed from pkg/bindings/connection.go
 	client.Transport = &http.Transport{

libpod/runtime.go

@@ -1208,7 +1208,7 @@ func (r *Runtime) getVolumePlugin(volConfig *VolumeConfig) (*plugin.VolumePlugin
 		return nil, fmt.Errorf("no volume plugin with name %s available: %w", name, define.ErrMissingPlugin)
 	}
 
-	return plugin.GetVolumePlugin(name, pluginPath, timeout)
+	return plugin.GetVolumePlugin(name, pluginPath, timeout, r.config)
 }
 
 // GetSecretsStorageDir returns the directory that the secrets manager should take

libpod/runtime_volume_linux.go

@@ -184,7 +184,7 @@ func (r *Runtime) UpdateVolumePlugins(ctx context.Context) *define.VolumeReload
 	)
 
 	for driverName, socket := range r.config.Engine.VolumePlugins {
-		driver, err := volplugin.GetVolumePlugin(driverName, socket, 0)
+		driver, err := volplugin.GetVolumePlugin(driverName, socket, nil, r.config)
 		if err != nil {
 			errs = append(errs, err)
 			continue

libpod/volume.go

@@ -56,7 +56,7 @@ type VolumeConfig struct {
 	// quota tracking.
 	DisableQuota bool `json:"disableQuota,omitempty"`
 	// Timeout allows users to override the default driver timeout of 5 seconds
-	Timeout int
+	Timeout *uint `json:"timeout,omitempty"`
 }
 
 // VolumeState holds the volume's mutable state.

libpod/volume_inspect.go

@@ -64,7 +64,12 @@ func (v *Volume) Inspect() (*define.InspectVolumeData, error) {
 	data.MountCount = v.state.MountCount
 	data.NeedsCopyUp = v.state.NeedsCopyUp
 	data.NeedsChown = v.state.NeedsChown
-	data.Timeout = v.config.Timeout
+
+	if v.config.Timeout != nil {
+		data.Timeout = *v.config.Timeout
+	} else if v.UsesVolumeDriver() {
+		data.Timeout = v.runtime.config.Engine.VolumePluginTimeout
+	}
 
 	return data, nil
 }

pkg/domain/infra/abi/parse/parse.go

@@ -86,8 +86,11 @@ func VolumeOptions(opts map[string]string) ([]libpod.VolumeCreateOption, error)
 					if err != nil {
 						return nil, fmt.Errorf("cannot convert Timeout %s to an integer: %w", splitO[1], err)
 					}
+					if intTimeout < 0 {
+						return nil, fmt.Errorf("volume timeout cannot be negative (got %d)", intTimeout)
+					}
 					logrus.Debugf("Removing timeout from options and adding WithTimeout for Timeout %d", intTimeout)
-					libpodOptions = append(libpodOptions, libpod.WithVolumeDriverTimeout(intTimeout))
+					libpodOptions = append(libpodOptions, libpod.WithVolumeDriverTimeout(uint(intTimeout)))
 				default:
 					finalVal = append(finalVal, o)
 				}

test/e2e/config/containers.conf

@@ -61,6 +61,8 @@ no_hosts=true
 network_cmd_options=["allow_host_loopback=true"]
 service_timeout=1234
 
+volume_plugin_timeout = 15
+
 # We need to ensure each test runs on a separate plugin instance...
 # For now, let's just make a bunch of plugin paths and have each test use one.
 [engine.volume_plugins]

test/e2e/volume_create_test.go

@@ -162,19 +162,4 @@ var _ = Describe("Podman volume create", func() {
 		Expect(inspectOpts).Should(Exit(0))
 		Expect(inspectOpts.OutputToString()).To(Equal(optionStrFormatExpect))
 	})
-
-	It("podman create volume with o=timeout", func() {
-		volName := "testVol"
-		timeout := 10
-		timeoutStr := "10"
-		session := podmanTest.Podman([]string{"volume", "create", "--opt", fmt.Sprintf("o=timeout=%d", timeout), volName})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(Exit(0))
-
-		inspectTimeout := podmanTest.Podman([]string{"volume", "inspect", "--format", "{{ .Timeout }}", volName})
-		inspectTimeout.WaitWithDefaultTimeout()
-		Expect(inspectTimeout).Should(Exit(0))
-		Expect(inspectTimeout.OutputToString()).To(Equal(timeoutStr))
-
-	})
 })

test/e2e/volume_plugin_test.go

@@ -256,4 +256,38 @@ Removed:
 		Expect(session.OutputToStringArray()).To(ContainElements(localvol, vol2))
 		Expect(session.ErrorToString()).To(Equal("")) // make no errors are shown
 	})
+
+	It("volume driver timeouts test", func() {
+		podmanTest.AddImageToRWStore(volumeTest)
+
+		pluginStatePath := filepath.Join(podmanTest.TempDir, "volumes")
+		err := os.Mkdir(pluginStatePath, 0755)
+		Expect(err).ToNot(HaveOccurred())
+
+		// Keep this distinct within tests to avoid multiple tests using the same plugin.
+		pluginName := "testvol6"
+		plugin := podmanTest.Podman([]string{"run", "--security-opt", "label=disable", "-v", "/run/docker/plugins:/run/docker/plugins", "-v", fmt.Sprintf("%v:%v", pluginStatePath, pluginStatePath), "-d", volumeTest, "--sock-name", pluginName, "--path", pluginStatePath})
+		plugin.WaitWithDefaultTimeout()
+		Expect(plugin).Should(Exit(0))
+
+		volName := "testVolume1"
+		create := podmanTest.Podman([]string{"volume", "create", "--driver", pluginName, volName})
+		create.WaitWithDefaultTimeout()
+		Expect(create).Should(Exit(0))
+
+		volInspect := podmanTest.Podman([]string{"volume", "inspect", "--format", "{{ .Timeout }}", volName})
+		volInspect.WaitWithDefaultTimeout()
+		Expect(volInspect).Should(Exit(0))
+		Expect(volInspect.OutputToString()).To(ContainSubstring("15"))
+
+		volName2 := "testVolume2"
+		create2 := podmanTest.Podman([]string{"volume", "create", "--driver", pluginName, "--opt", "o=timeout=3", volName2})
+		create2.WaitWithDefaultTimeout()
+		Expect(create2).Should(Exit(0))
+
+		volInspect2 := podmanTest.Podman([]string{"volume", "inspect", "--format", "{{ .Timeout }}", volName2})
+		volInspect2.WaitWithDefaultTimeout()
+		Expect(volInspect2).Should(Exit(0))
+		Expect(volInspect2.OutputToString()).To(ContainSubstring("3"))
+	})
 })

test/testvol/util.go

@@ -25,5 +25,5 @@ func getPluginName(pathOrName string) string {
 func getPlugin(sockNameOrPath string) (*plugin.VolumePlugin, error) {
 	path := getSocketPath(sockNameOrPath)
 	name := getPluginName(sockNameOrPath)
-	return plugin.GetVolumePlugin(name, path, 0)
+	return plugin.GetVolumePlugin(name, path, nil, nil)
 }

vendor/github.com/containers/common/libimage/load.go

@@ -99,7 +99,7 @@ func (r *Runtime) Load(ctx context.Context, path string, options *LoadOptions) (
 }
 
 // loadMultiImageDockerArchive loads the docker archive specified by ref.  In
-// case the path@reference notation was used, only the specifiec image will be
+// case the path@reference notation was used, only the specified image will be
 // loaded.  Otherwise, all images will be loaded.
 func (r *Runtime) loadMultiImageDockerArchive(ctx context.Context, ref types.ImageReference, options *CopyOptions) ([]string, error) {
 	// If we cannot stat the path, it either does not exist OR the correct

vendor/github.com/containers/common/libnetwork/cni/network.go

@@ -19,6 +19,7 @@ import (
 	"github.com/containers/common/pkg/config"
 	"github.com/containers/storage/pkg/lockfile"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/unix"
 )
 
 type cniNetwork struct {
@@ -62,6 +63,8 @@ type InitConfig struct {
 	CNIConfigDir string
 	// CNIPluginDirs is a list of directories where cni should look for the plugins.
 	CNIPluginDirs []string
+	// RunDir is a directory where temporary files can be stored.
+	RunDir string
 
 	// DefaultNetwork is the name for the default network.
 	DefaultNetwork string
@@ -81,7 +84,16 @@ func NewCNINetworkInterface(conf *InitConfig) (types.ContainerNetwork, error) {
 	// TODO: consider using a shared memory lock
 	lock, err := lockfile.GetLockfile(filepath.Join(conf.CNIConfigDir, "cni.lock"))
 	if err != nil {
-		return nil, err
+		// If we're on a read-only filesystem, there is no risk of
+		// contention. Fall back to a local lockfile.
+		if errors.Is(err, unix.EROFS) {
+			lock, err = lockfile.GetLockfile(filepath.Join(conf.RunDir, "cni.lock"))
+			if err != nil {
+				return nil, err
+			}
+		} else {
+			return nil, err
+		}
 	}
 
 	defaultNetworkName := conf.DefaultNetwork

vendor/github.com/containers/common/libnetwork/network/interface.go

@@ -169,6 +169,7 @@ func getCniInterface(conf *config.Config) (types.ContainerNetwork, error) {
 	return cni.NewCNINetworkInterface(&cni.InitConfig{
 		CNIConfigDir:       confDir,
 		CNIPluginDirs:      conf.Network.CNIPluginDirs,
+		RunDir:             conf.Engine.TmpDir,
 		DefaultNetwork:     conf.Network.DefaultNetwork,
 		DefaultSubnet:      conf.Network.DefaultSubnet,
 		DefaultsubnetPools: conf.Network.DefaultSubnetPools,

vendor/github.com/containers/common/pkg/config/config.go

@@ -447,6 +447,13 @@ type EngineConfig struct {
 	// may not be by other drivers.
 	VolumePath string `toml:"volume_path,omitempty"`
 
+	// VolumePluginTimeout sets the default timeout, in seconds, for
+	// operations that must contact a volume plugin. Plugins are external
+	// programs accessed via REST API; this sets a timeout for requests to
+	// that API.
+	// A value of 0 is treated as no timeout.
+	VolumePluginTimeout uint `toml:"volume_plugin_timeout,omitempty,omitzero"`
+
 	// VolumePlugins is a set of plugins that can be used as the backend for
 	// Podman named volumes. Each volume is specified as a name (what Podman
 	// will refer to the plugin as) mapped to a path, which must point to a

vendor/github.com/containers/common/pkg/config/containers.conf

@@ -605,6 +605,12 @@ default_sysctls = [
 #
 #volume_path = "/var/lib/containers/storage/volumes"
 
+# Default timeout (in seconds) for volume plugin operations.
+# Plugins are external programs accessed via a REST API; this sets a timeout
+# for requests to that API.
+# A value of 0 is treated as no timeout.
+#volume_plugin_timeout = 5
+
 # Paths to look for a valid OCI runtime (crun, runc, kata, runsc, krun, etc)
 [engine.runtimes]
 #crun = [

vendor/github.com/containers/common/pkg/config/default.go

@@ -159,6 +159,8 @@ const (
 	SeccompOverridePath = _etcDir + "/containers/seccomp.json"
 	// SeccompDefaultPath defines the default seccomp path.
 	SeccompDefaultPath = _installPrefix + "/share/containers/seccomp.json"
+	// DefaultVolumePluginTimeout is the default volume plugin timeout, in seconds
+	DefaultVolumePluginTimeout = 5
 )
 
 // DefaultConfig defines the default values from containers.conf.
@@ -292,6 +294,8 @@ func defaultConfigFromMemory() (*EngineConfig, error) {
 	c.StaticDir = filepath.Join(storeOpts.GraphRoot, "libpod")
 	c.VolumePath = filepath.Join(storeOpts.GraphRoot, "volumes")
 
+	c.VolumePluginTimeout = DefaultVolumePluginTimeout
+
 	c.HelperBinariesDir = defaultHelperBinariesDir
 	if additionalHelperBinariesDir != "" {
 		c.HelperBinariesDir = append(c.HelperBinariesDir, additionalHelperBinariesDir)

vendor/github.com/containers/common/pkg/subscriptions/subscriptions.go

@@ -372,7 +372,7 @@ func mountExists(mounts []rspec.Mount, dest string) bool {
 	return false
 }
 
-// resolveSymbolicLink resolves a possbile symlink path. If the path is a symlink, returns resolved
+// resolveSymbolicLink resolves symlink paths. If the path is a symlink, returns resolved
 // path; if not, returns the original path.
 func resolveSymbolicLink(path string) (string, error) {
 	info, err := os.Lstat(path)

vendor/github.com/containers/common/version/version.go

@@ -1,4 +1,4 @@
 package version
 
 // Version is the version of the build.
-const Version = "0.49.1"
+const Version = "0.49.2"

vendor/modules.txt

@@ -114,7 +114,7 @@ github.com/containers/buildah/pkg/rusage
 github.com/containers/buildah/pkg/sshagent
 github.com/containers/buildah/pkg/util
 github.com/containers/buildah/util
-# github.com/containers/common v0.49.1
+# github.com/containers/common v0.49.2
 ## explicit
 github.com/containers/common/libimage
 github.com/containers/common/libimage/define