execsync: rewrite to fix a bug in conmon
conmon has many flags that are parsed when it's executed, one of them is "-c". During PR #510 where we vendor latest kube master code, upstream has changed a test to call a "ctr execsync" with a command of "sh -c commmand ...". Turns out: a) conmon has a "-c" flag which refers to the container name/id b) the exec command has a "-c" flags but it's for "sh" That leads to conmon parsing the second "-c" flags from the exec command causing an error. The executed command looks like: conmon -c [..other flags..] CONTAINERID -e sh -c echo hello world This patch rewrites the exec sync code to not pass down to conmon the exec command via command line. Rather, we're now creating an OCI runtime process spec in a temp file, pass _the path_ down to conmon, and have runc exec the command using "runc exec --process /path/to/process-spec.json CONTAINERID". This is far better in which we don't need to bother anymore about conflicts with flags in conmon. Added and fixed some tests also. Signed-off-by: Antonio Murdaca <runcom@redhat.com>
This commit is contained in:
Antonio Murdaca
parent
26e90190fc
commit
b4251aebd8
4 changed files with 93 additions and 25 deletions
|
|
@ -87,6 +87,7 @@ static char *runtime_path = NULL;
|
|||
static char *bundle_path = NULL;
|
||||
static char *pid_file = NULL;
|
||||
static bool systemd_cgroup = false;
|
||||
static char *exec_process_spec = NULL;
|
||||
static bool exec = false;
|
||||
static char *log_path = NULL;
|
||||
static GOptionEntry entries[] =
|
||||
|
|
@ -98,6 +99,7 @@ static GOptionEntry entries[] =
|
|||
{ "pidfile", 'p', 0, G_OPTION_ARG_STRING, &pid_file, "PID file", NULL },
|
||||
{ "systemd-cgroup", 's', 0, G_OPTION_ARG_NONE, &systemd_cgroup, "Enable systemd cgroup manager", NULL },
|
||||
{ "exec", 'e', 0, G_OPTION_ARG_NONE, &exec, "Exec a command in a running container", NULL },
|
||||
{ "exec-process-spec", 0, 0, G_OPTION_ARG_STRING, &exec_process_spec, "Path to the process spec for exec", NULL },
|
||||
{ "log-path", 'l', 0, G_OPTION_ARG_STRING, &log_path, "Log file path", NULL },
|
||||
{ NULL }
|
||||
};
|
||||
|
|
@ -356,6 +358,10 @@ int main(int argc, char *argv[])
|
|||
bundle_path = cwd;
|
||||
}
|
||||
|
||||
if (exec && exec_process_spec == NULL) {
|
||||
nexit("Exec process spec path not provided. Use --exec-process-spec");
|
||||
}
|
||||
|
||||
if (pid_file == NULL) {
|
||||
if (snprintf(default_pid_file, sizeof(default_pid_file),
|
||||
"%s/pidfile-%s", cwd, cid) < 0) {
|
||||
|
|
@ -458,23 +464,14 @@ int main(int argc, char *argv[])
|
|||
if (terminal)
|
||||
g_string_append_printf(cmd, " --console-socket %s", csname);
|
||||
|
||||
/* Container name comes last. */
|
||||
g_string_append_printf(cmd, " %s", cid);
|
||||
|
||||
/* Set the exec arguments. */
|
||||
if (exec) {
|
||||
/*
|
||||
* FIXME: This code is broken if argv[1] contains spaces or other
|
||||
* similar characters that shells don't like. It's a bit silly
|
||||
* that we're doing things inside a shell at all -- this should
|
||||
* all be done in arrays.
|
||||
*/
|
||||
|
||||
int i;
|
||||
for (i = 1; i < argc; i++)
|
||||
g_string_append_printf(cmd, " %s", argv[i]);
|
||||
g_string_append_printf(cmd, " --process %s", exec_process_spec);
|
||||
}
|
||||
|
||||
/* Container name comes last. */
|
||||
g_string_append_printf(cmd, " %s", cid);
|
||||
|
||||
/*
|
||||
* We have to fork here because the current runC API dups the stdio of the
|
||||
* calling process over the container's fds. This is actually *very bad*
|
||||
|
|
|
|||
32
oci/oci.go
32
oci/oci.go
|
|
@ -15,6 +15,7 @@ import (
|
|||
|
||||
"github.com/Sirupsen/logrus"
|
||||
"github.com/kubernetes-incubator/cri-o/utils"
|
||||
rspec "github.com/opencontainers/runtime-spec/specs-go"
|
||||
"golang.org/x/sys/unix"
|
||||
)
|
||||
|
||||
|
|
@ -329,6 +330,15 @@ func (r *Runtime) ExecSync(c *Container, command []string, timeout int64) (resp
|
|||
os.RemoveAll(logPath)
|
||||
}()
|
||||
|
||||
f, err := ioutil.TempFile("", "exec-process")
|
||||
if err != nil {
|
||||
return nil, ExecSyncError{
|
||||
ExitCode: -1,
|
||||
Err: err,
|
||||
}
|
||||
}
|
||||
defer os.RemoveAll(f.Name())
|
||||
|
||||
var args []string
|
||||
args = append(args, "-c", c.name)
|
||||
args = append(args, "-r", r.Path(c))
|
||||
|
|
@ -339,7 +349,27 @@ func (r *Runtime) ExecSync(c *Container, command []string, timeout int64) (resp
|
|||
}
|
||||
args = append(args, "-l", logPath)
|
||||
|
||||
args = append(args, command...)
|
||||
pspec := rspec.Process{
|
||||
Env: r.conmonEnv,
|
||||
Args: command,
|
||||
Cwd: "/",
|
||||
}
|
||||
processJSON, err := json.Marshal(pspec)
|
||||
if err != nil {
|
||||
return nil, ExecSyncError{
|
||||
ExitCode: -1,
|
||||
Err: err,
|
||||
}
|
||||
}
|
||||
|
||||
if err := ioutil.WriteFile(f.Name(), processJSON, 0644); err != nil {
|
||||
return nil, ExecSyncError{
|
||||
ExitCode: -1,
|
||||
Err: err,
|
||||
}
|
||||
}
|
||||
|
||||
args = append(args, "--exec-process-spec", f.Name())
|
||||
|
||||
cmd := exec.Command(r.conmonPath, args...)
|
||||
|
||||
|
|
|
|||
|
|
@ -401,6 +401,28 @@ function teardown() {
|
|||
stop_crio
|
||||
}
|
||||
|
||||
@test "ctr execsync conflicting with conmon flags parsing" {
|
||||
start_crio
|
||||
run crioctl pod run --config "$TESTDATA"/sandbox_config.json
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
pod_id="$output"
|
||||
run crioctl ctr create --config "$TESTDATA"/container_redis.json --pod "$pod_id"
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
ctr_id="$output"
|
||||
run crioctl ctr start --id "$ctr_id"
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
run crioctl ctr execsync --id "$ctr_id" sh -c "echo hello world"
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
[[ "$output" =~ "hello world" ]]
|
||||
cleanup_ctrs
|
||||
cleanup_pods
|
||||
stop_crio
|
||||
}
|
||||
|
||||
@test "ctr execsync" {
|
||||
start_crio
|
||||
run crioctl pod run --config "$TESTDATA"/sandbox_config.json
|
||||
|
|
@ -511,19 +533,22 @@ function teardown() {
|
|||
run crioctl ctr start --id "$ctr_id"
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
run crioctl ctr execsync --id "$ctr_id" "echo hello0 stdout"
|
||||
run crioctl ctr execsync --id "$ctr_id" echo hello0 stdout
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
[[ "$output" == *"$(printf "Stdout:\nhello0 stdout")"* ]]
|
||||
run crioctl ctr execsync --id "$ctr_id" "echo hello1 stderr >&2"
|
||||
|
||||
stderrconfig=$(cat "$TESTDATA"/container_config.json | python -c 'import json,sys;obj=json.load(sys.stdin);obj["image"]["image"] = "runcom/stderr-test"; json.dump(obj, sys.stdout)')
|
||||
echo "$stderrconfig" > "$TESTDIR"/container_config_stderr.json
|
||||
run crioctl ctr create --config "$TESTDIR"/container_config_stderr.json --pod "$pod_id"
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
[[ "$output" == *"$(printf "Stderr:\nhello1 stderr")"* ]]
|
||||
run crioctl ctr execsync --id "$ctr_id" "echo hello2 stderr >&2; echo hello3 stdout"
|
||||
ctr_id="$output"
|
||||
run crioctl ctr execsync --id "$ctr_id" stderr
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
[[ "$output" == *"$(printf "Stderr:\nhello2 stderr")"* ]]
|
||||
[[ "$output" == *"$(printf "Stdout:\nhello3 stdout")"* ]]
|
||||
[[ "$output" == *"$(printf "Stderr:\nthis goes to stderr")"* ]]
|
||||
|
||||
run crioctl pod remove --id "$pod_id"
|
||||
echo "$output"
|
||||
[ "$status" -eq 0 ]
|
||||
|
|
|
|||
|
|
@ -77,6 +77,16 @@ if ! [ -d "$ARTIFACTS_PATH"/redis-image ]; then
|
|||
fi
|
||||
fi
|
||||
|
||||
# Make sure we have a copy of the runcom/stderr-test image.
|
||||
if ! [ -d "$ARTIFACTS_PATH"/stderr-test ]; then
|
||||
mkdir -p "$ARTIFACTS_PATH"/stderr-test
|
||||
if ! "$COPYIMG_BINARY" --import-from=docker://runcom/stderr-test:latest --export-to=dir:"$ARTIFACTS_PATH"/stderr-test --signature-policy="$INTEGRATION_ROOT"/policy.json ; then
|
||||
echo "Error pulling docker://stderr-test"
|
||||
rm -fr "$ARTIFACTS_PATH"/stderr-test
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
|
||||
# Make sure we have a copy of the busybox:latest image.
|
||||
if ! [ -d "$ARTIFACTS_PATH"/busybox-image ]; then
|
||||
mkdir -p "$ARTIFACTS_PATH"/busybox-image
|
||||
|
|
@ -159,6 +169,8 @@ function start_crio() {
|
|||
fi
|
||||
"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=redis:alpine --import-from=dir:"$ARTIFACTS_PATH"/redis-image --add-name=docker.io/library/redis:alpine --signature-policy="$INTEGRATION_ROOT"/policy.json
|
||||
"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=mrunalp/oom --import-from=dir:"$ARTIFACTS_PATH"/oom-image --add-name=docker.io/library/mrunalp/oom --signature-policy="$INTEGRATION_ROOT"/policy.json
|
||||
"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=busybox:latest --import-from=dir:"$ARTIFACTS_PATH"/busybox-image --add-name=docker.io/library/busybox:latest --signature-policy="$INTEGRATION_ROOT"/policy.json
|
||||
"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=runcom/stderr-test:latest --import-from=dir:"$ARTIFACTS_PATH"/stderr-test --add-name=docker.io/runcom/stderr-test:latest --signature-policy="$INTEGRATION_ROOT"/policy.json
|
||||
"$CRIO_BINARY" --conmon "$CONMON_BINARY" --listen "$CRIO_SOCKET" --cgroup-manager "$CGROUP_MANAGER" --runtime "$RUNTIME_BINARY" --root "$TESTDIR/crio" --runroot "$TESTDIR/crio-run" $STORAGE_OPTS --seccomp-profile "$seccomp" --apparmor-profile "$apparmor" --cni-config-dir "$CRIO_CNI_CONFIG" --signature-policy "$INTEGRATION_ROOT"/policy.json --config /dev/null config >$CRIO_CONFIG
|
||||
|
||||
# Prepare the CNI configuration files, we're running with non host networking by default
|
||||
|
|
@ -177,15 +189,10 @@ function start_crio() {
|
|||
crioctl image pull redis:alpine
|
||||
fi
|
||||
REDIS_IMAGEID=$(crioctl image status --id=redis:alpine | head -1 | sed -e "s/ID: //g")
|
||||
run crioctl image status --id=busybox
|
||||
if [ "$status" -ne 0 ] ; then
|
||||
crioctl image pull busybox:latest
|
||||
fi
|
||||
run crioctl image status --id=mrunalp/oom
|
||||
if [ "$status" -ne 0 ] ; then
|
||||
crioctl image pull mrunalp/oom
|
||||
fi
|
||||
|
||||
#
|
||||
#
|
||||
#
|
||||
|
|
@ -204,6 +211,15 @@ function start_crio() {
|
|||
#
|
||||
#
|
||||
#
|
||||
run crioctl image status --id=runcom/stderr-test
|
||||
if [ "$status" -ne 0 ] ; then
|
||||
crioctl image pull runcom/stderr-test:latest
|
||||
fi
|
||||
STDERR_IMAGEID=$(crioctl image status --id=runcom/stderr-test | head -1 | sed -e "s/ID: //g")
|
||||
run crioctl image status --id=busybox
|
||||
if [ "$status" -ne 0 ] ; then
|
||||
crioctl image pull busybox:latest
|
||||
fi
|
||||
BUSYBOX_IMAGEID=$(crioctl image status --id=busybox | head -1 | sed -e "s/ID: //g")
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue