Merge pull request #1696 from sboeuf/issue_1695

oci: Define a minimal timeout for WaitContainerStateStopped()

cmd/crio/config.go

@@ -193,6 +193,10 @@ uid_mappings = "{{ .UIDMappings }}"
 # ranges are separed by comma.
 gid_mappings = "{{ .GIDMappings }}"
 
+# ctr_stop_timeout specifies the time to wait before to generate an error
+# because the container state is still tagged as "running".
+ctr_stop_timeout = {{ .CtrStopTimeout }}
+
 [crio.image]
 
 # default_transport is the prefix we try prepending to an image name if the

lib/config.go

@@ -212,6 +212,10 @@ type RuntimeConfig struct {
 	// LogLevel determines the verbosity of the logs based on the level it is set to.
 	// Options are fatal, panic, error (default), warn, info, and debug.
 	LogLevel string `toml:"log_level"`
+
+	// CtrStopTimeout specifies the time to wait before to generate an
+	// error because the container state is still tagged as "running".
+	CtrStopTimeout int64 `toml:"ctr_stop_timeout"`
 }
 
 // ImageConfig represents the "crio.image" TOML config table.

lib/container_server.go

@@ -129,7 +129,7 @@ func New(ctx context.Context, config *Config) (*ContainerServer, error) {
 		return nil, err
 	}
 
-	runtime, err := oci.New(config.Runtime, config.RuntimeUntrustedWorkload, config.DefaultWorkloadTrust, config.Conmon, config.ConmonEnv, config.CgroupManager, config.ContainerExitsDir, config.ContainerAttachSocketDir, config.LogSizeMax, config.NoPivot)
+	runtime, err := oci.New(config.Runtime, config.RuntimeUntrustedWorkload, config.DefaultWorkloadTrust, config.Conmon, config.ConmonEnv, config.CgroupManager, config.ContainerExitsDir, config.ContainerAttachSocketDir, config.LogSizeMax, config.NoPivot, config.CtrStopTimeout)
 	if err != nil {
 		return nil, err
 	}

lib/stop.go

