From 0a3577c6808eebc842295d273cbf3d532f76794e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 29 Jun 2021 17:22:08 -0700 Subject: [PATCH 1/5] utils_linux: simplify newProcess newProcess do not need those extra arguments, they can be handled in the caller. Signed-off-by: Kir Kolyshkin --- utils_linux.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index 707bf5d3bdc..a36a3fafbf7 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -97,7 +97,7 @@ func getDefaultImagePath(context *cli.Context) string { // newProcess returns a new libcontainer Process with the arguments from the // spec and stdio from the current process. -func newProcess(p specs.Process, init bool, logLevel string) (*libcontainer.Process, error) { +func newProcess(p specs.Process) (*libcontainer.Process, error) { lp := &libcontainer.Process{ Args: p.Args, Env: p.Env, @@ -107,8 +107,6 @@ func newProcess(p specs.Process, init bool, logLevel string) (*libcontainer.Proc Label: p.SelinuxLabel, NoNewPrivileges: &p.NoNewPrivileges, AppArmorProfile: p.ApparmorProfile, - Init: init, - LogLevel: logLevel, } if p.ConsoleSize != nil { @@ -270,10 +268,13 @@ func (r *runner) run(config *specs.Process) (int, error) { if err = r.checkTerminal(config); err != nil { return -1, err } - process, err := newProcess(*config, r.init, r.logLevel) + process, err := newProcess(*config) if err != nil { return -1, err } + // Populate the fields that come from runner. + process.Init = r.init + process.LogLevel = r.logLevel if len(r.listenFDs) > 0 { process.Env = append(process.Env, "LISTEN_FDS="+strconv.Itoa(len(r.listenFDs)), "LISTEN_PID=1") process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...) From 6c4a3b13d132aa460e610cf2c972e9ed71e4a42d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Sun, 25 Jul 2021 20:47:26 -0700 Subject: [PATCH 2/5] runc init: pass _LIBCONTAINER_LOGLEVEL as int Instead of passing _LIBCONTAINER_LOGLEVEL as a string (like "debug" or "info"), use a numeric value. Also, simplify the init log level passing code -- since we actually use the same level as the runc binary, just get it from logrus. This is a preparation for the next commit. Signed-off-by: Kir Kolyshkin --- exec.go | 6 ------ init.go | 4 ++-- utils_linux.go | 9 +-------- 3 files changed, 3 insertions(+), 16 deletions(-) diff --git a/exec.go b/exec.go index 3649bdc1bbd..66ee3653c58 100644 --- a/exec.go +++ b/exec.go @@ -132,11 +132,6 @@ func execProcess(context *cli.Context) (int, error) { return -1, err } - logLevel := "info" - if context.GlobalBool("debug") { - logLevel = "debug" - } - r := &runner{ enableSubreaper: false, shouldDestroy: false, @@ -147,7 +142,6 @@ func execProcess(context *cli.Context) (int, error) { action: CT_ACT_RUN, init: false, preserveFDs: context.Int("preserve-fds"), - logLevel: logLevel, } return r.run(p) } diff --git a/init.go b/init.go index 61026e220e4..bddc237f6e5 100644 --- a/init.go +++ b/init.go @@ -17,7 +17,7 @@ func init() { runtime.GOMAXPROCS(1) runtime.LockOSThread() - logLevel, err := logrus.ParseLevel(os.Getenv("_LIBCONTAINER_LOGLEVEL")) + level, err := strconv.Atoi(os.Getenv("_LIBCONTAINER_LOGLEVEL")) if err != nil { panic(err) } @@ -27,7 +27,7 @@ func init() { panic(err) } - logrus.SetLevel(logLevel) + logrus.SetLevel(logrus.Level(level)) logrus.SetOutput(os.NewFile(uintptr(logPipeFd), "logpipe")) logrus.SetFormatter(new(logrus.JSONFormatter)) logrus.Debug("child process in init()") diff --git a/utils_linux.go b/utils_linux.go index a36a3fafbf7..5404f2eb98e 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -255,7 +255,6 @@ type runner struct { action CtAct notifySocket *notifySocket criuOpts *libcontainer.CriuOpts - logLevel string } func (r *runner) run(config *specs.Process) (int, error) { @@ -272,9 +271,9 @@ func (r *runner) run(config *specs.Process) (int, error) { if err != nil { return -1, err } + process.LogLevel = strconv.Itoa(int(logrus.GetLevel())) // Populate the fields that come from runner. process.Init = r.init - process.LogLevel = r.logLevel if len(r.listenFDs) > 0 { process.Env = append(process.Env, "LISTEN_FDS="+strconv.Itoa(len(r.listenFDs)), "LISTEN_PID=1") process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...) @@ -431,11 +430,6 @@ func startContainer(context *cli.Context, spec *specs.Spec, action CtAct, criuOp listenFDs = activation.Files(false) } - logLevel := "info" - if context.GlobalBool("debug") { - logLevel = "debug" - } - r := &runner{ enableSubreaper: !context.Bool("no-subreaper"), shouldDestroy: !context.Bool("keep"), @@ -449,7 +443,6 @@ func startContainer(context *cli.Context, spec *specs.Spec, action CtAct, criuOp action: action, criuOpts: criuOpts, init: true, - logLevel: logLevel, } return r.run(spec.Process) } From d2f49d4563244f4a34d132829e1a2740a5cb6404 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 17:16:37 -0700 Subject: [PATCH 3/5] libct/nsenter/nsexec.c: improve bail This makes it possible to use bail() even if logging is not set up (yet), so we don't have to think whether it's OK to use it or not. In addition, this might help some unit tests that do not set log forwarding. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsexec.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index 30b6d5e4ad3..554ffd6c2ab 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -182,10 +182,14 @@ static void write_log(const char *level, const char *format, ...) /* XXX: This is ugly. */ static int syncfd = -1; -#define bail(fmt, ...) \ - do { \ - write_log(FATAL, fmt ": %m", ##__VA_ARGS__); \ - exit(1); \ +#define bail(fmt, ...) \ + do { \ + if (logfd < 0) \ + fprintf(stderr, "FATAL: " fmt ": %m\n", \ + ##__VA_ARGS__); \ + else \ + write_log(FATAL, fmt ": %m", ##__VA_ARGS__); \ + exit(1); \ } while(0) static int write_file(char *data, size_t data_len, char *pathfmt, ...) @@ -407,9 +411,7 @@ static void setup_logpipe(void) logfd = strtol(logpipe, &endptr, 10); if (logpipe == endptr || *endptr != '\0') { - fprintf(stderr, "unable to parse _LIBCONTAINER_LOGPIPE, value: %s\n", logpipe); - /* It is too early to use bail */ - exit(1); + bail("unable to parse _LIBCONTAINER_LOGPIPE, value: %s", logpipe); } } From d5ffe83f94c8ac4dd171cdd6b2de2b2cdad8fcfd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 18:32:18 -0700 Subject: [PATCH 4/5] libct/nsenter/nsexec.c: factor out getenv_int The code already parses an environment variable into an integer twice, and we're about to add a third one. Factor it out to getenv_int(). Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsexec.c | 62 +++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index 554ffd6c2ab..c2fa8aa3412 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -380,39 +380,48 @@ static int clone_parent(jmp_buf *env, int jmpval) } /* - * Gets the init pipe fd from the environment, which is used to read the - * bootstrap data and tell the parent what the new pid is after we finish - * setting up the environment. + * Returns an environment variable value as a non-negative integer, or -ENOENT + * if the variable was not found or has an empty value. + * + * If the value can not be converted to an integer, or the result is out of + * range, the function bails out. */ -static int initpipe(void) +static int getenv_int(const char *name) { - int pipenum; - char *initpipe, *endptr; + char *val, *endptr; + int ret; - initpipe = getenv("_LIBCONTAINER_INITPIPE"); - if (initpipe == NULL || *initpipe == '\0') - return -1; + val = getenv(name); + /* Treat empty value as unset variable. */ + if (val == NULL || *val == '\0') + return -ENOENT; - pipenum = strtol(initpipe, &endptr, 10); - if (*endptr != '\0') - bail("unable to parse _LIBCONTAINER_INITPIPE"); + ret = strtol(val, &endptr, 10); + if (val == endptr || *endptr != '\0') + bail("unable to parse %s=%s", name, val); + /* + * Sanity check: this must be a non-negative number. + */ + if (ret < 0) + bail("bad value for %s=%s (%d)", name, val, ret); - return pipenum; + return ret; } +/* + * Sets up logging by getting log fd from the environment, + * if available. + */ static void setup_logpipe(void) { - char *logpipe, *endptr; + int i; - logpipe = getenv("_LIBCONTAINER_LOGPIPE"); - if (logpipe == NULL || *logpipe == '\0') { + i = getenv_int("_LIBCONTAINER_LOGPIPE"); + if (i < 0) { + /* We are not runc init, or log pipe was not provided. */ return; } - - logfd = strtol(logpipe, &endptr, 10); - if (logpipe == endptr || *endptr != '\0') { - bail("unable to parse _LIBCONTAINER_LOGPIPE, value: %s", logpipe); - } + logfd = i; } /* Returns the clone(2) flag for a namespace, given the name of a namespace. */ @@ -623,12 +632,15 @@ void nsexec(void) setup_logpipe(); /* - * If we don't have an init pipe, just return to the go routine. - * We'll only get an init pipe for start or exec. + * Get the init pipe fd from the environment. The init pipe is used to + * read the bootstrap data and tell the parent what the new pids are + * after the setup is done. */ - pipenum = initpipe(); - if (pipenum == -1) + pipenum = getenv_int("_LIBCONTAINER_INITPIPE"); + if (pipenum < 0) { + /* We are not a runc init. Just return to go runtime. */ return; + } /* * We need to re-exec if we are not in a cloned binary. This is necessary From f1b703fc45dd61429634e9d448206c9b75d87147 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 18:46:31 -0700 Subject: [PATCH 5/5] libct/nsenter/nsexec.c: honor _LIBCONTAINER_LOGLEVEL Currently, if the log level is not set to e.g. "debug", runc init sends some debug logs to the parent, which parses and discards it. It is better to not send those in the first place. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsexec.c | 41 ++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index c2fa8aa3412..7e79ca0eb24 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -89,14 +89,21 @@ struct nlconfig_t { size_t gidmappath_len; }; -#define PANIC "panic" -#define FATAL "fatal" -#define ERROR "error" -#define WARNING "warning" -#define INFO "info" -#define DEBUG "debug" +/* + * Log levels are the same as in logrus. + */ +#define PANIC 0 +#define FATAL 1 +#define ERROR 2 +#define WARNING 3 +#define INFO 4 +#define DEBUG 5 +#define TRACE 6 + +static const char *level_str[] = { "panic", "fatal", "error", "warning", "info", "debug", "trace" }; static int logfd = -1; +static int loglevel = DEBUG; /* * List of netlink message types sent to us as part of bootstrapping the init. @@ -134,13 +141,13 @@ int setns(int fd, int nstype) } #endif -static void write_log(const char *level, const char *format, ...) +static void write_log(int level, const char *format, ...) { char *message = NULL, *stage = NULL, *json = NULL; va_list args; int ret; - if (logfd < 0 || level == NULL) + if (logfd < 0 || level > loglevel) goto out; va_start(args, format); @@ -162,7 +169,8 @@ static void write_log(const char *level, const char *format, ...) goto out; } - ret = asprintf(&json, "{\"level\":\"%s\", \"msg\": \"%s[%d]: %s\"}\n", level, stage, getpid(), message); + ret = asprintf(&json, "{\"level\":\"%s\", \"msg\": \"%s[%d]: %s\"}\n", + level_str[level], stage, getpid(), message); if (ret < 0) { json = NULL; goto out; @@ -400,16 +408,18 @@ static int getenv_int(const char *name) if (val == endptr || *endptr != '\0') bail("unable to parse %s=%s", name, val); /* - * Sanity check: this must be a non-negative number. - */ - if (ret < 0) + * Sanity check: this must be a small non-negative number. + * Practically, we pass two fds (3 and 4) and a log level, + * for which the maximum is 6 (TRACE). + * */ + if (ret < 0 || ret > TRACE) bail("bad value for %s=%s (%d)", name, val, ret); return ret; } /* - * Sets up logging by getting log fd from the environment, + * Sets up logging by getting log fd and log level from the environment, * if available. */ static void setup_logpipe(void) @@ -422,6 +432,11 @@ static void setup_logpipe(void) return; } logfd = i; + + i = getenv_int("_LIBCONTAINER_LOGLEVEL"); + if (i < 0) + return; + loglevel = i; } /* Returns the clone(2) flag for a namespace, given the name of a namespace. */