Skip to content

Commit

Permalink
Merge pull request #3457 from haytok/fix-to-clean-up-orphaned-files
Browse files Browse the repository at this point in the history 
fix: Cleaning up orphaned directories and files when containers creat…
  • Loading branch information
@djdongjin
djdongjin authored Sep 28, 2024
2 parents cd4a62a + 2c2745e commit cdd9a79
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 32 deletions.
126 changes: 126 additions & 0 deletions cmd/nerdctl/container/container_create_linux_test.go
View file
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) {
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
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() {
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
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
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

0 comments on commit cdd9a79

Please sign in to comment.