Skip to content

Commit

Permalink
libcontainer: rm own error system
Browse files Browse the repository at this point in the history 
This removes libcontainer's own error wrapping system, consisting of a
few types and functions, aimed at typization, wrapping and unwrapping
of errors, as well as saving error stack traces.

Since Go 1.13 now provides its own error wrapping mechanism and a few
related functions, it makes sense to switch to it.

While doing that, improve some error messages so that they start
with "error", "unable to", or "can't".

A few things that are worth mentioning:

1. We lose stack traces (which were never shown anyway).

2. Users of libcontainer that relied on particular errors (like
   ContainerNotExists) need to switch to using errors.Is with
   the new errors defined in error.go.

3. encoding/json is unable to unmarshal the built-in error type,
   so we have to introduce initError and wrap the errors into it
   (basically passing the error as a string). This is the same
   as it was before, just a tad simpler (actually the initError
   is a type that got removed in commit afa8443; also suddenly
   ierr variable name makes sense now).

Signed-off-by: Kir Kolyshkin <[email protected]>
  • Loading branch information
@kolyshkin
kolyshkin committed Jun 22, 2021
1 parent 18aa2cc commit 35d2615
Show file tree
Hide file tree
Showing 19 changed files with 151 additions and 431 deletions.
3 changes: 1 addition & 2 deletions delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ status of "ubuntu01" as "stopped" the following will delete resources held for
force := context.Bool("force")
container, err := getContainer(context)
if err != nil {
var lerr libcontainer.Error
if errors.As(err, &lerr) && lerr.Code() == libcontainer.ContainerNotExists {
if errors.Is(err, libcontainer.ErrNotExist) {
// if there was an aborted start or something of the sort then the container's directory could exist but
// libcontainer does not see it because the state.json file inside that directory was never created.
path := filepath.Join(context.GlobalString("root"), id)
Expand Down
43 changes: 0 additions & 43 deletions libcontainer/container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,71 +74,39 @@ type BaseContainer interface {
ID() string

// Returns the current status of the container.
//
// errors:
// ContainerNotExists - Container no longer exists,
// Systemerror - System error.
Status() (Status, error)

// State returns the current container's state information.
//
// errors:
// SystemError - System error.
State() (*State, error)

// OCIState returns the current container's state information.
//
// errors:
// SystemError - System error.
OCIState() (*specs.State, error)

// Returns the current config of the container.
Config() configs.Config

// Returns the PIDs inside this container. The PIDs are in the namespace of the calling process.
//
// errors:
// ContainerNotExists - Container no longer exists,
// Systemerror - System error.
//
// Some of the returned PIDs may no longer refer to processes in the Container, unless
// the Container state is PAUSED in which case every PID in the slice is valid.
Processes() ([]int, error)

// Returns statistics for the container.
//
// errors:
// ContainerNotExists - Container no longer exists,
// Systemerror - System error.
Stats() (*Stats, error)

// Set resources of container as configured
//
// We can use this to change resources when containers are running.
//
// errors:
// SystemError - System error.
Set(config configs.Config) error

// Start a process inside the container. Returns error if process fails to
// start. You can track process lifecycle with passed Process structure.
//
// errors:
// ContainerNotExists - Container no longer exists,
// ConfigInvalid - config is invalid,
// ContainerPaused - Container is paused,
// SystemError - System error.
Start(process *Process) (err error)

// Run immediately starts the process inside the container. Returns error if process
// fails to start. It does not block waiting for the exec fifo after start returns but
// opens the fifo after start returns.
//
// errors:
// ContainerNotExists - Container no longer exists,
// ConfigInvalid - config is invalid,
// ContainerPaused - Container is paused,
// SystemError - System error.
Run(process *Process) (err error)

// Destroys the container, if its in a valid state, after killing any
Expand All @@ -149,25 +117,14 @@ type BaseContainer interface {
//
// Running containers must first be stopped using Signal(..).
// Paused containers must first be resumed using Resume(..).
//
// errors:
// ContainerNotStopped - Container is still running,
// ContainerPaused - Container is paused,
// SystemError - System error.
Destroy() error

// Signal sends the provided signal code to the container's initial process.
//
// If all is specified the signal is sent to all processes in the container
// including the initial process.
//
// errors:
// SystemError - System error.
Signal(s os.Signal, all bool) error

// Exec signals the container to exec the users process at the end of the init.
//
// errors:
// SystemError - System error.
Exec() error
}
64 changes: 21 additions & 43 deletions libcontainer/container_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,48 +97,26 @@ type Container interface {
// Methods below here are platform specific

// Checkpoint checkpoints the running container's state to disk using the criu(8) utility.
//
// errors:
// Systemerror - System error.
Checkpoint(criuOpts *CriuOpts) error

// Restore restores the checkpointed container to a running state using the criu(8) utility.
//
// errors:
// Systemerror - System error.
Restore(process *Process, criuOpts *CriuOpts) error

// If the Container state is RUNNING or CREATED, sets the Container state to PAUSING and pauses
// the execution of any user processes. Asynchronously, when the container finished being paused the
// state is changed to PAUSED.
// If the Container state is PAUSED, do nothing.
//
// errors:
// ContainerNotExists - Container no longer exists,
// ContainerNotRunning - Container not running or created,
// Systemerror - System error.
Pause() error

// If the Container state is PAUSED, resumes the execution of any user processes in the
// Container before setting the Container state to RUNNING.
// If the Container state is RUNNING, do nothing.
//
// errors:
// ContainerNotExists - Container no longer exists,
// ContainerNotPaused - Container is not paused,
// Systemerror - System error.
Resume() error

// NotifyOOM returns a read-only channel signaling when the container receives an OOM notification.
//
// errors:
// Systemerror - System error.
NotifyOOM() (<-chan struct{}, error)

// NotifyMemoryPressure returns a read-only channel signaling when the container reaches a given pressure level
//
// errors:
// Systemerror - System error.
NotifyMemoryPressure(level PressureLevel) (<-chan struct{}, error)
}

Expand Down Expand Up @@ -183,7 +161,7 @@ func (c *linuxContainer) Processes() ([]int, error) {

pids, err = c.cgroupManager.GetAllPids()
if err != nil {
return nil, newSystemErrorWithCause(err, "getting all container pids from cgroups")
return nil, fmt.Errorf("unable to get all container pids: %w", err)
}
return pids, nil
}
Expand All @@ -194,19 +172,19 @@ func (c *linuxContainer) Stats() (*Stats, error) {
stats = &Stats{}
)
if stats.CgroupStats, err = c.cgroupManager.GetStats(); err != nil {
return stats, newSystemErrorWithCause(err, "getting container stats from cgroups")
return stats, fmt.Errorf("unable to get container cgroup stats: %w", err)
}
if c.intelRdtManager != nil {
if stats.IntelRdtStats, err = c.intelRdtManager.GetStats(); err != nil {
return stats, newSystemErrorWithCause(err, "getting container's Intel RDT stats")
return stats, fmt.Errorf("unable to get container Intel RDT stats: %w", err)
}
}
for _, iface := range c.config.Networks {
switch iface.Type {
case "veth":
istats, err := getNetworkInterfaceStats(iface.HostInterfaceName)
if err != nil {
return stats, newSystemErrorWithCausef(err, "getting network stats for interface %q", iface.HostInterfaceName)
return stats, fmt.Errorf("unable to get network stats for interface %q: %w", iface.HostInterfaceName, err)
}
stats.Interfaces = append(stats.Interfaces, istats)
}
Expand All @@ -222,7 +200,7 @@ func (c *linuxContainer) Set(config configs.Config) error {
return err
}
if status == Stopped {
return newGenericError(errors.New("container not running"), ContainerNotRunning)
return ErrNotRunning
}
if err := c.cgroupManager.Set(config.Cgroups.Resources); err != nil {
// Set configs back
Expand Down Expand Up @@ -253,7 +231,7 @@ func (c *linuxContainer) Start(process *Process) error {
c.m.Lock()
defer c.m.Unlock()
if c.config.Cgroups.Resources.SkipDevices {
return newGenericError(errors.New("can't start container with SkipDevices set"), ConfigInvalid)
return &ConfigError{"can't start container with SkipDevices set"}
}
if process.Init {
if err := c.createExecFifo(); err != nil {
Expand Down Expand Up @@ -335,7 +313,7 @@ func fifoOpen(path string, block bool) openResult {
}
f, err := os.OpenFile(path, flags, 0)
if err != nil {
return openResult{err: newSystemErrorWithCause(err, "open exec fifo for reading")}
return openResult{err: fmt.Errorf("exec fifo: %w", err)}
}
return openResult{file: f}
}
Expand All @@ -360,7 +338,7 @@ type openResult struct {
func (c *linuxContainer) start(process *Process) (retErr error) {
parent, err := c.newParentProcess(process)
if err != nil {
return newSystemErrorWithCause(err, "creating new parent process")
return fmt.Errorf("unable to create new parent process: %w", err)
}

logsDone := parent.forwardChildLogs()
Expand All @@ -370,13 +348,13 @@ func (c *linuxContainer) start(process *Process) (retErr error) {
// runc init closing the _LIBCONTAINER_LOGPIPE log fd.
err := <-logsDone
if err != nil && retErr == nil {
retErr = newSystemErrorWithCause(err, "forwarding init logs")
retErr = fmt.Errorf("unable to forward init logs: %w", err)
}
}()
}

if err := parent.start(); err != nil {
return newSystemErrorWithCause(err, "starting container process")
return fmt.Errorf("unable to start container process: %w", err)
}

if process.Init {
Expand Down Expand Up @@ -415,11 +393,11 @@ func (c *linuxContainer) Signal(s os.Signal, all bool) error {
// to avoid a PID reuse attack
if status == Running || status == Created || status == Paused {
if err := c.initProcess.signal(s); err != nil {
return newSystemErrorWithCause(err, "signaling init process")
return fmt.Errorf("unable to signal init: %w", err)
}
return nil
}
return newGenericError(errors.New("container not running"), ContainerNotRunning)
return ErrNotRunning
}

func (c *linuxContainer) createExecFifo() error {
Expand Down Expand Up @@ -471,7 +449,7 @@ func (c *linuxContainer) includeExecFifo(cmd *exec.Cmd) error {
func (c *linuxContainer) newParentProcess(p *Process) (parentProcess, error) {
parentInitPipe, childInitPipe, err := utils.NewSockPair("init")
if err != nil {
return nil, newSystemErrorWithCause(err, "creating new init pipe")
return nil, fmt.Errorf("unable to create init pipe: %w", err)
}
messageSockPair := filePair{parentInitPipe, childInitPipe}

Expand All @@ -492,7 +470,7 @@ func (c *linuxContainer) newParentProcess(p *Process) (parentProcess, error) {
// that problem), but we no longer do that. However, there's no need to do
// this for `runc exec` so we just keep it this way to be safe.
if err := c.includeExecFifo(cmd); err != nil {
return nil, newSystemErrorWithCause(err, "including execfifo in cmd.Exec setup")
return nil, fmt.Errorf("unable to setup exec fifo: %w", err)
}
return c.newInitProcess(p, cmd, messageSockPair, logFilePair)
}
Expand Down Expand Up @@ -569,7 +547,7 @@ func (c *linuxContainer) newSetnsProcess(p *Process, cmd *exec.Cmd, messageSockP
cmd.Env = append(cmd.Env, "_LIBCONTAINER_INITTYPE="+string(initSetns))
state, err := c.currentState()
if err != nil {
return nil, newSystemErrorWithCause(err, "getting container's current state")
return nil, fmt.Errorf("unable to get container state: %w", err)
}
// for setns process, we don't have to set cloneflags as the process namespaces
// will only be set via setns syscall
Expand Down Expand Up @@ -654,7 +632,7 @@ func (c *linuxContainer) Pause() error {
c: c,
})
}
return newGenericError(fmt.Errorf("container not running or created: %s", status), ContainerNotRunning)
return ErrNotRunning
}

func (c *linuxContainer) Resume() error {
Expand All @@ -665,7 +643,7 @@ func (c *linuxContainer) Resume() error {
return err
}
if status != Paused {
return newGenericError(errors.New("container not paused"), ContainerNotPaused)
return ErrNotPaused
}
if err := c.cgroupManager.Freeze(configs.Thawed); err != nil {
return err
Expand Down Expand Up @@ -1476,7 +1454,7 @@ func (c *linuxContainer) criuApplyCgroups(pid int, req *criurpc.CriuReq) error {
}

if err := c.cgroupManager.Set(c.config.Cgroups.Resources); err != nil {
return newSystemError(err)
return err
}

if cgroups.IsCgroup2UnifiedMode() {
Expand Down Expand Up @@ -1997,16 +1975,16 @@ func (c *linuxContainer) orderNamespacePaths(namespaces map[configs.NamespaceTyp
if p, ok := namespaces[ns]; ok && p != "" {
// check if the requested namespace is supported
if !configs.IsNamespaceSupported(ns) {
return nil, newSystemError(fmt.Errorf("namespace %s is not supported", ns))
return nil, fmt.Errorf("namespace %s is not supported", ns)
}
// only set to join this namespace if it exists
if _, err := os.Lstat(p); err != nil {
return nil, newSystemErrorWithCausef(err, "running lstat on namespace path %q", p)
return nil, fmt.Errorf("namespace path: %w", err)
}
// do not allow namespace path with comma as we use it to separate
// the namespace paths
if strings.ContainsRune(p, ',') {
return nil, newSystemError(fmt.Errorf("invalid path %s", p))
return nil, fmt.Errorf("invalid namespace path %s", p)
}
paths = append(paths, fmt.Sprintf("%s:%s", configs.NsName(ns), p))
}
Expand Down
Loading

0 comments on commit 35d2615

Please sign in to comment.