@@ -25,7 +25,7 @@ func (c *ContainerServer) ContainerStop(ctx context.Context, container string, t
 			if err := c.runtime.StopContainer(ctx, ctr, timeout); err != nil {
 				return "", errors.Wrapf(err, "failed to stop container %s", ctrID)
 			}
-			if err := c.runtime.WaitContainerStateStopped(ctx, ctr, timeout); err != nil {
+			if err := c.runtime.WaitContainerStateStopped(ctx, ctr); err != nil {
 				return "", errors.Wrapf(err, "failed to get container 'stopped' status %s", ctrID)
 			}
 			if err := c.storageRuntimeServer.StopContainer(ctrID); err != nil {

lib/testdata/config.toml

@@ -17,6 +17,7 @@
     hooks_dir_path = "/usr/share/containers/oci/hooks.d"
     pids_limit = 2048
     container_exits_dir = "/var/run/podman/exits"
+    ctr_stop_timeout = 10
   [crio.image]
     default_transport = "docker://"
     pause_image = "kubernetes/pause"

oci/oci.go

@@ -43,6 +43,11 @@ const (
 	// killContainerTimeout is the timeout that we wait for the container to
 	// be SIGKILLed.
 	killContainerTimeout = 2 * time.Minute
+
+	// minCtrStopTimeout is the minimal amout of time in seconds to wait
+	// before issuing a timeout regarding the proper termination of the
+	// container.
+	minCtrStopTimeout = 10
 )
 
 // New creates a new Runtime with options provided
@@ -55,7 +60,8 @@ func New(runtimeTrustedPath string,
 	containerExitsDir string,
 	containerAttachSocketDir string,
 	logSizeMax int64,
-	noPivot bool) (*Runtime, error) {
+	noPivot bool,
+	ctrStopTimeout int64) (*Runtime, error) {
 	r := &Runtime{
 		name:                     filepath.Base(runtimeTrustedPath),
 		trustedPath:              runtimeTrustedPath,
@@ -68,6 +74,7 @@ func New(runtimeTrustedPath string,
 		containerAttachSocketDir: containerAttachSocketDir,
 		logSizeMax:               logSizeMax,
 		noPivot:                  noPivot,
+		ctrStopTimeout:           ctrStopTimeout,
 	}
 	return r, nil
 }
@@ -85,6 +92,7 @@ type Runtime struct {
 	containerAttachSocketDir string
 	logSizeMax               int64
 	noPivot                  bool
+	ctrStopTimeout           int64
 }
 
 // syncInfo is used to return data from monitor process to daemon
@@ -586,13 +594,21 @@ func waitContainerStop(ctx context.Context, c *Container, timeout time.Duration,
 // WaitContainerStateStopped runs a loop polling UpdateStatus(), seeking for
 // the container status to be updated to 'stopped'. Either it gets the expected
 // status and returns nil, or it reaches the timeout and returns an error.
-func (r *Runtime) WaitContainerStateStopped(ctx context.Context, c *Container, timeout int64) (err error) {
+func (r *Runtime) WaitContainerStateStopped(ctx context.Context, c *Container) (err error) {
 	// No need to go further and spawn the go routine if the container
 	// is already in the expected status.
 	if r.ContainerStatus(c).Status == ContainerStateStopped {
 		return nil
 	}
 
+	// We need to ensure the container termination will be properly waited
+	// for by defining a minimal timeout value. This will prevent timeout
+	// value defined in the configuration file to be too low.
+	timeout := r.ctrStopTimeout
+	if timeout < minCtrStopTimeout {
+		timeout = minCtrStopTimeout
+	}
+
 	done := make(chan error)
 	chControl := make(chan struct{})
 	go func() {

server/fixtures/crio.conf

@@ -22,6 +22,7 @@ seccomp_profile = "/etc/crio/seccomp.json"
 apparmor_profile = "crio-default"
 cgroup_manager = "cgroupfs"
 pids_limit = 1024
+ctr_stop_timeout = 10
 
 [crio.image]
 default_transport = "docker://"

server/sandbox_remove.go

@@ -53,7 +53,7 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR
 					// Assume container is already stopped
 					logrus.Warnf("failed to stop container %s: %v", c.Name(), err)
 				}
-				if err := s.Runtime().WaitContainerStateStopped(ctx, c, timeout); err != nil {
+				if err := s.Runtime().WaitContainerStateStopped(ctx, c); err != nil {
 					return nil, fmt.Errorf("failed to get container 'stopped' status %s in pod sandbox %s: %v", c.Name(), sb.ID(), err)
 				}
 			}

server/sandbox_stop_linux.go

@@ -63,7 +63,7 @@ func (s *Server) stopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque
 			if err := s.Runtime().StopContainer(ctx, c, timeout); err != nil {
 				return nil, fmt.Errorf("failed to stop container %s in pod sandbox %s: %v", c.Name(), sb.ID(), err)
 			}
-			if err := s.Runtime().WaitContainerStateStopped(ctx, c, timeout); err != nil {
+			if err := s.Runtime().WaitContainerStateStopped(ctx, c); err != nil {
 				return nil, fmt.Errorf("failed to get container 'stopped' status %s in pod sandbox %s: %v", c.Name(), sb.ID(), err)
 			}
 			if err := s.StorageRuntimeServer().StopContainer(c.ID()); err != nil && errors.Cause(err) != storage.ErrContainerUnknown {
@@ -82,7 +82,7 @@ func (s *Server) stopPodSandbox(ctx context.Context, req *pb.StopPodSandboxReque
 		if err := s.Runtime().StopContainer(ctx, podInfraContainer, timeout); err != nil {
 			return nil, fmt.Errorf("failed to stop infra container %s in pod sandbox %s: %v", podInfraContainer.Name(), sb.ID(), err)
 		}
-		if err := s.Runtime().WaitContainerStateStopped(ctx, podInfraContainer, timeout); err != nil {
+		if err := s.Runtime().WaitContainerStateStopped(ctx, podInfraContainer); err != nil {
 			return nil, fmt.Errorf("failed to get infra container 'stopped' status %s in pod sandbox %s: %v", podInfraContainer.Name(), sb.ID(), err)
 		}
 	}