From 044e2985071eca89377f8ff7c53cbb5f7859b879 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Tue, 8 Mar 2016 18:05:50 -0800 Subject: [PATCH] Improve error handling in runc The error handling on the runc cli is currenly pretty messy because messages to the user are split between regular stderr format and logrus message format. This changes all the error reporting to the cli to only output on stderr and exit(1) for consumers of the api. By default logrus logs to /dev/null so that it is not seen by the user. If the user wants extra and/or structured loggging/errors from runc they can use the `--log` flag to provide a path to the file where they want this information. This allows a consistent behavior on the cli but extra power and information when debugging with logs. This also includes a change to enable the same logging information inside the container's init by adding an init cli command that can share the existing flags for all other runc commands. Signed-off-by: Michael Crosby --- events.go | 4 ++-- exec.go | 5 ++--- main.go | 6 ++++-- main_unsupported.go | 7 ++----- restore.go | 3 +-- spec.go | 7 +++---- start.go | 15 +++++++++------ utils.go | 30 ++++++++++++++++++++++++------ 8 files changed, 47 insertions(+), 30 deletions(-) diff --git a/events.go b/events.go index 3c3255eaa..777ecea23 100644 --- a/events.go +++ b/events.go @@ -35,7 +35,7 @@ information is displayed once every 5 seconds.`, Action: func(context *cli.Context) { container, err := getContainer(context) if err != nil { - logrus.Fatal(err) + fatal(err) } var ( stats = make(chan *libcontainer.Stats, 1) @@ -74,7 +74,7 @@ information is displayed once every 5 seconds.`, }() n, err := container.NotifyOOM() if err != nil { - logrus.Fatal(err) + fatal(err) } for { select { diff --git a/exec.go b/exec.go index 4c04e6587..62b78ba05 100644 --- a/exec.go +++ b/exec.go @@ -9,7 +9,6 @@ import ( "strconv" "strings" - "github.com/Sirupsen/logrus" "github.com/codegangsta/cli" "github.com/opencontainers/specs" ) @@ -80,11 +79,11 @@ following will output a list of processes running in the container: }, Action: func(context *cli.Context) { if os.Geteuid() != 0 { - logrus.Fatal("runc should be run as root") + fatalf("runc should be run as root") } status, err := execProcess(context) if err != nil { - logrus.Fatalf("exec failed: %v", err) + fatalf("exec failed: %v", err) } os.Exit(status) }, diff --git a/main.go b/main.go index 32a954c78..1bfa9a3a7 100644 --- a/main.go +++ b/main.go @@ -49,6 +49,7 @@ func main() { }, cli.StringFlag{ Name: "log", + Value: "/dev/null", Usage: "set the log file path where internal debug information is written", }, cli.StringFlag{ @@ -72,6 +73,7 @@ func main() { deleteCommand, eventsCommand, execCommand, + initCommand, killCommand, listCommand, pauseCommand, @@ -86,7 +88,7 @@ func main() { logrus.SetLevel(logrus.DebugLevel) } if path := context.GlobalString("log"); path != "" { - f, err := os.Create(path) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) if err != nil { return err } @@ -103,6 +105,6 @@ func main() { return nil } if err := app.Run(os.Args); err != nil { - logrus.Fatal(err) + fatal(err) } } diff --git a/main_unsupported.go b/main_unsupported.go index 610012044..3a83f4c10 100644 --- a/main_unsupported.go +++ b/main_unsupported.go @@ -2,10 +2,7 @@ package main -import ( - "github.com/Sirupsen/logrus" - "github.com/codegangsta/cli" -) +import "github.com/codegangsta/cli" var ( checkpointCommand cli.Command @@ -16,5 +13,5 @@ var ( ) func runAction(*cli.Context) { - logrus.Fatal("Current OS is not supported yet") + fatalf("Current OS is not supported yet") } diff --git a/restore.go b/restore.go index 5584c7859..3a644f1f8 100644 --- a/restore.go +++ b/restore.go @@ -3,7 +3,6 @@ package main import ( - "fmt" "os" "syscall" @@ -124,7 +123,7 @@ func restoreContainer(context *cli.Context, spec *specs.LinuxSpec, config *confi logrus.Error(err) } if status == libcontainer.Running { - fatal(fmt.Errorf("Container with id %s already running", id)) + fatalf("Container with id %s already running", id) } setManageCgroupsMode(context, options) diff --git a/spec.go b/spec.go index 6c497aa25..9c05a6dc4 100644 --- a/spec.go +++ b/spec.go @@ -13,7 +13,6 @@ import ( "strings" "syscall" - "github.com/Sirupsen/logrus" "github.com/codegangsta/cli" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" @@ -163,14 +162,14 @@ var specCommand = cli.Command{ } } if err := checkNoFile(specConfig); err != nil { - logrus.Fatal(err) + fatal(err) } data, err := json.MarshalIndent(&spec, "", "\t") if err != nil { - logrus.Fatal(err) + fatal(err) } if err := ioutil.WriteFile(specConfig, data, 0666); err != nil { - logrus.Fatal(err) + fatal(err) } }, } diff --git a/start.go b/start.go index 4df3e44f7..ec8f217c1 100644 --- a/start.go +++ b/start.go @@ -6,7 +6,6 @@ import ( "os" "runtime" - "github.com/Sirupsen/logrus" "github.com/codegangsta/cli" "github.com/coreos/go-systemd/activation" "github.com/opencontainers/runc/libcontainer" @@ -63,12 +62,12 @@ is a directory with a specification file and a root filesystem.`, } if os.Geteuid() != 0 { - logrus.Fatal("runc should be run as root") + fatalf("runc should be run as root") } status, err := startContainer(context, spec) if err != nil { - logrus.Fatalf("Container start failed: %v", err) + fatalf("Container start failed: %v", err) } // exit with the container's exit status so any external supervisor is // notified of the exit with the correct exit status. @@ -76,8 +75,12 @@ is a directory with a specification file and a root filesystem.`, }, } -func init() { - if len(os.Args) > 1 && os.Args[1] == "init" { +var initCommand = cli.Command{ + Name: "init", + Usage: `init is used to initialize the containers namespaces and launch the users process. + This command should not be called outside of runc. + `, + Action: func(context *cli.Context) { runtime.GOMAXPROCS(1) runtime.LockOSThread() factory, _ := libcontainer.New("") @@ -85,7 +88,7 @@ func init() { fatal(err) } panic("libcontainer: container init failed to exec") - } + }, } func startContainer(context *cli.Context, spec *specs.LinuxSpec) (int, error) { diff --git a/utils.go b/utils.go index 64412189b..256d8549f 100644 --- a/utils.go +++ b/utils.go @@ -152,15 +152,31 @@ func containerPreload(context *cli.Context) error { // loadFactory returns the configured factory instance for execing containers. func loadFactory(context *cli.Context) (libcontainer.Factory, error) { - root := context.GlobalString("root") + var ( + debug = "false" + root = context.GlobalString("root") + ) + if context.GlobalBool("debug") { + debug = "true" + } abs, err := filepath.Abs(root) if err != nil { return nil, err } + logAbs, err := filepath.Abs(context.GlobalString("log")) + if err != nil { + return nil, err + } return libcontainer.New(abs, libcontainer.Cgroupfs, func(l *libcontainer.LinuxFactory) error { l.CriuPath = context.GlobalString("criu") return nil - }) + }, + libcontainer.InitArgs(os.Args[0], + "--log", logAbs, + "--log-format", context.GlobalString("log-format"), + fmt.Sprintf("--debug=%s", debug), + "init"), + ) } // getContainer returns the specified container instance by loading it from state @@ -180,6 +196,8 @@ func getContainer(context *cli.Context) (libcontainer.Container, error) { // fatal prints the error's details if it is a libcontainer specific error type // then exits the program with an exit status of 1. func fatal(err error) { + // make sure the error is written to the logger + logrus.Error(err) // return proper unix error codes if exerr, ok := err.(*exec.Error); ok { switch exerr.Err { @@ -196,14 +214,14 @@ func fatal(err error) { } } } - if lerr, ok := err.(libcontainer.Error); ok { - lerr.Detail(os.Stderr) - os.Exit(1) - } fmt.Fprintln(os.Stderr, err) os.Exit(1) } +func fatalf(t string, v ...interface{}) { + fatal(fmt.Errorf(t, v...)) +} + func getDefaultImagePath(context *cli.Context) string { cwd, err := os.Getwd() if err != nil {