libct: better errors for hooks

When a hook has failed, the error message looks like this: > error running hook: error running hook #1: exit status 1, stdout: ... The two problems here are: 1. it is impossible to know what kind of hook it was; 2. "error running hook" stuttering; Change that to > error running createContainer hook #1: exit status 1, stdout: ... Signed-off-by: Kir Kolyshkin <[email protected]>
opencontainers · Aug 25, 2023 · 6a4870e · 6a4870e
1 parent 693d1c6
commit 6a4870e
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 16 deletions.
diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go
@@ -285,6 +285,7 @@ type Capabilities struct {
 	Ambient []string
 }
 
+// Deprecated: use (Hooks).Run instead.
 func (hooks HookList) RunHooks(state *specs.State) error {
 	for i, h := range hooks {
 		if err := h.Run(state); err != nil {
@@ -341,6 +342,18 @@ func (hooks *Hooks) MarshalJSON() ([]byte, error) {
 	})
 }
 
+// Run executes all hooks for the given hook name.
+func (hooks Hooks) Run(name HookName, state *specs.State) error {
+	list := hooks[name]
+	for i, h := range list {
+		if err := h.Run(state); err != nil {
+			return fmt.Errorf("error running %s hook #%d: %w", name, i, err)
+		}
+	}
+
+	return nil
+}
+
 type Hook interface {
 	// Run executes the hook with the provided state.
 	Run(*specs.State) error
@@ -401,7 +414,7 @@ func (c Command) Run(s *specs.State) error {
 	go func() {
 		err := cmd.Wait()
 		if err != nil {
-			err = fmt.Errorf("error running hook: %w, stdout: %s, stderr: %s", err, stdout.String(), stderr.String())
+			err = fmt.Errorf("%w, stdout: %s, stderr: %s", err, stdout.String(), stderr.String())
 		}
 		errC <- err
 	}()

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
@@ -341,7 +341,7 @@ func (c *Container) start(process *Process) (retErr error) {
 				return err
 			}
 
-			if err := c.config.Hooks[configs.Poststart].RunHooks(s); err != nil {
+			if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
 				if err := ignoreTerminateErrors(parent.terminate()); err != nil {
 					logrus.Warn(fmt.Errorf("error running poststart hook: %w", err))
 				}

diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go
@@ -1134,10 +1134,10 @@ func (c *Container) criuNotifications(resp *criurpc.CriuResp, process *Process,
 			}
 			s.Pid = int(notify.GetPid())
 
-			if err := c.config.Hooks[configs.Prestart].RunHooks(s); err != nil {
+			if err := c.config.Hooks.Run(configs.Prestart, s); err != nil {
 				return err
 			}
-			if err := c.config.Hooks[configs.CreateRuntime].RunHooks(s); err != nil {
+			if err := c.config.Hooks.Run(configs.CreateRuntime, s); err != nil {
 				return err
 			}
 		}

diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
@@ -536,10 +536,10 @@ func (p *initProcess) start() (retErr error) {
 					s.Status = specs.StateCreating
 					hooks := p.config.Config.Hooks
 
-					if err := hooks[configs.Prestart].RunHooks(s); err != nil {
+					if err := hooks.Run(configs.Prestart, s); err != nil {
 						return err
 					}
-					if err := hooks[configs.CreateRuntime].RunHooks(s); err != nil {
+					if err := hooks.Run(configs.CreateRuntime, s); err != nil {
 						return err
 					}
 				}
@@ -590,10 +590,10 @@ func (p *initProcess) start() (retErr error) {
 				s.Status = specs.StateCreating
 				hooks := p.config.Config.Hooks
 
-				if err := hooks[configs.Prestart].RunHooks(s); err != nil {
+				if err := hooks.Run(configs.Prestart, s); err != nil {
 					return err
 				}
-				if err := hooks[configs.CreateRuntime].RunHooks(s); err != nil {
+				if err := hooks.Run(configs.CreateRuntime, s); err != nil {
 					return err
 				}
 			}

diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go
@@ -143,7 +143,7 @@ func prepareRootfs(pipe *os.File, iConfig *initConfig, mountFds mountFds) (err e
 	s := iConfig.SpecState
 	s.Pid = unix.Getpid()
 	s.Status = specs.StateCreating
-	if err := iConfig.Config.Hooks[configs.CreateContainer].RunHooks(s); err != nil {
+	if err := iConfig.Config.Hooks.Run(configs.CreateContainer, s); err != nil {
 		return err
 	}
 

diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go
@@ -258,7 +258,7 @@ func (l *linuxStandardInit) Init() error {
 	s := l.config.SpecState
 	s.Pid = unix.Getpid()
 	s.Status = specs.StateCreated
-	if err := l.config.Config.Hooks[configs.StartContainer].RunHooks(s); err != nil {
+	if err := l.config.Config.Hooks.Run(configs.StartContainer, s); err != nil {
 		return err
 	}
 

diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go
@@ -64,11 +64,7 @@ func runPoststopHooks(c *Container) error {
 	}
 	s.Status = specs.StateStopped
 
-	if err := hooks[configs.Poststop].RunHooks(s); err != nil {
-		return err
-	}
-
-	return nil
+	return hooks.Run(configs.Poststop, s)
 }
 
 // stoppedState represents a container is a stopped/destroyed state.

diff --git a/tests/integration/seccomp.bats b/tests/integration/seccomp.bats
@@ -182,6 +182,6 @@ function flags_value() {
 
 	runc run test_busybox
 	[ "$status" -ne 0 ]
-	[[ "$output" == *"error running hook"* ]]
+	[[ "$output" == *"error running startContainer hook"* ]]
 	[[ "$output" == *"bad system call"* ]]
 }