Merge pull request #3353 from kolyshkin/rm-criu-opt

runc: remove --criu option

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,

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)
 		'')

libcontainer/container_linux.go

@@ -45,7 +45,6 @@ type linuxContainer struct {
 	initArgs             []string
 	initProcess          parentProcess
 	initProcessStartTime uint64
-	criuPath             string
 	newuidmapPath        string
 	newgidmapPath        string
 	m                    sync.Mutex
@@ -829,7 +828,6 @@ func (c *linuxContainer) checkCriuVersion(minVersion int) error {
 	}
 
 	criu := criu.MakeCriu()
-	criu.SetCriuPath(c.criuPath)
 	var err error
 	c.criuVersion, err = criu.GetCriuVersion()
 	if err != nil {
@@ -1602,13 +1600,12 @@ func (c *linuxContainer) criuSwrk(process *Process, req *criurpc.CriuReq, opts *
 	criuServer := os.NewFile(uintptr(fds[1]), "criu-transport-server")
 	defer criuServer.Close()
 
-	args := []string{"swrk", "3"}
 	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", "swrk", "3")
 	if process != nil {
 		cmd.Stdin = process.Stdin
 		cmd.Stdout = process.Stdout

libcontainer/factory_linux.go

@@ -76,15 +76,6 @@ func TmpfsRoot(l *LinuxFactory) error {
 	return nil
 }
 
-// CriuPath returns an option func to configure a LinuxFactory with the
-// provided criupath
-func CriuPath(criupath string) func(*LinuxFactory) error {
-	return func(l *LinuxFactory) error {
-		l.CriuPath = criupath
-		return nil
-	}
-}
-
 // New returns a linux based container factory based in the root directory and
 // configures the factory with the provided option funcs.
 func New(root string, options ...func(*LinuxFactory) error) (Factory, error) {
@@ -98,7 +89,6 @@ func New(root string, options ...func(*LinuxFactory) error) (Factory, error) {
 		InitPath:  "/proc/self/exe",
 		InitArgs:  []string{os.Args[0], "init"},
 		Validator: validate.New(),
-		CriuPath:  "criu",
 	}
 
 	for _, opt := range options {
@@ -125,10 +115,6 @@ type LinuxFactory struct {
 	// a container.
 	InitArgs []string
 
-	// CriuPath is the path to the criu binary used for checkpoint and restore of
-	// containers.
-	CriuPath string
-
 	// New{u,g}idmapPath is the path to the binaries used for mapping with
 	// rootless containers.
 	NewuidmapPath string
@@ -204,7 +190,6 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err
 		config:        config,
 		initPath:      l.InitPath,
 		initArgs:      l.InitArgs,
-		criuPath:      l.CriuPath,
 		newuidmapPath: l.NewuidmapPath,
 		newgidmapPath: l.NewgidmapPath,
 		cgroupManager: cm,
@@ -248,7 +233,6 @@ func (l *LinuxFactory) Load(id string) (Container, error) {
 		config:               &state.Config,
 		initPath:             l.InitPath,
 		initArgs:             l.InitArgs,
-		criuPath:             l.CriuPath,
 		newuidmapPath:        l.NewuidmapPath,
 		newgidmapPath:        l.NewgidmapPath,
 		cgroupManager:        cm,

main.go

@@ -102,8 +102,8 @@ func main() {
 		},
 		cli.StringFlag{
 			Name:   "criu",
-			Value: "criu",
+			Usage:  "(obsoleted; do not use)",
-			Usage: "path to the criu binary used for checkpoint and restore",
+			Hidden: true,
 		},
 		cli.BoolFlag{
 			Name:  "systemd-cgroup",
@@ -151,6 +151,10 @@ func main() {
 		if err := reviseRootDir(context); err != nil {
 			return err
 		}
+		// TODO: remove this in runc 1.3.0.
+		if context.IsSet("criu") {
+			fmt.Fprintln(os.Stderr, "WARNING: --criu ignored (criu binary from $PATH is used); do not use")
+		}
 
 		return configLogrus(context)
 	}

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

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,7 +109,7 @@ 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 ]
 
@@ -117,7 +117,7 @@ function simple_cr() {
 		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,7 +290,7 @@ 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 ]
 
@@ -298,7 +298,7 @@ function simple_cr() {
 		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 ]
 

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
+# Whether we have criu binary.
-CRIU="$(which criu 2>/dev/null || true)"
+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
 			;;

utils_linux.go

@@ -47,7 +47,6 @@ func loadFactory(context *cli.Context) (libcontainer.Factory, error) {
 	}
 
 	return libcontainer.New(abs, intelRdtManager,
-		libcontainer.CriuPath(context.GlobalString("criu")),
 		libcontainer.NewuidmapPath(newuidmap),
 		libcontainer.NewgidmapPath(newgidmap))
 }