Skip to content

Commit 2c2745e

Browse files
haytokhaytok
committed
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/<namespace>/<container id>` - `/var/lib/nerdctl/1935db59/etchosts/<namespace>/<container id>` 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: - containerd#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/<namespace>/<container id>` Note that tests of logic to fix bug in Issue containerd#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 <[email protected]>
1 parent b94c658 commit 2c2745e

File tree

4 files changed

+181
-32
lines changed

4 files changed

+181
-32
lines changed

‎cmd/nerdctl/container/container_create_linux_test.go

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,22 @@
1717
package container
1818

1919
import (
20+
"errors"
2021
"fmt"
22+
"os"
23+
"path/filepath"
2124
"strings"
2225
"testing"
2326

27+
"github.com/opencontainers/go-digest"
2428
"gotest.tools/v3/assert"
2529

30+
"github.com/containerd/containerd/v2/defaults"
31+
2632
"github.com/containerd/nerdctl/v2/pkg/testutil"
33+
"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
2734
"github.com/containerd/nerdctl/v2/pkg/testutil/nettestutil"
35+
"github.com/containerd/nerdctl/v2/pkg/testutil/test"
2836
)
2937

3038
func TestCreate(t *testing.T) {
@@ -174,3 +182,121 @@ func TestCreateWithTty(t *testing.T) {
174182
withTtyContainer := base.InspectContainer(withTtyContainerName)
175183
assert.Equal(base.T, 0, withTtyContainer.State.ExitCode)
176184
}
185+
186+
// TestIssue2993 tests https://github.com/containerd/nerdctl/issues/2993
187+
func TestIssue2993(t *testing.T) {
188+
testutil.DockerIncompatible(t)
189+
190+
nerdtest.Setup()
191+
192+
const (
193+
containersPathKey = "containersPath"
194+
etchostsPathKey = "etchostsPath"
195+
)
196+
197+
getAddrHash := func(addr string) string {
198+
const addrHashLen = 8
199+
200+
d := digest.SHA256.FromString(addr)
201+
h := d.Encoded()[0:addrHashLen]
202+
203+
return h
204+
}
205+
206+
testCase := &test.Group{
207+
{
208+
Description: "Issue #2993 - nerdctl no longer leaks containers and etchosts directories and files when container creation fails.",
209+
Require: nerdtest.Private,
210+
Setup: func(data test.Data, helpers test.Helpers) {
211+
helpers.Ensure("run", "--name", data.Identifier(), "-d", testutil.AlpineImage, "sleep", "infinity")
212+
213+
dataRoot := string(data.ReadConfig(nerdtest.DataRoot))
214+
h := getAddrHash(defaults.DefaultAddress)
215+
dataStore := filepath.Join(dataRoot, h)
216+
namespace := data.Identifier()
217+
218+
containersPath := filepath.Join(dataStore, "containers", namespace)
219+
containersDirs, err := os.ReadDir(containersPath)
220+
assert.NilError(t, err)
221+
assert.Equal(t, len(containersDirs), 1)
222+
223+
etchostsPath := filepath.Join(dataStore, "etchosts", namespace)
224+
etchostsDirs, err := os.ReadDir(etchostsPath)
225+
assert.NilError(t, err)
226+
assert.Equal(t, len(etchostsDirs), 1)
227+
228+
data.Set(containersPathKey, containersPath)
229+
data.Set(etchostsPathKey, etchostsPath)
230+
},
231+
Cleanup: func(data test.Data, helpers test.Helpers) {
232+
helpers.Anyhow("rm", "-f", data.Identifier())
233+
},
234+
Command: func(data test.Data, helpers test.Helpers) test.Command {
235+
return helpers.Command("run", "--name", data.Identifier(), "-d", testutil.AlpineImage, "sleep", "infinity")
236+
},
237+
Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
238+
return &test.Expected{
239+
ExitCode: 1,
240+
Errors: []error{errors.New("is already used by ID")},
241+
Output: func(stdout string, info string, t *testing.T) {
242+
containersDirs, err := os.ReadDir(data.Get(containersPathKey))
243+
assert.NilError(t, err)
244+
assert.Equal(t, len(containersDirs), 1)
245+
246+
etchostsDirs, err := os.ReadDir(data.Get(etchostsPathKey))
247+
assert.NilError(t, err)
248+
assert.Equal(t, len(etchostsDirs), 1)
249+
},
250+
}
251+
},
252+
},
253+
{
254+
Description: "Issue #2993 - nerdctl no longer leaks containers and etchosts directories and files when containers are removed.",
255+
Require: nerdtest.Private,
256+
Setup: func(data test.Data, helpers test.Helpers) {
257+
helpers.Ensure("run", "--name", data.Identifier(), "-d", testutil.AlpineImage, "sleep", "infinity")
258+
259+
dataRoot := string(data.ReadConfig(nerdtest.DataRoot))
260+
h := getAddrHash(defaults.DefaultAddress)
261+
dataStore := filepath.Join(dataRoot, h)
262+
namespace := data.Identifier()
263+
264+
containersPath := filepath.Join(dataStore, "containers", namespace)
265+
containersDirs, err := os.ReadDir(containersPath)
266+
assert.NilError(t, err)
267+
assert.Equal(t, len(containersDirs), 1)
268+
269+
etchostsPath := filepath.Join(dataStore, "etchosts", namespace)
270+
etchostsDirs, err := os.ReadDir(etchostsPath)
271+
assert.NilError(t, err)
272+
assert.Equal(t, len(etchostsDirs), 1)
273+
274+
data.Set(containersPathKey, containersPath)
275+
data.Set(etchostsPathKey, etchostsPath)
276+
},
277+
Cleanup: func(data test.Data, helpers test.Helpers) {
278+
helpers.Anyhow("rm", "-f", data.Identifier())
279+
},
280+
Command: func(data test.Data, helpers test.Helpers) test.Command {
281+
return helpers.Command("rm", "-f", data.Identifier())
282+
},
283+
Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
284+
return &test.Expected{
285+
ExitCode: 0,
286+
Errors: []error{},
287+
Output: func(stdout string, info string, t *testing.T) {
288+
containersDirs, err := os.ReadDir(data.Get(containersPathKey))
289+
assert.NilError(t, err)
290+
assert.Equal(t, len(containersDirs), 0)
291+
292+
etchostsDirs, err := os.ReadDir(data.Get(etchostsPathKey))
293+
assert.NilError(t, err)
294+
assert.Equal(t, len(etchostsDirs), 0)
295+
},
296+
}
297+
},
298+
},
299+
}
300+
301+
testCase.Run(t)
302+
}

