Merge pull request #72 from AkihiroSuda/dev-fix-restart

fix restarting rootless containers

Author: AkihiroSuda

.github/workflows/test.yml

@@ -31,7 +31,7 @@ jobs:
     - name: "Prepare test environment"
       run: DOCKER_BUILDKIT=1 docker build -t test --target test .
     - name: "Test"
-      run: docker run -t --rm --privileged test
+      run: docker run -t --rm --privileged test go test -v -test.kill-daemon ./...
 
   cross:
     runs-on: ubuntu-20.04
@@ -57,7 +57,7 @@ jobs:
       with:
         fetch-depth: 1
     - name: "Ensure that the test suite is compatible with Docker"
-      run: go test -v -exec sudo -test.target=docker .
+      run: go test -v -exec sudo -test.target=docker -test.kill-daemon .
 
   test-cgroup2:
     name: "Cgroup2 + rootless"
@@ -83,6 +83,6 @@ jobs:
     - name: "Install rootless containerd"
       run: ssh default -- containerd-rootless-setuptool.sh install
     - name: "Run tests (rootless)"
-      run: ssh default -- "CONTAINERD_SNAPSHOTTER=native /vagrant/nerdctl.test -test.v"
+      run: ssh default -- "CONTAINERD_SNAPSHOTTER=native /vagrant/nerdctl.test -test.v -test.kill-daemon"
     - name: "Uninstall rootless containerd"
       run: ssh default -- containerd-rootless-setuptool.sh uninstall

README.md

