runc: remove --criu option

This was introduced in an initial commit, and is not really needed for most of the users, since they rely on CRIU binary provided by their distro. Now, if someone needs a custom criu binary to be used, they can put it into some directory and make sure this directory is listed first in $PATH. Make --criu a hidden option (thus removing it from help). Remove it from man pages, integration tests, etc. Remove all traces of CriuPath from data structures. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
opencontainers · Jan 25, 2022 · 5ce43c4 · 5ce43c4
1 parent 13c6cae
commit 5ce43c4
Show file tree

Hide file tree

Showing 9 changed files with 44 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Deprecated
+
+ * `runc` option `--criu` is now ignored (with a warning), and the option will
+   be removed entirely in a future release. Users who need a non-standard
+   `criu` binary should rely on the standard way of looking up binaries in
+   `$PATH`. (#3316)
+
 ### Changed
 
  * When Intel RDT feature is not available, its initialization is skipped,

diff --git a/contrib/completions/bash/runc b/contrib/completions/bash/runc
@@ -231,12 +231,11 @@ _runc_runc() {
 		--log
 		--log-format
 		--root
-		--criu
 		--rootless
 	"
 
 	case "$prev" in
-	--log | --root | --criu)
+	--log | --root)
 		case "$cur" in
 		*:*) ;; # TODO somehow do _filedir for stuff inside the image, if it's already specified (which is also somewhat difficult to determine)
 		'')

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
@@ -43,7 +43,6 @@ type Container struct {
 	intelRdtManager      *intelrdt.Manager
 	initProcess          parentProcess
 	initProcessStartTime uint64
-	criuPath             string
 	m                    sync.Mutex
 	criuVersion          int
 	state                containerState
@@ -834,7 +833,6 @@ func (c *Container) checkCriuVersion(minVersion int) error {
 	}
 
 	criu := criu.MakeCriu()
-	criu.SetCriuPath(c.criuPath)
 	var err error
 	c.criuVersion, err = criu.GetCriuVersion()
 	if err != nil {
@@ -1615,9 +1613,9 @@ func (c *Container) criuSwrk(process *Process, req *criurpc.CriuReq, opts *CriuO
 	if c.criuVersion != 0 {
 		// If the CRIU Version is still '0' then this is probably
 		// the initial CRIU run to detect the version. Skip it.
-		logrus.Debugf("Using CRIU %d at: %s", c.criuVersion, c.criuPath)
+		logrus.Debugf("Using CRIU %d", c.criuVersion)
 	}
-	cmd := exec.Command(c.criuPath, args...)
+	cmd := exec.Command("criu", args...)
 	if process != nil {
 		cmd.Stdin = process.Stdin
 		cmd.Stdout = process.Stdout

diff --git a/libcontainer/factory.go b/libcontainer/factory.go
@@ -31,19 +31,14 @@ func New(root string) (*Factory, error) {
 		return nil, err
 	}
 	return &Factory{
-		Root:     root,
-		CriuPath: "criu",
+		Root: root,
 	}, nil
 }
 
 // Factory implements the default factory interface for linux based systems.
 type Factory struct {
 	// Root directory for the factory to store state.
 	Root string
-
-	// CriuPath is the path to the criu binary used for checkpoint and restore of
-	// containers.
-	CriuPath string
 }
 
 // Creates a new container with the given id and starts the initial process inside it.
@@ -117,7 +112,6 @@ func (l *Factory) Create(id string, config *configs.Config) (*Container, error)
 		id:              id,
 		root:            containerRoot,
 		config:          config,
-		criuPath:        l.CriuPath,
 		cgroupManager:   cm,
 		intelRdtManager: intelrdt.NewManager(config, id, ""),
 	}
@@ -157,7 +151,6 @@ func (l *Factory) Load(id string) (*Container, error) {
 		initProcessStartTime: state.InitProcessStartTime,
 		id:                   id,
 		config:               &state.Config,
-		criuPath:             l.CriuPath,
 		cgroupManager:        cm,
 		intelRdtManager:      intelrdt.NewManager(&state.Config, id, state.IntelRdtPath),
 		root:                 containerRoot,

diff --git a/main.go b/main.go
@@ -101,9 +101,9 @@ func main() {
 			Usage: "root directory for storage of container state (this should be located in tmpfs)",
 		},
 		cli.StringFlag{
-			Name:  "criu",
-			Value: "criu",
-			Usage: "path to the criu binary used for checkpoint and restore",
+			Name:   "criu",
+			Usage:  "(obsoleted; do not use)",
+			Hidden: true,
 		},
 		cli.BoolFlag{
 			Name:  "systemd-cgroup",
@@ -152,7 +152,15 @@ func main() {
 			return err
 		}
 
-		return configLogrus(context)
+		if err := configLogrus(context); err != nil {
+			return err
+		}
+
+		// TODO: remove this in runc 1.3.0.
+		if context.IsSet("criu") {
+			logrus.Warn("--criu value ignored (criu binary from $PATH is used); do not use")
+		}
+		return nil
 	}
 
 	// If the command returns an error, cli takes upon itself to print

diff --git a/man/runc.8.md b/man/runc.8.md
@@ -110,10 +110,6 @@ These options can be used with any command, and must precede the **command**.
 located on tmpfs. Default is */run/runc*, or *$XDG_RUNTIME_DIR/runc* for
 rootless containers.
 
-**--criu** _path_
-: Set the path to the **criu**(8) binary used for checkpoint and restore.
-Default is **criu**.
-
 **--systemd-cgroup**
 : Enable systemd cgroup support. If this is set, the container spec
 (_config.json_) is expected to have **cgroupsPath** value in the

diff --git a/tests/integration/checkpoint.bats b/tests/integration/checkpoint.bats
@@ -84,7 +84,7 @@ function runc_restore_with_pipes() {
 	shift
 
 	ret=0
-	__runc --criu "$CRIU" restore -d --work-path "$workdir" --image-path ./image-dir "$@" "$name" <&${in_r} >&${out_w} 2>&${err_w} || ret=$?
+	__runc restore -d --work-path "$workdir" --image-path ./image-dir "$@" "$name" <&${in_r} >&${out_w} 2>&${err_w} || ret=$?
 	if [ "$ret" -ne 0 ]; then
 		echo "__runc restore $name failed (status: $ret)"
 		exec {err_w}>&-
@@ -109,15 +109,15 @@ function simple_cr() {
 
 	for _ in $(seq 2); do
 		# checkpoint the running container
-		runc --criu "$CRIU" "$@" checkpoint --work-path ./work-dir test_busybox
+		runc "$@" checkpoint --work-path ./work-dir test_busybox
 		grep -B 5 Error ./work-dir/dump.log || true
 		[ "$status" -eq 0 ]
 
 		# after checkpoint busybox is no longer running
 		testcontainer test_busybox checkpointed
 
 		# restore from checkpoint
-		runc --criu "$CRIU" "$@" restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
+		runc "$@" restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
 		grep -B 5 Error ./work-dir/restore.log || true
 		[ "$status" -eq 0 ]
 
@@ -162,12 +162,12 @@ function simple_cr() {
 	testcontainer test_busybox running
 
 	# runc should fail with absolute parent image path.
-	runc --criu "$CRIU" checkpoint --parent-path "$(pwd)"/parent-dir --work-path ./work-dir --image-path ./image-dir test_busybox
+	runc checkpoint --parent-path "$(pwd)"/parent-dir --work-path ./work-dir --image-path ./image-dir test_busybox
 	[[ "${output}" == *"--parent-path"* ]]
 	[ "$status" -ne 0 ]
 
 	# runc should fail with invalid parent image path.
-	runc --criu "$CRIU" checkpoint --parent-path ./parent-dir --work-path ./work-dir --image-path ./image-dir test_busybox
+	runc checkpoint --parent-path ./parent-dir --work-path ./work-dir --image-path ./image-dir test_busybox
 	[[ "${output}" == *"--parent-path"* ]]
 	[ "$status" -ne 0 ]
 }
@@ -178,7 +178,7 @@ function simple_cr() {
 
 	#test checkpoint pre-dump
 	mkdir parent-dir
-	runc --criu "$CRIU" checkpoint --pre-dump --image-path ./parent-dir test_busybox
+	runc checkpoint --pre-dump --image-path ./parent-dir test_busybox
 	[ "$status" -eq 0 ]
 
 	# busybox should still be running
@@ -187,7 +187,7 @@ function simple_cr() {
 	# checkpoint the running container
 	mkdir image-dir
 	mkdir work-dir
-	runc --criu "$CRIU" checkpoint --parent-path ../parent-dir --work-path ./work-dir --image-path ./image-dir test_busybox
+	runc checkpoint --parent-path ../parent-dir --work-path ./work-dir --image-path ./image-dir test_busybox
 	grep -B 5 Error ./work-dir/dump.log || true
 	[ "$status" -eq 0 ]
 
@@ -203,7 +203,7 @@ function simple_cr() {
 
 @test "checkpoint --lazy-pages and restore" {
 	# check if lazy-pages is supported
-	if ! "${CRIU}" check --feature uffd-noncoop; then
+	if ! criu check --feature uffd-noncoop; then
 		skip "this criu does not support lazy migration"
 	fi
 
@@ -224,7 +224,7 @@ function simple_cr() {
 	# TCP port for lazy migration
 	port=27277
 
-	__runc --criu "$CRIU" checkpoint --lazy-pages --page-server 0.0.0.0:${port} --status-fd ${lazy_w} --work-path ./work-dir --image-path ./image-dir test_busybox &
+	__runc checkpoint --lazy-pages --page-server 0.0.0.0:${port} --status-fd ${lazy_w} --work-path ./work-dir --image-path ./image-dir test_busybox &
 	cpt_pid=$!
 
 	# wait for lazy page server to be ready
@@ -242,7 +242,7 @@ function simple_cr() {
 	[ -e image-dir/inventory.img ]
 
 	# Start CRIU in lazy-daemon mode
-	${CRIU} lazy-pages --page-server --address 127.0.0.1 --port ${port} -D image-dir &
+	criu lazy-pages --page-server --address 127.0.0.1 --port ${port} -D image-dir &
 	lp_pid=$!
 
 	# Restore lazily from checkpoint.
@@ -264,7 +264,7 @@ function simple_cr() {
 
 @test "checkpoint and restore in external network namespace" {
 	# check if external_net_ns is supported; only with criu 3.10++
-	if ! "${CRIU}" check --feature external_net_ns; then
+	if ! criu check --feature external_net_ns; then
 		# this criu does not support external_net_ns; skip the test
 		skip "this criu does not support external network namespaces"
 	fi
@@ -290,15 +290,15 @@ function simple_cr() {
 	for _ in $(seq 2); do
 		# checkpoint the running container; this automatically tells CRIU to
 		# handle the network namespace defined in config.json as an external
-		runc --criu "$CRIU" checkpoint --work-path ./work-dir test_busybox
+		runc checkpoint --work-path ./work-dir test_busybox
 		grep -B 5 Error ./work-dir/dump.log || true
 		[ "$status" -eq 0 ]
 
 		# after checkpoint busybox is no longer running
 		testcontainer test_busybox checkpointed
 
 		# restore from checkpoint; this should restore the container into the existing network namespace
-		runc --criu "$CRIU" restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
+		runc restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
 		grep -B 5 Error ./work-dir/restore.log || true
 		[ "$status" -eq 0 ]
 
@@ -341,7 +341,7 @@ function simple_cr() {
 	testcontainer test_busybox running
 
 	# checkpoint the running container
-	runc --criu "$CRIU" checkpoint --work-path ./work-dir test_busybox
+	runc checkpoint --work-path ./work-dir test_busybox
 	grep -B 5 Error ./work-dir/dump.log || true
 	[ "$status" -eq 0 ]
 	! test -f ./work-dir/"$tmplog1"
@@ -352,7 +352,7 @@ function simple_cr() {
 
 	test -f ./work-dir/"$tmplog2" && unlink ./work-dir/"$tmplog2"
 	# restore from checkpoint
-	runc --criu "$CRIU" restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
+	runc restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
 	grep -B 5 Error ./work-dir/restore.log || true
 	[ "$status" -eq 0 ]
 	! test -f ./work-dir/"$tmplog1"
@@ -386,7 +386,7 @@ function simple_cr() {
 	testcontainer test_busybox running
 
 	# checkpoint the running container
-	runc --criu "$CRIU" checkpoint --work-path ./work-dir test_busybox
+	runc checkpoint --work-path ./work-dir test_busybox
 	grep -B 5 Error ./work-dir/dump.log || true
 	[ "$status" -eq 0 ]
 
@@ -398,7 +398,7 @@ function simple_cr() {
 	rm -rf "${bind1:?}"/*
 
 	# restore from checkpoint
-	runc --criu "$CRIU" restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
+	runc restore -d --work-path ./work-dir --console-socket "$CONSOLE_SOCKET" test_busybox
 	grep -B 5 Error ./work-dir/restore.log || true
 	[ "$status" -eq 0 ]
 

diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash
@@ -23,8 +23,8 @@ SECCOMP_AGENT="${INTEGRATION_ROOT}/../../contrib/cmd/seccompagent/seccompagent"
 # shellcheck disable=SC2034
 TESTDATA="${INTEGRATION_ROOT}/testdata"
 
-# CRIU PATH
-CRIU="$(which criu 2>/dev/null || true)"
+# Whether we have criu binary.
+command -v criu &>/dev/null && HAVE_CRIU=yes
 
 # Kernel version
 KERNEL_VERSION="$(uname -r)"
@@ -350,7 +350,7 @@ function requires() {
 		local skip_me
 		case $var in
 		criu)
-			if [ ! -e "$CRIU" ]; then
+			if [ -n "$HAVE_CRIU" ]; then
 				skip_me=1
 			fi
 			;;

diff --git a/utils_linux.go b/utils_linux.go
@@ -30,16 +30,7 @@ func loadFactory(context *cli.Context) (*libcontainer.Factory, error) {
 		return nil, err
 	}
 
-	f, err := libcontainer.New(abs)
-	if err != nil {
-		return nil, err
-	}
-
-	if criu := context.GlobalString("criu"); criu != "" {
-		f.CriuPath = criu
-	}
-
-	return f, nil
+	return libcontainer.New(abs)
 }
 
 // getContainer returns the specified container instance by loading it from state