‎pkg/cmd/container/create.go

Lines changed: 47 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import (
4646
"github.com/containerd/nerdctl/v2/pkg/cmd/image"
4747
"github.com/containerd/nerdctl/v2/pkg/cmd/volume"
4848
"github.com/containerd/nerdctl/v2/pkg/containerutil"
49+
"github.com/containerd/nerdctl/v2/pkg/dnsutil/hostsstore"
4950
"github.com/containerd/nerdctl/v2/pkg/flagutil"
5051
"github.com/containerd/nerdctl/v2/pkg/idgen"
5152
"github.com/containerd/nerdctl/v2/pkg/imgutil"
@@ -118,7 +119,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
118119

119120
platformOpts, err := setPlatformOptions(ctx, client, id, netManager.NetworkOptions().UTSNamespace, &internalLabels, options)
120121
if err != nil {
121-
return nil, nil, err
122+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
122123
}
123124
opts = append(opts, platformOpts...)
124125

@@ -130,7 +131,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
130131
}
131132
ocispecPlatforms, err := platformutil.NewOCISpecPlatformSlice(false, platformSS)
132133
if err != nil {
133-
return nil, nil, err
134+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
134135
}
135136
rawRef := args[0]
136137

@@ -141,13 +142,13 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
141142

142143
ensuredImage, err = image.EnsureImage(ctx, client, rawRef, options.ImagePullOpt)
143144
if err != nil {
144-
return nil, nil, err
145+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
145146
}
146147
}
147148

148149
rootfsOpts, rootfsCOpts, err := generateRootfsOpts(args, id, ensuredImage, options)
149150
if err != nil {
150-
return nil, nil, err
151+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
151152
}
152153
opts = append(opts, rootfsOpts...)
153154
cOpts = append(cOpts, rootfsCOpts...)
@@ -158,12 +159,12 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
158159

159160
envs, err := flagutil.MergeEnvFileAndOSEnv(options.EnvFile, options.Env)
160161
if err != nil {
161-
return nil, nil, err
162+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
162163
}
163164

164165
if options.Interactive {
165166
if options.Detach {
166-
return nil, nil, errors.New("currently flag -i and -d cannot be specified together (FIXME)")
167+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), errors.New("currently flag -i and -d cannot be specified together (FIXME)")
167168
}
168169
}
169170