@@ -116,8 +116,8 @@ In addition to containerd, the following components should be installed (optiona
 - [CNI isolation plugin](https://github.com/AkihiroSuda/cni-isolation): for isolating bridge networks (`nerdctl network create`)
 - [BuildKit](https://github.com/moby/buildkit): for using `nerdctl build`. BuildKit daemon (`buildkitd`) needs to be running.
 - [RootlessKit](https://github.com/rootless-containers/rootlesskit) and [slirp4netns](https://github.com/rootless-containers/slirp4netns): for [Rootless mode](./docs/rootless.md)
-   - RootlessKit needs to be v0.10.0 or later
-   - slirp4netns needs toe be v0.4.0 or later
+   - RootlessKit needs to be v0.10.0 or later. v0.13.2 or later is recommended.
+   - slirp4netns needs toe be v0.4.0 or later. v1.1.7 or later is recommended.
 
 To run nerdctl inside Docker:
 ```bash

Vagrantfile

@@ -45,7 +45,7 @@ Vagrant.configure("2") do |config|
     chmod +x /usr/local/sbin/runc
 
     # Install RootlessKit
-    ROOTLESSKIT_VERSION=0.13.1
+    ROOTLESSKIT_VERSION=0.13.2
     curl -sSL https://github.com/rootless-containers/rootlesskit/releases/download/v${ROOTLESSKIT_VERSION}/rootlesskit-$(uname -m).tar.gz | tar Cxzv /usr/local/bin
 
     # Delegate cgroup v2 controllers

extras/rootless/containerd-rootless.sh

@@ -25,8 +25,8 @@
 # External dependencies:
 # * newuidmap and newgidmap needs to be installed.
 # * /etc/subuid and /etc/subgid needs to be configured for the current user.
-# * RootlessKit (>= v0.10.0) needs to be installed
-# * Either one of slirp4netns (>= v0.4.0), VPNKit, lxc-user-nic needs to be installed.
+# * RootlessKit (>= v0.10.0) needs to be installed. RootlessKit >= v0.13.2 is recommended.
+# * Either one of slirp4netns (>= v0.4.0), VPNKit, lxc-user-nic needs to be installed. slirp4netns >= v1.1.7 is recommended.
 #
 # Recognized environment variables:
 # * CONTAINERD_ROOTLESS_ROOTLESSKIT_STATE_DIR=DIR: the rootlesskit state dir. Defaults to "$XDG_RUNTIME_DIR/containerd-rootless".

pkg/dnsutil/hostsstore/hostsstore.go

@@ -64,9 +64,9 @@ func ensureFile(path string) error {
 	return err
 }
 
-// EnsureHostsFile is used for creating mount-bindable /etc/hosts file.
+// AllocHostsFile is used for creating mount-bindable /etc/hosts file.
 // The file is initialized with no content.
-func EnsureHostsFile(dataStore, ns, id string) (string, error) {
+func AllocHostsFile(dataStore, ns, id string) (string, error) {
 	lockDir := filepath.Join(dataStore, hostsDirBasename)
 	if err := os.MkdirAll(lockDir, 0700); err != nil {
 		return "", err
@@ -79,6 +79,18 @@ func EnsureHostsFile(dataStore, ns, id string) (string, error) {
 	return path, err
 }
 
+func DeallocHostsFile(dataStore, ns, id string) error {
+	lockDir := filepath.Join(dataStore, hostsDirBasename)
+	if err := os.MkdirAll(lockDir, 0700); err != nil {
+		return err
+	}
+	dirToBeRemoved := filepath.Dir(HostsPath(dataStore, ns, id))
+	fn := func() error {
+		return os.RemoveAll(dirToBeRemoved)
+	}
+	return lockutil.WithDirLock(lockDir, fn)
+}
+
 func NewStore(dataStore string) (Store, error) {
 	store := &store{
 		dataStore: dataStore,
@@ -128,11 +140,15 @@ func (x *store) Acquire(meta Meta) error {
 
 func (x *store) Release(ns, id string) error {
 	fn := func() error {
-		d := filepath.Join(x.hostsD, ns, id)
-		if _, err := os.Stat(d); errors.Is(err, os.ErrNotExist) {
+		metaPath := filepath.Join(x.hostsD, ns, id, metaJSON)
+		if _, err := os.Stat(metaPath); errors.Is(err, os.ErrNotExist) {
 			return nil
 		}
-		if err := os.RemoveAll(d); err != nil {
+		// We remove "meta.json" but we still retain the "hosts" file
+		// because it is needed for restarting. The "hosts" is removed on
+		// `nerdctl rm`.
+		// https://github.com/rootless-containers/rootlesskit/issues/220#issuecomment-783224610
+		if err := os.RemoveAll(metaPath); err != nil {
 			return err
 		}
 		return newUpdater(x.hostsD).update()

pkg/testutil/testutil.go

@@ -23,16 +23,19 @@ import (
 	"os"
 	"os/exec"
 	"testing"
+	"time"
 
+	"github.com/pkg/errors"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/icmd"
 )
 
 type Base struct {
-	T      testing.TB
-	Target Target
-	Binary string
-	Args   []string
+	T                testing.TB
+	Target           Target
+	DaemonIsKillable bool
+	Binary           string
+	Args             []string
 }
 
 func (b *Base) Cmd(args ...string) *Cmd {
@@ -44,13 +47,75 @@ func (b *Base) Cmd(args ...string) *Cmd {
 	return cmd
 }
 
+func (b *Base) systemctlTarget() string {
+	switch b.Target {
+	case Nerdctl:
+		return "containerd.service"
+	case Docker:
+		return "docker.service"
+	default:
+		b.T.Fatalf("unexpected target %q", b.Target)
+		return ""
+	}
+}
+
+func (b *Base) systemctlArgs() []string {
+	var systemctlArgs []string
+	if os.Geteuid() != 0 {
+		systemctlArgs = append(systemctlArgs, "--user")
+	}
+	return systemctlArgs
+}
+
+func (b *Base) KillDaemon() {
+	b.T.Helper()
+	if !b.DaemonIsKillable {
+		b.T.Skip("daemon is not killable (hint: set \"-test.kill-daemon\")")
+	}
+	target := b.systemctlTarget()
+	b.T.Logf("killing %q", target)
+	cmdKill := exec.Command("systemctl",
+		append(b.systemctlArgs(),
+			[]string{"kill", "-s", "KILL", target}...)...)
+	if out, err := cmdKill.CombinedOutput(); err != nil {
+		err = errors.Wrapf(err, "cannot kill %q: %q", target, string(out))
+		b.T.Fatal(err)
+	}
+	// the daemon should restart automatically
+}
+
+func (b *Base) EnsureDaemonActive() {
+	b.T.Helper()
+	target := b.systemctlTarget()
+	b.T.Logf("checking activity of %q", target)
+	systemctlArgs := b.systemctlArgs()
+	const (
+		maxRetry = 30
+		sleep    = 3 * time.Second
+	)
+	for i := 0; i < maxRetry; i++ {
+		cmd := exec.Command("systemctl",
+			append(systemctlArgs,
+				[]string{"is-active", target}...)...)
+		out, err := cmd.CombinedOutput()
+		b.T.Logf("(retry=%d) %s", i, string(out))
+		if err == nil {
+			b.T.Logf("daemon %q is now running", target)
+			return
+		}
+		time.Sleep(sleep)
+	}
+	b.T.Fatalf("daemon %q not running", target)
+}
+
 type Cmd struct {
 	icmd.Cmd
 	*Base
 	DockerIncompatible bool
 }
 
 func (c *Cmd) Run() *icmd.Result {
+	c.Base.T.Helper()
 	if c.Base.Target == Docker && c.DockerIncompatible {
 		c.Base.T.Skip("test is incompatible with Docker")
 	}
@@ -91,10 +156,14 @@ const (
 	Docker  = Target("docker")
 )
 
-var flagTestTarget Target
+var (
+	flagTestTarget     Target
+	flagTestKillDaemon bool
+)
 
 func M(m *testing.M) {
 	flag.StringVar(&flagTestTarget, "test.target", Nerdctl, "target to test")
+	flag.BoolVar(&flagTestKillDaemon, "test.kill-daemon", false, "enable tests that kill the daemon")
 	flag.Parse()
 	fmt.Printf("test target: %q\n", flagTestTarget)
 	os.Exit(m.Run())
@@ -107,12 +176,17 @@ func GetTarget() string {
 	return flagTestTarget
 }
 
+func GetDaemonIsKillable() bool {
+	return flagTestKillDaemon
+}
+
 const Namespace = "nerdctl-test"
 
 func NewBase(t *testing.T) *Base {
 	base := &Base{
-		T:      t,
-		Target: GetTarget(),
+		T:                t,
+		Target:           GetTarget(),
+		DaemonIsKillable: GetDaemonIsKillable(),
 	}
 	var err error
 	switch base.Target {
@@ -136,6 +210,6 @@ func NewBase(t *testing.T) *Base {
 // use GCR mirror to avoid hitting Docker Hub rate limit
 const (
 	AlpineImage                 = "mirror.gcr.io/library/alpine:3.13"
-	NginxAlpineImage            = "mirror.gcr.io/library/nginx:1.19.6-alpine"
+	NginxAlpineImage            = "mirror.gcr.io/library/nginx:1.19-alpine"
 	NginxAlpineIndexHTMLSnippet = "<title>Welcome to nginx!</title>"
 )

rm.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"os"
 
+	"github.com/AkihiroSuda/nerdctl/pkg/dnsutil/hostsstore"
 	"github.com/AkihiroSuda/nerdctl/pkg/idutil/containerwalker"
 	"github.com/AkihiroSuda/nerdctl/pkg/labels"
 	"github.com/AkihiroSuda/nerdctl/pkg/namestore"
@@ -65,7 +66,8 @@ func rmAction(clicontext *cli.Context) error {
 
 	force := clicontext.Bool("force")
 
-	containerNameStore, err := namestore.New(dataStore, clicontext.String("namespace"))
+	ns := clicontext.String("namespace")
+	containerNameStore, err := namestore.New(dataStore, ns)
 	if err != nil {
 		return err
 	}
@@ -77,7 +79,7 @@ func rmAction(clicontext *cli.Context) error {
 			if err != nil {
 				return err
 			}
-			err = removeContainer(clicontext, ctx, client, found.Container.ID(), found.Req, force, stateDir, containerNameStore)
+			err = removeContainer(clicontext, ctx, client, ns, found.Container.ID(), found.Req, force, dataStore, stateDir, containerNameStore)
 			return err
 		},
 	}
@@ -93,7 +95,8 @@ func rmAction(clicontext *cli.Context) error {
 }
 
 // removeContainer returns nil when the container cannot be found
-func removeContainer(clicontext *cli.Context, ctx context.Context, client *containerd.Client, id, req string, force bool, stateDir string, namst namestore.NameStore) (retErr error) {
+// FIXME: refactoring
+func removeContainer(clicontext *cli.Context, ctx context.Context, client *containerd.Client, ns, id, req string, force bool, dataStore, stateDir string, namst namestore.NameStore) (retErr error) {
 	var name string
 	defer func() {
 		if errdefs.IsNotFound(retErr) {
@@ -111,6 +114,11 @@ func removeContainer(clicontext *cli.Context, ctx context.Context, client *conta
 		} else {
 			logrus.WithError(retErr).Warnf("failed to remove container %q", id)
 		}
+		if retErr == nil {
+			retErr = hostsstore.DeallocHostsFile(dataStore, ns, id)
+		} else {
+			logrus.WithError(retErr).Warnf("failed to release name store for container %q", id)
+		}
 	}()
 	container, err := client.LoadContainer(ctx, id)
 	if err != nil {

run.go

@@ -39,7 +39,6 @@ import (
 	"github.com/AkihiroSuda/nerdctl/pkg/namestore"
 	"github.com/AkihiroSuda/nerdctl/pkg/netutil"
 	"github.com/AkihiroSuda/nerdctl/pkg/portutil"
-	"github.com/AkihiroSuda/nerdctl/pkg/rootlessutil"
 	"github.com/containerd/console"
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/cio"
@@ -223,6 +222,8 @@ func runAction(clicontext *cli.Context) error {
 		return errors.New("image name needs to be specified")
 	}
 
+	ns := clicontext.String("namespace")
+
 	client, ctx, cancel, err := newClient(clicontext)
 	if err != nil {
 		return err
@@ -376,7 +377,7 @@ func runAction(clicontext *cli.Context) error {
 			return err
 		}
 		// the content of /etc/hosts is created in OCI Hook
-		etcHostsPath, err := hostsstore.EnsureHostsFile(dataStore, clicontext.String("namespace"), id)
+		etcHostsPath, err := hostsstore.AllocHostsFile(dataStore, ns, id)
 		if err != nil {
 			return err
 		}
@@ -446,15 +447,15 @@ func runAction(clicontext *cli.Context) error {
 	var containerNameStore namestore.NameStore
 	name := clicontext.String("name")
 	if name != "" {
-		containerNameStore, err = namestore.New(dataStore, clicontext.String("namespace"))
+		containerNameStore, err = namestore.New(dataStore, ns)
 		if err != nil {
 			return err
 		}
 		if err := containerNameStore.Acquire(name, id); err != nil {
 			return err
 		}
 	}
-	ilOpt, err := withInternalLabels(clicontext.String("namespace"), name, hostname, stateDir, clicontext.StringSlice("network"), ports)
+	ilOpt, err := withInternalLabels(ns, name, hostname, stateDir, clicontext.StringSlice("network"), ports)
 	if err != nil {
 		return err
 	}
@@ -475,7 +476,7 @@ func runAction(clicontext *cli.Context) error {
 			return errors.New("flag -d and --rm cannot be specified together")
 		}
 		defer func() {
-			if removeErr := removeContainer(clicontext, ctx, client, id, id, true, stateDir, containerNameStore); removeErr != nil {
+			if removeErr := removeContainer(clicontext, ctx, client, ns, id, id, true, dataStore, stateDir, containerNameStore); removeErr != nil {
 				logrus.WithError(removeErr).Warnf("failed to remove container %s", id)
 			}
 		}()
@@ -617,12 +618,6 @@ func generateRestartOpts(restartFlag, logURI string) ([]containerd.NewContainerO
 	case "", "no":
 		return nil, nil
 	case "always":
-		if rootlessutil.IsRootless() {
-			// $ systemctl --user kill -s KILL containerd
-			// $ nerdctl info
-			// FATA[0000] stat /run/user/1001/containerd-rootless/child_pid: no such file or directory
-			logrus.Warn("FIXME: --restart=always is broken on rootless")
-		}
 		opts := []containerd.NewContainerOpts{restart.WithStatus(containerd.Running)}
 		if logURI != "" {
 			opts = append(opts, restart.WithLogURIString(logURI))

run_network_test.go

@@ -26,6 +26,7 @@ import (
 	"time"
 
 	"github.com/AkihiroSuda/nerdctl/pkg/testutil"
+	"github.com/containerd/containerd/errdefs"
 	"github.com/pkg/errors"
 	"gotest.tools/v3/assert"
 )
@@ -165,6 +166,9 @@ func httpGet(urlStr string, attempts int) (*http.Response, error) {
 		resp *http.Response
 		err  error
 	)
+	if attempts < 1 {
+		return nil, errdefs.ErrInvalidArgument
+	}
 	for i := 0; i < attempts; i++ {
 		resp, err = http.Get(urlStr)
 		if err == nil {