From 2c2745e85751f467bd9089830d8bd8bc33d063ed Mon Sep 17 00:00:00 2001 From: Hayato Kiwata Date: Tue, 24 Sep 2024 00:01:03 +0900 Subject: [PATCH] fix: Cleaning up orphaned directories and files when containers creation fails When trying to create new containers, the following directories are created based on the new container ID: - `/var/lib/nerdctl/1935db59/containers//` - `/var/lib/nerdctl/1935db59/etchosts//` When containers with existing names are attempted to be created, the processes fail. However, in the events of failures, these directories are not cleaned up. This issue is reported in the following: - https://github.com/containerd/nerdctl/issues/2993 This commit resolves the issue by cleaning up the mentioned directories when containers creation fails. Also, It has also been modified so that the following directory is also cleaned up when the container is removed. - `/var/lib/nerdctl/1935db59/etchosts//` Note that tests of logic to fix bug in Issue #2993 are added based on the following testing principles. - https://github.com/containerd/nerdctl/blob/main/docs/testing/tools.md Signed-off-by: Hayato Kiwata --- .../container/container_create_linux_test.go | 126 ++++++++++++++++++ pkg/cmd/container/create.go | 70 ++++++---- pkg/cmd/container/remove.go | 2 +- pkg/dnsutil/hostsstore/hostsstore.go | 15 +-- 4 files changed, 181 insertions(+), 32 deletions(-) diff --git a/cmd/nerdctl/container/container_create_linux_test.go b/cmd/nerdctl/container/container_create_linux_test.go index e3e2122bf80..234b2170db1 100644 --- a/cmd/nerdctl/container/container_create_linux_test.go +++ b/cmd/nerdctl/container/container_create_linux_test.go @@ -17,14 +17,22 @@ package container import ( + "errors" "fmt" + "os" + "path/filepath" "strings" "testing" + "github.com/opencontainers/go-digest" "gotest.tools/v3/assert" + "github.com/containerd/containerd/v2/defaults" + "github.com/containerd/nerdctl/v2/pkg/testutil" + "github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest" "github.com/containerd/nerdctl/v2/pkg/testutil/nettestutil" + "github.com/containerd/nerdctl/v2/pkg/testutil/test" ) func TestCreate(t *testing.T) { @@ -174,3 +182,121 @@ func TestCreateWithTty(t *testing.T) { withTtyContainer := base.InspectContainer(withTtyContainerName) assert.Equal(base.T, 0, withTtyContainer.State.ExitCode) } + +// TestIssue2993 tests https://github.com/containerd/nerdctl/issues/2993 +func TestIssue2993(t *testing.T) { + testutil.DockerIncompatible(t) + + nerdtest.Setup() + + const ( + containersPathKey = "containersPath" + etchostsPathKey = "etchostsPath" + ) + + getAddrHash := func(addr string) string { + const addrHashLen = 8 + + d := digest.SHA256.FromString(addr) + h := d.Encoded()[0:addrHashLen] + + return h + } + + testCase := &test.Group{ + { + Description: "Issue #2993 - nerdctl no longer leaks containers and etchosts directories and files when container creation fails.", + Require: nerdtest.Private, + Setup: func(data test.Data, helpers test.Helpers) { + helpers.Ensure("run", "--name", data.Identifier(), "-d", testutil.AlpineImage, "sleep", "infinity") + + dataRoot := string(data.ReadConfig(nerdtest.DataRoot)) + h := getAddrHash(defaults.DefaultAddress) + dataStore := filepath.Join(dataRoot, h) + namespace := data.Identifier() + + containersPath := filepath.Join(dataStore, "containers", namespace) + containersDirs, err := os.ReadDir(containersPath) + assert.NilError(t, err) + assert.Equal(t, len(containersDirs), 1) + + etchostsPath := filepath.Join(dataStore, "etchosts", namespace) + etchostsDirs, err := os.ReadDir(etchostsPath) + assert.NilError(t, err) + assert.Equal(t, len(etchostsDirs), 1) + + data.Set(containersPathKey, containersPath) + data.Set(etchostsPathKey, etchostsPath) + }, + Cleanup: func(data test.Data, helpers test.Helpers) { + helpers.Anyhow("rm", "-f", data.Identifier()) + }, + Command: func(data test.Data, helpers test.Helpers) test.Command { + return helpers.Command("run", "--name", data.Identifier(), "-d", testutil.AlpineImage, "sleep", "infinity") + }, + Expected: func(data test.Data, helpers test.Helpers) *test.Expected { + return &test.Expected{ + ExitCode: 1, + Errors: []error{errors.New("is already used by ID")}, + Output: func(stdout string, info string, t *testing.T) { + containersDirs, err := os.ReadDir(data.Get(containersPathKey)) + assert.NilError(t, err) + assert.Equal(t, len(containersDirs), 1) + + etchostsDirs, err := os.ReadDir(data.Get(etchostsPathKey)) + assert.NilError(t, err) + assert.Equal(t, len(etchostsDirs), 1) + }, + } + }, + }, + { + Description: "Issue #2993 - nerdctl no longer leaks containers and etchosts directories and files when containers are removed.", + Require: nerdtest.Private, + Setup: func(data test.Data, helpers test.Helpers) { + helpers.Ensure("run", "--name", data.Identifier(), "-d", testutil.AlpineImage, "sleep", "infinity") + + dataRoot := string(data.ReadConfig(nerdtest.DataRoot)) + h := getAddrHash(defaults.DefaultAddress) + dataStore := filepath.Join(dataRoot, h) + namespace := data.Identifier() + + containersPath := filepath.Join(dataStore, "containers", namespace) + containersDirs, err := os.ReadDir(containersPath) + assert.NilError(t, err) + assert.Equal(t, len(containersDirs), 1) + + etchostsPath := filepath.Join(dataStore, "etchosts", namespace) + etchostsDirs, err := os.ReadDir(etchostsPath) + assert.NilError(t, err) + assert.Equal(t, len(etchostsDirs), 1) + + data.Set(containersPathKey, containersPath) + data.Set(etchostsPathKey, etchostsPath) + }, + Cleanup: func(data test.Data, helpers test.Helpers) { + helpers.Anyhow("rm", "-f", data.Identifier()) + }, + Command: func(data test.Data, helpers test.Helpers) test.Command { + return helpers.Command("rm", "-f", data.Identifier()) + }, + Expected: func(data test.Data, helpers test.Helpers) *test.Expected { + return &test.Expected{ + ExitCode: 0, + Errors: []error{}, + Output: func(stdout string, info string, t *testing.T) { + containersDirs, err := os.ReadDir(data.Get(containersPathKey)) + assert.NilError(t, err) + assert.Equal(t, len(containersDirs), 0) + + etchostsDirs, err := os.ReadDir(data.Get(etchostsPathKey)) + assert.NilError(t, err) + assert.Equal(t, len(etchostsDirs), 0) + }, + } + }, + }, + } + + testCase.Run(t) +} diff --git a/pkg/cmd/container/create.go b/pkg/cmd/container/create.go index 328fe7e6f6a..ec23038274a 100644 --- a/pkg/cmd/container/create.go +++ b/pkg/cmd/container/create.go @@ -46,6 +46,7 @@ import ( "github.com/containerd/nerdctl/v2/pkg/cmd/image" "github.com/containerd/nerdctl/v2/pkg/cmd/volume" "github.com/containerd/nerdctl/v2/pkg/containerutil" + "github.com/containerd/nerdctl/v2/pkg/dnsutil/hostsstore" "github.com/containerd/nerdctl/v2/pkg/flagutil" "github.com/containerd/nerdctl/v2/pkg/idgen" "github.com/containerd/nerdctl/v2/pkg/imgutil" @@ -118,7 +119,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa platformOpts, err := setPlatformOptions(ctx, client, id, netManager.NetworkOptions().UTSNamespace, &internalLabels, options) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } opts = append(opts, platformOpts...) @@ -130,7 +131,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa } ocispecPlatforms, err := platformutil.NewOCISpecPlatformSlice(false, platformSS) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } rawRef := args[0] @@ -141,13 +142,13 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa ensuredImage, err = image.EnsureImage(ctx, client, rawRef, options.ImagePullOpt) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } } rootfsOpts, rootfsCOpts, err := generateRootfsOpts(args, id, ensuredImage, options) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } opts = append(opts, rootfsOpts...) cOpts = append(cOpts, rootfsCOpts...) @@ -158,12 +159,12 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa envs, err := flagutil.MergeEnvFileAndOSEnv(options.EnvFile, options.Env) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } if options.Interactive { if options.Detach { - return nil, nil, errors.New("currently flag -i and -d cannot be specified together (FIXME)") + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), errors.New("currently flag -i and -d cannot be specified together (FIXME)") } } @@ -174,7 +175,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa var mountOpts []oci.SpecOpts mountOpts, internalLabels.anonVolumes, internalLabels.mountPoints, err = generateMountOpts(ctx, client, ensuredImage, volStore, options) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } opts = append(opts, mountOpts...) @@ -186,30 +187,30 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa // 3, nerdctl start/restart demo logConfig, err := generateLogConfig(dataStore, id, options.LogDriver, options.LogOpt, options.GOptions.Namespace) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } internalLabels.logURI = logConfig.LogURI restartOpts, err := generateRestartOpts(ctx, client, options.Restart, logConfig.LogURI, options.InRun) if err != nil { - return nil, nil, err + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err } cOpts = append(cOpts, restartOpts...) if err = netManager.VerifyNetworkOptions(ctx); err != nil { - return nil, nil, fmt.Errorf("failed to verify networking settings: %s", err) + return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), fmt.Errorf("failed to verify networking settings: %s", err) } netOpts, netNewContainerOpts, err := netManager.ContainerNetworkingOpts(ctx, id) if err != nil { - return nil, nil, fmt.Errorf("failed to generate networking spec options: %s", err) + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), fmt.Errorf("failed to generate networking spec options: %s", err) } opts = append(opts, netOpts...) cOpts = append(cOpts, netNewContainerOpts...) netLabelOpts, err := netManager.InternalNetworkingOptionLabels(ctx) if err != nil { - return nil, nil, fmt.Errorf("failed to generate internal networking labels: %s", err) + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), fmt.Errorf("failed to generate internal networking labels: %s", err) } envs = append(envs, "HOSTNAME="+netLabelOpts.Hostname) @@ -230,37 +231,37 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa if runtime.GOOS != "windows" { hookOpt, err := withNerdctlOCIHook(options.NerdctlCmd, options.NerdctlArgs) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } opts = append(opts, hookOpt) } uOpts, err := generateUserOpts(options.User) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } opts = append(opts, uOpts...) gOpts, err := generateGroupsOpts(options.GroupAdd) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } opts = append(opts, gOpts...) umaskOpts, err := generateUmaskOpts(options.Umask) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } opts = append(opts, umaskOpts...) rtCOpts, err := generateRuntimeCOpts(options.GOptions.CgroupManager, options.Runtime) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } cOpts = append(cOpts, rtCOpts...) lCOpts, err := withContainerLabels(options.Label, options.LabelFile, ensuredImage) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } cOpts = append(cOpts, lCOpts...) @@ -276,10 +277,10 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa if options.Name != "" { containerNameStore, err = namestore.New(dataStore, options.GOptions.Namespace) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } if err := containerNameStore.Acquire(options.Name, id); err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } } internalLabels.name = options.Name @@ -287,14 +288,14 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa internalLabels.extraHosts = strutil.DedupeStrSlice(netManager.NetworkOptions().AddHost) for i, host := range internalLabels.extraHosts { if _, err := dockercliopts.ValidateExtraHost(host); err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } parts := strings.SplitN(host, ":", 2) // If the IP Address is a string called "host-gateway", replace this value with the IP address stored // in the daemon level HostGateway IP config variable. if len(parts) == 2 && parts[1] == dockeropts.HostGatewayName { if options.GOptions.HostGatewayIP == "" { - return nil, nil, fmt.Errorf("unable to derive the IP value for host-gateway") + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), fmt.Errorf("unable to derive the IP value for host-gateway") } parts[1] = options.GOptions.HostGatewayIP internalLabels.extraHosts[i] = fmt.Sprintf(`%s:%s`, parts[0], parts[1]) @@ -304,7 +305,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa // TODO: abolish internal labels and only use annotations ilOpt, err := withInternalLabels(internalLabels) if err != nil { - return nil, nil, err + return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err } cOpts = append(cOpts, ilOpt) @@ -817,6 +818,29 @@ func generateLogConfig(dataStore string, id string, logDriver string, logOpt []s return logConfig, nil } +func generateRemoveStateDirFunc(ctx context.Context, id string, internalLabels internalLabels) func() { + return func() { + if rmErr := os.RemoveAll(internalLabels.stateDir); rmErr != nil { + log.G(ctx).WithError(rmErr).Warnf("failed to remove container %q state dir %q", id, internalLabels.stateDir) + } + } +} + +func generateRemoveOrphanedDirsFunc(ctx context.Context, id, dataStore string, internalLabels internalLabels) func() { + return func() { + if rmErr := os.RemoveAll(internalLabels.stateDir); rmErr != nil { + log.G(ctx).WithError(rmErr).Warnf("failed to remove container %q state dir %q", id, internalLabels.stateDir) + } + + hs, err := hostsstore.New(dataStore, internalLabels.namespace) + if err != nil { + log.G(ctx).WithError(err).Warnf("failed to instantiate hostsstore for %q", internalLabels.namespace) + } else if err = hs.Delete(id); err != nil { + log.G(ctx).WithError(err).Warnf("failed to remove an etchosts directory for container %q", id) + } + } +} + func generateGcFunc(ctx context.Context, container containerd.Container, ns, id, name, dataStore string, containerErr error, containerNameStore namestore.NameStore, netManager containerutil.NetworkOptionsManager, internalLabels internalLabels) func() { return func() { if containerErr == nil { diff --git a/pkg/cmd/container/remove.go b/pkg/cmd/container/remove.go index bc055945824..d425b718972 100644 --- a/pkg/cmd/container/remove.go +++ b/pkg/cmd/container/remove.go @@ -233,7 +233,7 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions hs, err := hostsstore.New(dataStore, containerNamespace) if err != nil { log.G(ctx).WithError(err).Warnf("failed to instantiate hostsstore for %q", containerNamespace) - } else if err = hs.DeallocHostsFile(id); err != nil { + } else if err = hs.Delete(id); err != nil { // De-allocate hosts file - soft failure log.G(ctx).WithError(err).Warnf("failed to remove hosts file for container %q", id) } diff --git a/pkg/dnsutil/hostsstore/hostsstore.go b/pkg/dnsutil/hostsstore/hostsstore.go index 7e32e561468..507f93ec708 100644 --- a/pkg/dnsutil/hostsstore/hostsstore.go +++ b/pkg/dnsutil/hostsstore/hostsstore.go @@ -89,7 +89,7 @@ type Store interface { Release(id string) error Update(id, newName string) error HostsPath(id string) (location string, err error) - DeallocHostsFile(id string) (err error) + Delete(id string) (err error) AllocHostsFile(id string, content []byte) (location string, err error) } @@ -185,14 +185,13 @@ func (x *hostsStore) AllocHostsFile(id string, content []byte) (location string, return x.safeStore.Location(id, hostsFile) } -func (x *hostsStore) DeallocHostsFile(id string) (err error) { - defer func() { - if err != nil { - err = errors.Join(ErrHostsStore, err) - } - }() +func (x *hostsStore) Delete(id string) (err error) { + err = x.safeStore.WithLock(func() error { return x.safeStore.Delete(id) }) + if err != nil { + err = errors.Join(ErrHostsStore, err) + } - return x.safeStore.WithLock(func() error { return x.safeStore.Delete(id, hostsFile) }) + return err } func (x *hostsStore) HostsPath(id string) (location string, err error) {