@@ -174,7 +175,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
174175
var mountOpts []oci.SpecOpts
175176
mountOpts, internalLabels.anonVolumes, internalLabels.mountPoints, err = generateMountOpts(ctx, client, ensuredImage, volStore, options)
176177
if err != nil {
177-
return nil, nil, err
178+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
178179
}
179180
opts = append(opts, mountOpts...)
180181

@@ -186,30 +187,30 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
186187
// 3, nerdctl start/restart demo
187188
logConfig, err := generateLogConfig(dataStore, id, options.LogDriver, options.LogOpt, options.GOptions.Namespace)
188189
if err != nil {
189-
return nil, nil, err
190+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
190191
}
191192
internalLabels.logURI = logConfig.LogURI
192193

193194
restartOpts, err := generateRestartOpts(ctx, client, options.Restart, logConfig.LogURI, options.InRun)
194195
if err != nil {
195-
return nil, nil, err
196+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), err
196197
}
197198
cOpts = append(cOpts, restartOpts...)
198199

199200
if err = netManager.VerifyNetworkOptions(ctx); err != nil {
200-
return nil, nil, fmt.Errorf("failed to verify networking settings: %s", err)
201+
return nil, generateRemoveStateDirFunc(ctx, id, internalLabels), fmt.Errorf("failed to verify networking settings: %s", err)
201202
}
202203

203204
netOpts, netNewContainerOpts, err := netManager.ContainerNetworkingOpts(ctx, id)
204205
if err != nil {
205-
return nil, nil, fmt.Errorf("failed to generate networking spec options: %s", err)
206+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), fmt.Errorf("failed to generate networking spec options: %s", err)
206207
}
207208
opts = append(opts, netOpts...)
208209
cOpts = append(cOpts, netNewContainerOpts...)
209210

210211
netLabelOpts, err := netManager.InternalNetworkingOptionLabels(ctx)
211212
if err != nil {
212-
return nil, nil, fmt.Errorf("failed to generate internal networking labels: %s", err)
213+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), fmt.Errorf("failed to generate internal networking labels: %s", err)
213214
}
214215

215216
envs = append(envs, "HOSTNAME="+netLabelOpts.Hostname)
@@ -230,37 +231,37 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
230231
if runtime.GOOS != "windows" {
231232
hookOpt, err := withNerdctlOCIHook(options.NerdctlCmd, options.NerdctlArgs)
232233
if err != nil {
233-
return nil, nil, err
234+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
234235
}
235236
opts = append(opts, hookOpt)
236237
}
237238

238239
uOpts, err := generateUserOpts(options.User)
239240
if err != nil {
240-
return nil, nil, err
241+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
241242
}
242243
opts = append(opts, uOpts...)
243244
gOpts, err := generateGroupsOpts(options.GroupAdd)
244245
if err != nil {
245-
return nil, nil, err
246+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
246247
}
247248
opts = append(opts, gOpts...)
248249

249250
umaskOpts, err := generateUmaskOpts(options.Umask)
250251
if err != nil {
251-
return nil, nil, err
252+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
252253
}
253254
opts = append(opts, umaskOpts...)
254255

255256
rtCOpts, err := generateRuntimeCOpts(options.GOptions.CgroupManager, options.Runtime)
256257
if err != nil {
257-
return nil, nil, err
258+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
258259
}
259260
cOpts = append(cOpts, rtCOpts...)
260261

261262
lCOpts, err := withContainerLabels(options.Label, options.LabelFile, ensuredImage)
262263
if err != nil {
263-
return nil, nil, err
264+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
264265
}
265266
cOpts = append(cOpts, lCOpts...)
266267

