Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Cleaning up orphaned directories and files when containers creat… #3457

Merged
merged 1 commit into from
Sep 28, 2024
Merged

fix: Cleaning up orphaned directories and files when containers creat… #3457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions cmd/nerdctl/container/container_create_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to mark this test as DockerIncompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !
Added testutil.DockerIncompatible(t) to tests.

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)
}
70 changes: 47 additions & 23 deletions pkg/cmd/container/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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...)

Expand All @@ -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]

Expand All @@ -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...)
Expand All @@ -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)")
}
}

Expand All @@ -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...)

Expand All @@ -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)
Expand All @@ -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...)

Expand All @@ -276,25 +277,25 @@ 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
internalLabels.pidFile = options.PidFile
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])
Expand All @@ -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)

Expand Down Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: are we deleting multiple dirs, or only 1. If only 1, we should name it generateRemoveOrphanedDirFunc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it deletes one or more of the following directories, the function name is Dirs.

  • /var/lib/nerdctl/1935db59/containers/<namespace>/<container id>
  • /var/lib/nerdctl/1935db59/etchosts/<namespace>/<container id>

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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/container/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
15 changes: 7 additions & 8 deletions pkg/dnsutil/hostsstore/hostsstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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) {
Expand Down