Merge pull request containers#15437 from mheon/default_volume_timeout

Add support for containers.conf volume timeouts

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.0
-	github.com/containers/common v0.49.2-0.20220817132854-f6679f170eca
+	github.com/containers/common v0.49.2-0.20220823130605-72a7da3358ac
 	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

@@ -140,8 +140,9 @@ github.com/Microsoft/hcsshim v0.8.21/go.mod h1:+w2gRZ5ReXQhFOrvSQeNfhrYB/dg3oDwT
 github.com/Microsoft/hcsshim v0.8.22/go.mod h1:91uVCVzvX2QD16sMCenoxxXo6L1wJnLMX2PSufFMtF0=
 github.com/Microsoft/hcsshim v0.8.23/go.mod h1:4zegtUJth7lAvFyc6cH2gGQ5B3OFQim01nnU2M8jKDg=
 github.com/Microsoft/hcsshim v0.9.2/go.mod h1:7pLA8lDk46WKDWlVsENo92gC0XFa8rbKfyFRBqxEbCc=
-github.com/Microsoft/hcsshim v0.9.3 h1:k371PzBuRrz2b+ebGuI2nVgVhgsVX60jMfSw80NECxo=
 github.com/Microsoft/hcsshim v0.9.3/go.mod h1:7pLA8lDk46WKDWlVsENo92gC0XFa8rbKfyFRBqxEbCc=
+github.com/Microsoft/hcsshim v0.9.4 h1:mnUj0ivWy6UzbB1uLFqKR6F+ZyiDc7j4iGgHTpO+5+I=
+github.com/Microsoft/hcsshim v0.9.4/go.mod h1:7pLA8lDk46WKDWlVsENo92gC0XFa8rbKfyFRBqxEbCc=
 github.com/Microsoft/hcsshim/test v0.0.0-20201218223536-d3e5debf77da/go.mod h1:5hlzMzRKMLyo42nCZ9oml8AdTlq/0cvIaBv6tK1RehU=
 github.com/Microsoft/hcsshim/test v0.0.0-20210227013316-43a75bb4edd3/go.mod h1:mw7qgWloBUl75W/gVH3cQszUg1+gUITj7D6NY7ywVnY=
 github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ=
@@ -323,8 +324,9 @@ github.com/containerd/containerd v1.5.7/go.mod h1:gyvv6+ugqY25TiXxcZC3L5yOeYgEw0
 github.com/containerd/containerd v1.5.8/go.mod h1:YdFSv5bTFLpG2HIYmfqDpSYYTDX+mc5qtSuYx1YUb/s=
 github.com/containerd/containerd v1.5.9/go.mod h1:fvQqCfadDGga5HZyn3j4+dx56qj2I9YwBrlSdalvJYQ=
 github.com/containerd/containerd v1.6.1/go.mod h1:1nJz5xCZPusx6jJU8Frfct988y0NpumIq9ODB0kLtoE=
-github.com/containerd/containerd v1.6.6 h1:xJNPhbrmz8xAMDNoVjHy9YHtWwEQNS+CDkcIRh7t8Y0=
 github.com/containerd/containerd v1.6.6/go.mod h1:ZoP1geJldzCVY3Tonoz7b1IXk8rIX0Nltt5QE4OMNk0=
+github.com/containerd/containerd v1.6.8 h1:h4dOFDwzHmqFEP754PgfgTeVXFnLiRc6kiqC7tplDJs=
+github.com/containerd/containerd v1.6.8/go.mod h1:By6p5KqPK0/7/CgO/A6t/Gz+CUYUu2zf1hUaaymVXB0=
 github.com/containerd/continuity v0.0.0-20190426062206-aaeac12a7ffc/go.mod h1:GL3xCUCBDV3CZiTSEKksMWbLE66hEyuu9qyDOOqM47Y=
 github.com/containerd/continuity v0.0.0-20190815185530-f2a389ac0a02/go.mod h1:GL3xCUCBDV3CZiTSEKksMWbLE66hEyuu9qyDOOqM47Y=
 github.com/containerd/continuity v0.0.0-20191127005431-f65d91d395eb/go.mod h1:GL3xCUCBDV3CZiTSEKksMWbLE66hEyuu9qyDOOqM47Y=
@@ -395,8 +397,8 @@ github.com/containernetworking/plugins v1.1.1/go.mod h1:Sr5TH/eBsGLXK/h71HeLfX19
 github.com/containers/buildah v1.27.0 h1:LJ1ks7vKxwPzJGr5BWVvigbtVL9w7XeHtNEmiIOPJqI=
 github.com/containers/buildah v1.27.0/go.mod h1:anH3ExvDXRNP9zLQCrOc1vWb5CrhqLF/aYFim4tslvA=
 github.com/containers/common v0.49.1/go.mod h1:ueM5hT0itKqCQvVJDs+EtjornAQtrHYxQJzP2gxeGIg=
-github.com/containers/common v0.49.2-0.20220817132854-f6679f170eca h1:OjhEBVpFskIJ6Vq9nikYW7M6YXfkTxOBu+EQBoCyhuM=
-github.com/containers/common v0.49.2-0.20220817132854-f6679f170eca/go.mod h1:eT2iSsNzjOlF5VFLkyj9OU2SXznURvEYndsioQImuoE=
+github.com/containers/common v0.49.2-0.20220823130605-72a7da3358ac h1:rLbTzosxPKrQd+EgMRxfC1WYm3azPiQfig+Lr7mCQ4k=
+github.com/containers/common v0.49.2-0.20220823130605-72a7da3358ac/go.mod h1:xC4qkLfW9R+YSDknlT9xU+NDNxIw017U8AyohGtr9Ec=
 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

@@ -1695,14 +1695,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

@@ -1097,7 +1097,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)
 }