@@ -276,25 +277,25 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
276277
if options.Name != "" {
277278
containerNameStore, err = namestore.New(dataStore, options.GOptions.Namespace)
278279
if err != nil {
279-
return nil, nil, err
280+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
280281
}
281282
if err := containerNameStore.Acquire(options.Name, id); err != nil {
282-
return nil, nil, err
283+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
283284
}
284285
}
285286
internalLabels.name = options.Name
286287
internalLabels.pidFile = options.PidFile
287288
internalLabels.extraHosts = strutil.DedupeStrSlice(netManager.NetworkOptions().AddHost)
288289
for i, host := range internalLabels.extraHosts {
289290
if _, err := dockercliopts.ValidateExtraHost(host); err != nil {
290-
return nil, nil, err
291+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
291292
}
292293
parts := strings.SplitN(host, ":", 2)
293294
// If the IP Address is a string called "host-gateway", replace this value with the IP address stored
294295
// in the daemon level HostGateway IP config variable.
295296
if len(parts) == 2 && parts[1] == dockeropts.HostGatewayName {
296297
if options.GOptions.HostGatewayIP == "" {
297-
return nil, nil, fmt.Errorf("unable to derive the IP value for host-gateway")
298+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), fmt.Errorf("unable to derive the IP value for host-gateway")
298299
}
299300
parts[1] = options.GOptions.HostGatewayIP
300301
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
304305
// TODO: abolish internal labels and only use annotations
305306
ilOpt, err := withInternalLabels(internalLabels)
306307
if err != nil {
307-
return nil, nil, err
308+
return nil, generateRemoveOrphanedDirsFunc(ctx, id, dataStore, internalLabels), err
308309
}
309310
cOpts = append(cOpts, ilOpt)
310311

@@ -817,6 +818,29 @@ func generateLogConfig(dataStore string, id string, logDriver string, logOpt []s
817818
return logConfig, nil
818819
}
819820

821+
func generateRemoveStateDirFunc(ctx context.Context, id string, internalLabels internalLabels) func() {
822+
return func() {
823+
if rmErr := os.RemoveAll(internalLabels.stateDir); rmErr != nil {
824+
log.G(ctx).WithError(rmErr).Warnf("failed to remove container %q state dir %q", id, internalLabels.stateDir)
825+
}
826+
}
827+
}
828+
829+
func generateRemoveOrphanedDirsFunc(ctx context.Context, id, dataStore string, internalLabels internalLabels) func() {
830+
return func() {
831+
if rmErr := os.RemoveAll(internalLabels.stateDir); rmErr != nil {
832+
log.G(ctx).WithError(rmErr).Warnf("failed to remove container %q state dir %q", id, internalLabels.stateDir)
833+
}
834+
835+
hs, err := hostsstore.New(dataStore, internalLabels.namespace)
836+
if err != nil {
837+
log.G(ctx).WithError(err).Warnf("failed to instantiate hostsstore for %q", internalLabels.namespace)
838+
} else if err = hs.Delete(id); err != nil {
839+
log.G(ctx).WithError(err).Warnf("failed to remove an etchosts directory for container %q", id)
840+
}
841+
}
842+
}
843+
820844
func generateGcFunc(ctx context.Context, container containerd.Container, ns, id, name, dataStore string, containerErr error, containerNameStore namestore.NameStore, netManager containerutil.NetworkOptionsManager, internalLabels internalLabels) func() {
821845
return func() {
822846
if containerErr == nil {

‎pkg/cmd/container/remove.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions
233233
hs, err := hostsstore.New(dataStore, containerNamespace)
234234
if err != nil {
235235
log.G(ctx).WithError(err).Warnf("failed to instantiate hostsstore for %q", containerNamespace)
236-
} else if err = hs.DeallocHostsFile(id); err != nil {
236+
} else if err = hs.Delete(id); err != nil {
237237
// De-allocate hosts file - soft failure
238238
log.G(ctx).WithError(err).Warnf("failed to remove hosts file for container %q", id)
239239
}

‎pkg/dnsutil/hostsstore/hostsstore.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ type Store interface {
8989
Release(id string) error
9090
Update(id, newName string) error
9191
HostsPath(id string) (location string, err error)
92-
DeallocHostsFile(id string) (err error)
92+
Delete(id string) (err error)
9393
AllocHostsFile(id string, content []byte) (location string, err error)
9494
}
9595

@@ -185,14 +185,13 @@ func (x *hostsStore) AllocHostsFile(id string, content []byte) (location string,
185185
return x.safeStore.Location(id, hostsFile)
186186
}
187187

188-
func (x *hostsStore) DeallocHostsFile(id string) (err error) {
189-
defer func() {
190-
if err != nil {
191-
err = errors.Join(ErrHostsStore, err)
192-
}
193-
}()
188+
func (x *hostsStore) Delete(id string) (err error) {
189+
err = x.safeStore.WithLock(func() error { return x.safeStore.Delete(id) })
190+
if err != nil {
191+
err = errors.Join(ErrHostsStore, err)
192+
}
194193

195-
return x.safeStore.WithLock(func() error { return x.safeStore.Delete(id, hostsFile) })
194+
return err
196195
}
197196

198197
func (x *hostsStore) HostsPath(id string) (location string, err error) {

0 commit comments

Comments
 (0)