From a42487c4140c407ac257b4f0d479f0c85a5f08b2 Mon Sep 17 00:00:00 2001 From: apostasie Date: Thu, 20 Jun 2024 16:37:58 -0700 Subject: [PATCH] Volume QA: inspect, prune, create and other volume fixes Signed-off-by: apostasie --- cmd/nerdctl/namespace.go | 4 +- cmd/nerdctl/system_prune_linux_test.go | 3 +- cmd/nerdctl/volume_create.go | 9 + cmd/nerdctl/volume_create_test.go | 142 +++++++-- cmd/nerdctl/volume_inspect.go | 2 +- cmd/nerdctl/volume_inspect_test.go | 256 +++++++++++++-- cmd/nerdctl/volume_list_test.go | 12 +- cmd/nerdctl/volume_namespace_test.go | 139 ++++++++ cmd/nerdctl/volume_prune_linux_test.go | 119 ++++++- cmd/nerdctl/volume_remove_linux_test.go | 73 ++++- pkg/cmd/container/create.go | 15 +- pkg/cmd/container/remove.go | 8 + pkg/cmd/container/run_mount.go | 10 +- pkg/cmd/volume/create.go | 4 - pkg/cmd/volume/inspect.go | 29 +- pkg/cmd/volume/prune.go | 28 +- pkg/cmd/volume/rm.go | 45 ++- pkg/composer/up_volume.go | 3 + pkg/lockutil/lockutil_unix.go | 30 +- pkg/lockutil/lockutil_windows.go | 21 ++ pkg/mountutil/mountutil.go | 15 +- .../mountutilmock/volumestore.mock.go | 26 +- pkg/mountutil/volumestore/volumestore.go | 297 +++++++++++------- 23 files changed, 1033 insertions(+), 257 deletions(-) create mode 100644 cmd/nerdctl/volume_namespace_test.go diff --git a/cmd/nerdctl/namespace.go b/cmd/nerdctl/namespace.go index b60e89ccd37..20d188f3f57 100644 --- a/cmd/nerdctl/namespace.go +++ b/cmd/nerdctl/namespace.go @@ -113,11 +113,11 @@ func namespaceLsAction(cmd *cobra.Command, args []string) error { } numImages = len(images) - volStore, err := volumestore.Path(dataStore, ns) + volStore, err := volumestore.New(dataStore, ns) if err != nil { log.L.Warn(err) } else { - volEnts, err := os.ReadDir(volStore) + volEnts, err := volStore.List(false) if err != nil { if !os.IsNotExist(err) { log.L.Warn(err) diff --git a/cmd/nerdctl/system_prune_linux_test.go b/cmd/nerdctl/system_prune_linux_test.go index c1e91658f31..ce297cfc31d 100644 --- a/cmd/nerdctl/system_prune_linux_test.go +++ b/cmd/nerdctl/system_prune_linux_test.go @@ -32,7 +32,8 @@ import ( func TestSystemPrune(t *testing.T) { testutil.RequiresBuild(t) - base := testutil.NewBase(t) + namespaceID := testutil.Identifier(t) + base := testutil.NewBaseWithNamespace(t, namespaceID) base.Cmd("container", "prune", "-f").AssertOK() base.Cmd("network", "prune", "-f").AssertOK() base.Cmd("volume", "prune", "-f").AssertOK() diff --git a/cmd/nerdctl/volume_create.go b/cmd/nerdctl/volume_create.go index 972d5fc0359..9c8bfda26f1 100644 --- a/cmd/nerdctl/volume_create.go +++ b/cmd/nerdctl/volume_create.go @@ -17,6 +17,9 @@ package main import ( + "fmt" + + "github.com/containerd/containerd/errdefs" "github.com/containerd/nerdctl/v2/pkg/api/types" "github.com/containerd/nerdctl/v2/pkg/cmd/volume" @@ -45,6 +48,12 @@ func processVolumeCreateOptions(cmd *cobra.Command) (types.VolumeCreateOptions, if err != nil { return types.VolumeCreateOptions{}, err } + for _, label := range labels { + if label == "" { + return types.VolumeCreateOptions{}, fmt.Errorf("labels cannot be empty (%w)", errdefs.ErrInvalidArgument) + } + } + return types.VolumeCreateOptions{ GOptions: globalOptions, Labels: labels, diff --git a/cmd/nerdctl/volume_create_test.go b/cmd/nerdctl/volume_create_test.go index f8f5997b501..40643ef723c 100644 --- a/cmd/nerdctl/volume_create_test.go +++ b/cmd/nerdctl/volume_create_test.go @@ -19,40 +19,132 @@ package main import ( "testing" + "github.com/containerd/containerd/errdefs" "github.com/containerd/nerdctl/v2/pkg/testutil" - "gotest.tools/v3/assert" + "gotest.tools/v3/icmd" ) -// Test TestVolumeCreate for creating volume with given name. func TestVolumeCreate(t *testing.T) { - base := testutil.NewBase(t) - testVolume := testutil.Identifier(t) + t.Parallel() - base.Cmd("volume", "create", testVolume).AssertOK() - defer base.Cmd("volume", "rm", "-f", testVolume).Run() + base := testutil.NewBase(t) - base.Cmd("volume", "list").AssertOutContains(testVolume) -} + malformed := errdefs.ErrInvalidArgument.Error() + exitCodeVariant := 1 + if base.Target == testutil.Docker { + malformed = "invalid" + exitCodeVariant = 125 + } -// Test TestVolumeCreateTooManyArgs for creating volume with too many args. -func TestVolumeCreateTooManyArgs(t *testing.T) { - base := testutil.NewBase(t) + testCases := []struct { + description string + command func(tID string) *testutil.Cmd + tearUp func(tID string) + tearDown func(tID string) + expected func(tID string) icmd.Expected + }{ + { + description: "arg missing", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "create") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + } + }, + }, + { + description: "invalid identifier", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "create", "∞") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: malformed, + } + }, + }, + { + description: "too many args", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "create", "too", "many") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: "at most 1 arg", + } + }, + }, + { + description: "success", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "create", tID) + }, + tearDown: func(tID string) { + base.Cmd("volume", "rm", "-f", tID).Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + Out: tID, + } + }, + }, + { + description: "success with labels", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "create", "--label", "foo1=baz1", "--label", "foo2=baz2", tID) + }, + tearDown: func(tID string) { + base.Cmd("volume", "rm", "-f", tID).Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + Out: tID, + } + }, + }, + { + description: "invalid labels", + command: func(tID string) *testutil.Cmd { + // See https://github.com/containerd/nerdctl/issues/3126 + return base.Cmd("volume", "create", "--label", "a", "--label", "", tID) + }, + tearDown: func(tID string) { + base.Cmd("volume", "rm", "-f", tID).Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: exitCodeVariant, + Err: malformed, + } + }, + }, + } - base.Cmd("volume", "create", "too", "many").AssertFail() -} + for _, test := range testCases { + currentTest := test + t.Run(currentTest.description, func(tt *testing.T) { + tt.Parallel() -// Test TestVolumeCreateWithLabels for creating volume with given labels. -func TestVolumeCreateWithLabels(t *testing.T) { - base := testutil.NewBase(t) - testVolume := testutil.Identifier(t) + tID := testutil.Identifier(tt) - base.Cmd("volume", "create", testVolume, "--label", "foo1=baz1", "--label", "foo2=baz2").AssertOK() - defer base.Cmd("volume", "rm", "-f", testVolume).Run() + if currentTest.tearDown != nil { + currentTest.tearDown(tID) + tt.Cleanup(func() { + currentTest.tearDown(tID) + }) + } + if currentTest.tearUp != nil { + currentTest.tearUp(tID) + } - inspect := base.InspectVolume(testVolume) - inspectNerdctlLabels := *inspect.Labels - expected := make(map[string]string, 2) - expected["foo1"] = "baz1" - expected["foo2"] = "baz2" - assert.DeepEqual(base.T, expected, inspectNerdctlLabels) + cmd := currentTest.command(tID) + cmd.Assert(currentTest.expected(tID)) + }) + } } diff --git a/cmd/nerdctl/volume_inspect.go b/cmd/nerdctl/volume_inspect.go index 0eee9c41231..302b047f734 100644 --- a/cmd/nerdctl/volume_inspect.go +++ b/cmd/nerdctl/volume_inspect.go @@ -66,7 +66,7 @@ func volumeInspectAction(cmd *cobra.Command, args []string) error { if err != nil { return err } - return volume.Inspect(args, options) + return volume.Inspect(cmd.Context(), args, options) } func volumeInspectShellComplete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { diff --git a/cmd/nerdctl/volume_inspect_test.go b/cmd/nerdctl/volume_inspect_test.go index 24fff144a77..26f3d52d75e 100644 --- a/cmd/nerdctl/volume_inspect_test.go +++ b/cmd/nerdctl/volume_inspect_test.go @@ -18,48 +18,246 @@ package main import ( "crypto/rand" + "encoding/json" + "fmt" "os" "path/filepath" "testing" + "github.com/containerd/containerd/errdefs" + "github.com/containerd/nerdctl/v2/pkg/inspecttypes/native" "github.com/containerd/nerdctl/v2/pkg/testutil" "gotest.tools/v3/assert" + "gotest.tools/v3/icmd" ) -func TestVolumeInspectContainsLabels(t *testing.T) { - t.Parallel() - testVolume := testutil.Identifier(t) - - base := testutil.NewBase(t) - base.Cmd("volume", "create", "--label", "tag=testVolume", testVolume).AssertOK() - defer base.Cmd("volume", "rm", "-f", testVolume).Run() - - inspect := base.InspectVolume(testVolume) - inspectNerdctlLabels := (*inspect.Labels) - expected := make(map[string]string, 1) - expected["tag"] = "testVolume" - assert.DeepEqual(base.T, expected, inspectNerdctlLabels) +func createFileWithSize(base *testutil.Base, vol string, size int64) { + v := base.InspectVolume(vol) + token := make([]byte, size) + _, _ = rand.Read(token) + err := os.WriteFile(filepath.Join(v.Mountpoint, "test-file"), token, 0644) + assert.NilError(base.T, err) } -func TestVolumeInspectSize(t *testing.T) { - testutil.DockerIncompatible(t) +func TestVolumeInspect(t *testing.T) { t.Parallel() - testVolume := testutil.Identifier(t) + base := testutil.NewBase(t) - base.Cmd("volume", "create", testVolume).AssertOK() - defer base.Cmd("volume", "rm", "-f", testVolume).Run() + tID := testutil.Identifier(t) var size int64 = 1028 - createFileWithSize(t, testVolume, size) - volumeWithSize := base.InspectVolume(testVolume, []string{"--size"}...) - assert.Equal(t, volumeWithSize.Size, size) -} -func createFileWithSize(t *testing.T, volume string, bytes int64) { - base := testutil.NewBase(t) - v := base.InspectVolume(volume) - token := make([]byte, bytes) - rand.Read(token) - err := os.WriteFile(filepath.Join(v.Mountpoint, "test-file"), token, 0644) - assert.NilError(t, err) + malformed := errdefs.ErrInvalidArgument.Error() + notFound := errdefs.ErrNotFound.Error() + requireArg := "requires at least 1 arg" + if base.Target == testutil.Docker { + malformed = "no such volume" + notFound = "no such volume" + } + + tearUp := func(t *testing.T) { + base.Cmd("volume", "create", tID).AssertOK() + base.Cmd("volume", "create", "--label", "foo=fooval", "--label", "bar=barval", tID+"-second").AssertOK() + + // Obviously note here that if inspect code gets totally hosed, this entire suite will + // probably fail right here on the tearUp instead of actually testing something + createFileWithSize(base, tID, size) + } + + tearDown := func(t *testing.T) { + base.Cmd("volume", "rm", "-f", tID).Run() + base.Cmd("volume", "rm", "-f", tID+"-second").Run() + } + + tearDown(t) + t.Cleanup(func() { + tearDown(t) + }) + tearUp(t) + + testCases := []struct { + description string + command func(tID string) *testutil.Cmd + tearUp func(tID string) + tearDown func(tID string) + expected func(tID string) icmd.Expected + inspect func(t *testing.T, stdout string) + dockerIncompatible bool + }{ + { + description: "arg missing", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: requireArg, + } + }, + }, + { + description: "invalid identifier", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", "∞") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: malformed, + } + }, + }, + { + description: "non existent volume", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", "doesnotexist") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: notFound, + } + }, + }, + { + description: "success", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", tID) + }, + tearDown: func(tID string) { + base.Cmd("volume", "rm", "-f", tID) + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + Out: tID, + } + }, + inspect: func(t *testing.T, stdout string) { + var dc []native.Volume + if err := json.Unmarshal([]byte(stdout), &dc); err != nil { + t.Fatal(err) + } + assert.Assert(t, len(dc) == 1, fmt.Sprintf("one result, not %d", len(dc))) + assert.Assert(t, dc[0].Name == tID, fmt.Sprintf("expected name to be %q (was %q)", tID, dc[0].Name)) + assert.Assert(t, dc[0].Labels == nil, fmt.Sprintf("expected labels to be nil and were %v", dc[0].Labels)) + }, + }, + { + description: "multi success", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", tID, tID+"-second") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + } + }, + inspect: func(t *testing.T, stdout string) { + var dc []native.Volume + if err := json.Unmarshal([]byte(stdout), &dc); err != nil { + t.Fatal(err) + } + assert.Assert(t, len(dc) == 2, fmt.Sprintf("two results, not %d", len(dc))) + assert.Assert(t, dc[0].Name == tID, fmt.Sprintf("expected name to be %q (was %q)", tID, dc[0].Name)) + assert.Assert(t, dc[1].Name == tID+"-second", fmt.Sprintf("expected name to be %q (was %q)", tID+"-second", dc[1].Name)) + }, + }, + { + description: "inspect labels", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", tID+"-second") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + Out: tID, + } + }, + inspect: func(t *testing.T, stdout string) { + var dc []native.Volume + if err := json.Unmarshal([]byte(stdout), &dc); err != nil { + t.Fatal(err) + } + + labels := *dc[0].Labels + assert.Assert(t, len(labels) == 2, fmt.Sprintf("two results, not %d", len(labels))) + assert.Assert(t, labels["foo"] == "fooval", fmt.Sprintf("label foo should be fooval, not %s", labels["foo"])) + assert.Assert(t, labels["bar"] == "barval", fmt.Sprintf("label bar should be barval, not %s", labels["bar"])) + }, + }, + { + description: "inspect size", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", "--size", tID) + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + Out: tID, + } + }, + inspect: func(t *testing.T, stdout string) { + var dc []native.Volume + if err := json.Unmarshal([]byte(stdout), &dc); err != nil { + t.Fatal(err) + } + assert.Assert(t, dc[0].Size == size, fmt.Sprintf("expected size to be %d (was %d)", size, dc[0].Size)) + }, + dockerIncompatible: true, + }, + { + description: "part success multi", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "inspect", "invalid∞", "nonexistent", tID) + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Out: tID, + Err: notFound, + } + }, + inspect: func(t *testing.T, stdout string) { + var dc []native.Volume + if err := json.Unmarshal([]byte(stdout), &dc); err != nil { + t.Fatal(err) + } + assert.Assert(t, len(dc) == 1, fmt.Sprintf("one result, not %d", len(dc))) + assert.Assert(t, dc[0].Name == tID, fmt.Sprintf("expected name to be %q (was %q)", tID, dc[0].Name)) + }, + }, + } + + for _, test := range testCases { + currentTest := test + t.Run(currentTest.description, func(tt *testing.T) { + if currentTest.dockerIncompatible { + testutil.DockerIncompatible(tt) + } + + tt.Parallel() + + // We use the main test tID here + if currentTest.tearDown != nil { + currentTest.tearDown(tID) + tt.Cleanup(func() { + currentTest.tearDown(tID) + }) + } + if currentTest.tearUp != nil { + currentTest.tearUp(tID) + } + + // See https://github.com/containerd/nerdctl/issues/3130 + // We run first to capture the underlying icmd command and output + cmd := currentTest.command(tID) + res := cmd.Run() + cmd.Assert(currentTest.expected(tID)) + if currentTest.inspect != nil { + currentTest.inspect(tt, res.Stdout()) + } + }) + } } diff --git a/cmd/nerdctl/volume_list_test.go b/cmd/nerdctl/volume_list_test.go index 9b88e13eac9..94a8522407a 100644 --- a/cmd/nerdctl/volume_list_test.go +++ b/cmd/nerdctl/volume_list_test.go @@ -42,8 +42,8 @@ func TestVolumeLs(t *testing.T) { base.Cmd("volume", "create", vol3).AssertOK() defer base.Cmd("volume", "rm", "-f", vol3).Run() - createFileWithSize(t, vol1, 102400) - createFileWithSize(t, vol2, 204800) + createFileWithSize(base, vol1, 102400) + createFileWithSize(base, vol2, 204800) base.Cmd("volume", "ls", "--size").AssertOutWithFunc(func(stdout string) error { var lines = strings.Split(strings.TrimSpace(stdout), "\n") @@ -279,10 +279,10 @@ func TestVolumeLsFilterSize(t *testing.T) { base.Cmd("volume", "create", vol4).AssertOK() defer base.Cmd("volume", "rm", "-f", vol4).Run() - createFileWithSize(t, vol1, 409600) - createFileWithSize(t, vol2, 1024000) - createFileWithSize(t, vol3, 409600) - createFileWithSize(t, vol4, 1024000) + createFileWithSize(base, vol1, 409600) + createFileWithSize(base, vol2, 1024000) + createFileWithSize(base, vol3, 409600) + createFileWithSize(base, vol4, 1024000) base.Cmd("volume", "ls", "--size", "--filter", "size=1024000").AssertOutWithFunc(func(stdout string) error { var lines = strings.Split(strings.TrimSpace(stdout), "\n") diff --git a/cmd/nerdctl/volume_namespace_test.go b/cmd/nerdctl/volume_namespace_test.go new file mode 100644 index 00000000000..71a94ed590f --- /dev/null +++ b/cmd/nerdctl/volume_namespace_test.go @@ -0,0 +1,139 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package main + +import ( + "fmt" + "testing" + + "github.com/containerd/containerd/errdefs" + "github.com/containerd/nerdctl/v2/pkg/testutil" + "gotest.tools/v3/icmd" +) + +func TestVolumeNamespace(t *testing.T) { + testutil.DockerIncompatible(t) + + t.Parallel() + + base := testutil.NewBase(t) + tID := testutil.Identifier(t) + otherBase := testutil.NewBaseWithNamespace(t, tID+"-1") + thirdBase := testutil.NewBaseWithNamespace(t, tID+"-2") + + tearUp := func(t *testing.T) { + base.Cmd("volume", "create", tID).AssertOK() + } + + tearDown := func(t *testing.T) { + base.Cmd("volume", "rm", "-f", tID).Run() + otherBase.Cmd("namespace", "rm", "-f", tID+"-1").Run() + thirdBase.Cmd("namespace", "rm", "-f", tID+"-2").Run() + } + + tearDown(t) + t.Cleanup(func() { + tearDown(t) + }) + tearUp(t) + + testCases := []struct { + description string + command func(tID string) *testutil.Cmd + tearUp func(tID string) + tearDown func(tID string) + expected func(tID string) icmd.Expected + }{ + { + description: "inspect", + command: func(tID string) *testutil.Cmd { + return otherBase.Cmd("volume", "inspect", tID) + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: fmt.Errorf("%s (%w)", tID, errdefs.ErrNotFound).Error(), + } + }, + }, + { + description: "remove", + command: func(tID string) *testutil.Cmd { + return otherBase.Cmd("volume", "remove", tID) + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: fmt.Errorf("%s (%w)", tID, errdefs.ErrNotFound).Error(), + } + }, + }, + { + description: "prune", + command: func(tID string) *testutil.Cmd { + return otherBase.Cmd("volume", "prune", "-a", "-f") + }, + tearDown: func(tID string) { + // Assert that the volume is here in the base namespace + // both before and after the prune command + base.Cmd("volume", "inspect", tID).AssertOK() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + } + }, + }, + { + description: "create", + command: func(tID string) *testutil.Cmd { + return thirdBase.Cmd("volume", "create", tID) + }, + tearDown: func(tID string) { + thirdBase.Cmd("volume", "remove", "-f", tID).Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + Out: tID, + } + }, + }, + } + + for _, test := range testCases { + currentTest := test + t.Run(currentTest.description, func(tt *testing.T) { + tt.Parallel() + + // Note that here we are using the main test tID + // since we are testing against the volume created in it + if currentTest.tearDown != nil { + currentTest.tearDown(tID) + tt.Cleanup(func() { + currentTest.tearDown(tID) + }) + } + if currentTest.tearUp != nil { + currentTest.tearUp(tID) + } + + cmd := currentTest.command(tID) + cmd.Assert(currentTest.expected(tID)) + }) + } +} diff --git a/cmd/nerdctl/volume_prune_linux_test.go b/cmd/nerdctl/volume_prune_linux_test.go index df610ca1a04..da6f833ac7a 100644 --- a/cmd/nerdctl/volume_prune_linux_test.go +++ b/cmd/nerdctl/volume_prune_linux_test.go @@ -17,30 +17,117 @@ package main import ( - "fmt" + "strings" "testing" + "gotest.tools/v3/assert" + "gotest.tools/v3/icmd" + "github.com/containerd/nerdctl/v2/pkg/testutil" ) +// Volume pruning cannot be parallelized for Docker, since we need namespaces to do that in a way that does interact with other tests func TestVolumePrune(t *testing.T) { - base := testutil.NewBase(t) - tID := testutil.Identifier(t) - base.Cmd("volume", "prune", "-a", "-f").Run() + if testutil.GetTarget() != testutil.Docker { + t.Parallel() + } + + subTearUp := func(tID string) { + base := testutil.NewBaseWithNamespace(t, tID) + res := base.Cmd("volume", "create").Run() + anonIDBusy := res.Stdout() + base.Cmd("volume", "create").Run() + base.Cmd("volume", "create", tID+"-busy").AssertOK() + base.Cmd("volume", "create", tID+"-free").AssertOK() + + base.Cmd("run", "--name", tID, "-v", tID+"-busy:/whatever", "-v", anonIDBusy, testutil.CommonImage).AssertOK() + } + + subTearDown := func(tID string) { + base := testutil.NewBaseWithNamespace(t, tID) + base.Cmd("rm", "-f", tID).Run() + //base.Cmd("volume", "rm", "-f", tID+"-busy").Run() + // base.Cmd("volume", "rm", "-f", tID+"-free").Run() + // base.Cmd("volume", "rm", "-f", anonIDFree).Run() + // base.Cmd("volume", "rm", "-f", anonIDBusy).Run() + base.Cmd("namespace", "remove", "-f", tID).Run() + } + + testCases := []struct { + description string + command func(tID string) *testutil.Cmd + tearUp func(tID string) + tearDown func(tID string) + expected func(tID string) icmd.Expected + inspect func(tID string, stdout string) + }{ + { + description: "prune ano", + command: func(tID string) *testutil.Cmd { + base := testutil.NewBaseWithNamespace(t, tID) + return base.Cmd("volume", "prune", "-f") + }, + tearUp: subTearUp, + tearDown: subTearDown, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + } + }, + inspect: func(tID string, stdout string) { + base := testutil.NewBaseWithNamespace(t, tID) + assert.Assert(base.T, !strings.Contains(stdout, tID+"-free")) + base.Cmd("inspect", tID+"-free").AssertOK() + assert.Assert(base.T, !strings.Contains(stdout, tID+"-busy")) + base.Cmd("inspect", tID+"-busy").AssertOK() + // TODO verify the anonymous volumes status + }, + }, + { + description: "prune all", + command: func(tID string) *testutil.Cmd { + base := testutil.NewBaseWithNamespace(t, tID) + return base.Cmd("volume", "prune", "-f", "--all") + }, + tearUp: subTearUp, + tearDown: subTearDown, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 0, + } + }, + inspect: func(tID string, stdout string) { + base := testutil.NewBaseWithNamespace(t, tID) + assert.Assert(t, !strings.Contains(stdout, tID+"-busy")) + base.Cmd("inspect", tID+"-busy").AssertOK() + assert.Assert(t, strings.Contains(stdout, tID+"-free")) + base.Cmd("inspect", tID+"-free").AssertFail() + // TODO verify the anonymous volumes status + }, + }, + } - vID := base.Cmd("volume", "create").Out() - base.Cmd("volume", "create", tID+"-1").AssertOK() - base.Cmd("volume", "create", tID+"-2").AssertOK() + for _, test := range testCases { + currentTest := test + t.Run(currentTest.description, func(tt *testing.T) { + if testutil.GetTarget() != testutil.Docker { + tt.Parallel() + } - base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID+"-1"), "--name", tID, testutil.CommonImage).AssertOK() - defer base.Cmd("rm", "-f", tID).Run() + subTID := testutil.Identifier(tt) - base.Cmd("volume", "prune", "-f").AssertOutContains(vID) - base.Cmd("volume", "prune", "-a", "-f").AssertOutContains(tID + "-2") - base.Cmd("volume", "ls").AssertOutContains(tID + "-1") - base.Cmd("volume", "ls").AssertOutNotContains(tID + "-2") + if currentTest.tearDown != nil { + currentTest.tearDown(subTID) + tt.Cleanup(func() { + currentTest.tearDown(subTID) + }) + } + if currentTest.tearUp != nil { + currentTest.tearUp(subTID) + } - base.Cmd("rm", "-f", tID).AssertOK() - base.Cmd("volume", "prune", "-a", "-f").AssertOK() - base.Cmd("volume", "ls").AssertOutNotContains(tID + "-1") + cmd := currentTest.command(subTID) + cmd.Assert(currentTest.expected(subTID)) + }) + } } diff --git a/cmd/nerdctl/volume_remove_linux_test.go b/cmd/nerdctl/volume_remove_linux_test.go index 6023381c782..57deb87e8b2 100644 --- a/cmd/nerdctl/volume_remove_linux_test.go +++ b/cmd/nerdctl/volume_remove_linux_test.go @@ -20,21 +20,29 @@ import ( "fmt" "testing" + "github.com/containerd/containerd/errdefs" "github.com/containerd/nerdctl/v2/pkg/testutil" + "gotest.tools/v3/assert" "gotest.tools/v3/icmd" ) +// TestVolumeRemove does test a large variety of volume remove situations, albeit none of them being +// hard filesystem errors. +// Behavior in such cases is largely unspecified, as there is no easy way to compare with Docker. +// Anyhow, borked filesystem conditions is not something we should be expected to deal with in a smart way. func TestVolumeRemove(t *testing.T) { t.Parallel() base := testutil.NewBase(t) - malformed := "malformed volume name" - notFound := "no such volume" + inUse := errdefs.ErrFailedPrecondition.Error() + malformed := errdefs.ErrInvalidArgument.Error() + notFound := errdefs.ErrNotFound.Error() requireArg := "requires at least 1 arg" - inUse := "is in use" if base.Target == testutil.Docker { malformed = "no such volume" + notFound = "no such volume" + inUse = "volume is in use" } testCases := []struct { @@ -101,6 +109,39 @@ func TestVolumeRemove(t *testing.T) { }, }, + { + description: "busy anonymous volume", + command: func(tID string) *testutil.Cmd { + // Inspect the container and find the anonymous volume id + inspect := base.InspectContainer(tID) + var anonName string + for _, v := range inspect.Mounts { + if v.Destination == "/volume" { + anonName = v.Name + break + } + } + assert.Assert(t, anonName != "", "Failed to find anonymous volume id") + + // Try to remove that anon volume + return base.Cmd("volume", "rm", anonName) + }, + tearUp: func(tID string) { + // base.Cmd("volume", "create", tID).AssertOK() + base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID), "--name", tID, testutil.CommonImage).AssertOK() + }, + tearDown: func(tID string) { + base.Cmd("rm", "-f", tID).Run() + base.Cmd("volume", "rm", "-f", tID).Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: inUse, + } + + }, + }, { description: "freed volume", command: func(tID string) *testutil.Cmd { @@ -139,17 +180,21 @@ func TestVolumeRemove(t *testing.T) { }, }, { - "part success multi remove", - func(tID string) *testutil.Cmd { - return base.Cmd("volume", "rm", "invalid∞", "nonexistent", tID) + description: "part success multi-remove", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "rm", "invalid∞", "nonexistent", tID+"-busy", tID) }, - func(tID string) { + tearUp: func(tID string) { base.Cmd("volume", "create", tID).AssertOK() + base.Cmd("volume", "create", tID+"-busy").AssertOK() + base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID+"-busy"), "--name", tID, testutil.CommonImage).AssertOK() }, - func(tID string) { + tearDown: func(tID string) { + base.Cmd("rm", "-f", tID).Run() base.Cmd("volume", "rm", "-f", tID).Run() + base.Cmd("volume", "rm", "-f", tID+"-busy").Run() }, - func(tID string) icmd.Expected { + expected: func(tID string) icmd.Expected { return icmd.Expected{ ExitCode: 1, Out: tID, @@ -177,8 +222,16 @@ func TestVolumeRemove(t *testing.T) { }, { description: "failing multi-remove", + tearUp: func(tID string) { + base.Cmd("volume", "create", tID+"-busy").AssertOK() + base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID+"-busy"), "--name", tID, testutil.CommonImage).AssertOK() + }, + tearDown: func(tID string) { + base.Cmd("rm", "-f", tID).Run() + base.Cmd("volume", "rm", "-f", tID+"-busy").Run() + }, command: func(tID string) *testutil.Cmd { - return base.Cmd("volume", "rm", "nonexist1", "nonexist2") + return base.Cmd("volume", "rm", "invalid∞", "nonexistent", tID+"-busy") }, expected: func(tID string) icmd.Expected { return icmd.Expected{ diff --git a/pkg/cmd/container/create.go b/pkg/cmd/container/create.go index 2b347d890e1..9979f673565 100644 --- a/pkg/cmd/container/create.go +++ b/pkg/cmd/container/create.go @@ -39,6 +39,7 @@ import ( "github.com/containerd/nerdctl/v2/pkg/api/types" "github.com/containerd/nerdctl/v2/pkg/clientutil" "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/flagutil" "github.com/containerd/nerdctl/v2/pkg/idgen" @@ -61,6 +62,17 @@ import ( // Create will create a container. func Create(ctx context.Context, client *containerd.Client, args []string, netManager containerutil.NetworkOptionsManager, options types.ContainerCreateOptions) (containerd.Container, func(), error) { + // Acquire an exclusive lock on the volume store until we are done to avoid being raced by other volume operations + volStore, err := volume.Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) + if err != nil { + return nil, nil, err + } + err = volStore.Lock() + if err != nil { + return nil, nil, err + } + defer volStore.Unlock() + // simulate the behavior of double dash newArg := []string{} if len(args) >= 2 && args[1] == "--" { @@ -83,6 +95,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa return nil, nil, err } } + dataStore, err := clientutil.DataStore(options.GOptions.DataRoot, options.GOptions.Address) if err != nil { return nil, nil, err @@ -151,7 +164,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, options) + mountOpts, internalLabels.anonVolumes, internalLabels.mountPoints, err = generateMountOpts(ctx, client, ensuredImage, volStore, options) if err != nil { return nil, nil, err } diff --git a/pkg/cmd/container/remove.go b/pkg/cmd/container/remove.go index 56cdcc3ebea..4dca4d117a2 100644 --- a/pkg/cmd/container/remove.go +++ b/pkg/cmd/container/remove.go @@ -132,6 +132,14 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions if err != nil { return err } + // Note: technically, it is not strictly necessary to acquire an exclusive lock on the volume store here. + // Worst case scenario, we would fail removing anonymous volumes later on, which is a soft error, and which would + // only happen if we concurrently tried to remove the same container. + err = volStore.Lock() + if err != nil { + return err + } + defer volStore.Unlock() // Decode IPC ipc, err := ipcutil.DecodeIPCLabel(containerLabels[labels.IPC]) if err != nil { diff --git a/pkg/cmd/container/run_mount.go b/pkg/cmd/container/run_mount.go index 3a53c8c6f70..87c36f45142 100644 --- a/pkg/cmd/container/run_mount.go +++ b/pkg/cmd/container/run_mount.go @@ -37,7 +37,6 @@ import ( "github.com/containerd/continuity/fs" "github.com/containerd/log" "github.com/containerd/nerdctl/v2/pkg/api/types" - "github.com/containerd/nerdctl/v2/pkg/cmd/volume" "github.com/containerd/nerdctl/v2/pkg/idgen" "github.com/containerd/nerdctl/v2/pkg/imgutil" "github.com/containerd/nerdctl/v2/pkg/inspecttypes/dockercompat" @@ -122,13 +121,8 @@ func parseMountFlags(volStore volumestore.VolumeStore, options types.ContainerCr // generateMountOpts generates volume-related mount opts. // Other mounts such as procfs mount are not handled here. -func generateMountOpts(ctx context.Context, client *containerd.Client, ensuredImage *imgutil.EnsuredImage, options types.ContainerCreateOptions) ([]oci.SpecOpts, []string, []*mountutil.Processed, error) { - // volume store is corresponds to a directory like `/var/lib/nerdctl/1935db59/volumes/default` - volStore, err := volume.Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) - if err != nil { - return nil, nil, nil, err - } - +func generateMountOpts(ctx context.Context, client *containerd.Client, ensuredImage *imgutil.EnsuredImage, + volStore volumestore.VolumeStore, options types.ContainerCreateOptions) ([]oci.SpecOpts, []string, []*mountutil.Processed, error) { //nolint:golint,prealloc var ( opts []oci.SpecOpts diff --git a/pkg/cmd/volume/create.go b/pkg/cmd/volume/create.go index c00fe00efa3..f8cdbbcd345 100644 --- a/pkg/cmd/volume/create.go +++ b/pkg/cmd/volume/create.go @@ -19,7 +19,6 @@ package volume import ( "fmt" - "github.com/containerd/containerd/identifiers" "github.com/containerd/nerdctl/v2/pkg/api/types" "github.com/containerd/nerdctl/v2/pkg/inspecttypes/native" "github.com/containerd/nerdctl/v2/pkg/labels" @@ -32,9 +31,6 @@ func Create(name string, options types.VolumeCreateOptions) (*native.Volume, err name = stringid.GenerateRandomID() options.Labels = append(options.Labels, labels.AnonymousVolumes+"=") } - if err := identifiers.Validate(name); err != nil { - return nil, fmt.Errorf("malformed name %s: %w", name, err) - } volStore, err := Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) if err != nil { return nil, err diff --git a/pkg/cmd/volume/inspect.go b/pkg/cmd/volume/inspect.go index 4c9442ed286..4a7ee1cd2e5 100644 --- a/pkg/cmd/volume/inspect.go +++ b/pkg/cmd/volume/inspect.go @@ -17,23 +17,40 @@ package volume import ( + "context" + "errors" + + "github.com/containerd/log" "github.com/containerd/nerdctl/v2/pkg/api/types" "github.com/containerd/nerdctl/v2/pkg/formatter" ) -func Inspect(volumes []string, options types.VolumeInspectOptions) error { +func Inspect(ctx context.Context, volumes []string, options types.VolumeInspectOptions) error { volStore, err := Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) if err != nil { return err } - result := make([]interface{}, len(volumes)) + result := []interface{}{} - for i, name := range volumes { + warns := []error{} + for _, name := range volumes { var vol, err = volStore.Get(name, options.Size) if err != nil { - return err + warns = append(warns, err) + continue } - result[i] = vol + result = append(result, vol) + } + err = formatter.FormatSlice(options.Format, options.Stdout, result) + if err != nil { + return err + } + for _, warn := range warns { + log.G(ctx).Warn(warn) + } + + if len(warns) != 0 { + return errors.New("some volumes could not be inspected") } - return formatter.FormatSlice(options.Format, options.Stdout, result) + return nil } diff --git a/pkg/cmd/volume/prune.go b/pkg/cmd/volume/prune.go index f9dff99f428..d2a2967ecee 100644 --- a/pkg/cmd/volume/prune.go +++ b/pkg/cmd/volume/prune.go @@ -26,37 +26,55 @@ import ( ) func Prune(ctx context.Context, client *containerd.Client, options types.VolumePruneOptions) error { + // Get the volume store and lock it until we are done. + // This will prevent racing new containers from being created or removed until we are done with the cleanup of volumes volStore, err := Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) if err != nil { return err } - volumes, err := volStore.List(false) + err = volStore.Lock() if err != nil { return err } + defer volStore.Unlock() + // Get containers and see which volumes are used containers, err := client.Containers(ctx) if err != nil { return err } + usedVolumes, err := usedVolumes(ctx, containers) if err != nil { return err } var removeNames []string // nolint: prealloc + + // Get the list + volumes, err := volStore.List(false) + if err != nil { + return err + } + + // Iterate through the known volumes, making sure we do not remove in-use volumes + // but capture as well anon volumes (if --all was passed) for _, volume := range volumes { if _, ok := usedVolumes[volume.Name]; ok { continue } if !options.All { - val, ok := (*volume.Labels)[labels.AnonymousVolumes] - //skip the named volume and only remove the anonymous volume - if !ok || val != "" { - continue + if volume.Labels != nil { + val, ok := (*volume.Labels)[labels.AnonymousVolumes] + //skip the named volume and only remove the anonymous volume + if !ok || val != "" { + continue + } } } removeNames = append(removeNames, volume.Name) } + + // Remove the volumes from that list removedNames, _, err := volStore.Remove(removeNames) if err != nil { return err diff --git a/pkg/cmd/volume/rm.go b/pkg/cmd/volume/rm.go index 509957bb978..89dee6bb79c 100644 --- a/pkg/cmd/volume/rm.go +++ b/pkg/cmd/volume/rm.go @@ -21,8 +21,8 @@ import ( "encoding/json" "errors" "fmt" - "github.com/containerd/containerd" + "github.com/containerd/containerd/errdefs" "github.com/containerd/log" "github.com/containerd/nerdctl/v2/pkg/api/types" "github.com/containerd/nerdctl/v2/pkg/inspecttypes/dockercompat" @@ -31,50 +31,67 @@ import ( ) func Remove(ctx context.Context, client *containerd.Client, volumes []string, options types.VolumeRemoveOptions) error { - containers, err := client.Containers(ctx) + // Get the volume store and lock it until we are done. + // This will prevent racing new containers from being created or removed until we are done with the cleanup of volumes + volStore, err := Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) if err != nil { return err } - volStore, err := Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address) + err = volStore.Lock() + if err != nil { + return err + } + defer volStore.Unlock() + + containers, err := client.Containers(ctx) if err != nil { return err } - usedVolumes, err := usedVolumes(ctx, containers) + + usedVolumesList, err := usedVolumes(ctx, containers) if err != nil { return err } - var volumenames []string // nolint: prealloc + volumeNames := []string{} + cannotRemove := []error{} for _, name := range volumes { - if _, ok := usedVolumes[name]; ok { - return fmt.Errorf("volume %q is in use", name) + if _, ok := usedVolumesList[name]; ok { + cannotRemove = append(cannotRemove, fmt.Errorf("volume %q is in use (%w)", name, errdefs.ErrFailedPrecondition)) + continue } - volumenames = append(volumenames, name) + volumeNames = append(volumeNames, name) } // if err is set, this is a hard filesystem error - removedNames, warns, err := volStore.Remove(volumenames) + removedNames, warns, err := volStore.Remove(volumeNames) if err != nil { return err } + cannotRemove = append(cannotRemove, warns...) // Otherwise, output on stdout whatever was successful for _, name := range removedNames { fmt.Fprintln(options.Stdout, name) } // Log the rest - for _, volErr := range warns { + for _, volErr := range cannotRemove { log.G(ctx).Warn(volErr) } - if len(warns) > 0 { + if len(cannotRemove) > 0 { return errors.New("some volumes could not be removed") } return nil } func usedVolumes(ctx context.Context, containers []containerd.Container) (map[string]struct{}, error) { - usedVolumes := make(map[string]struct{}) + usedVolumesList := make(map[string]struct{}) for _, c := range containers { l, err := c.Labels(ctx) if err != nil { + // Containerd note: there is no guarantee that the containers we got from the list still exist at this point + // If that is the case, just ignore and move on + if errors.Is(err, errdefs.ErrNotFound) { + continue + } return nil, err } mountsJSON, ok := l[labels.Mounts] @@ -89,9 +106,9 @@ func usedVolumes(ctx context.Context, containers []containerd.Container) (map[st } for _, m := range mounts { if m.Type == mountutil.Volume { - usedVolumes[m.Name] = struct{}{} + usedVolumesList[m.Name] = struct{}{} } } } - return usedVolumes, nil + return usedVolumesList, nil } diff --git a/pkg/composer/up_volume.go b/pkg/composer/up_volume.go index 436c85c51c5..190001956cf 100644 --- a/pkg/composer/up_volume.go +++ b/pkg/composer/up_volume.go @@ -41,6 +41,9 @@ func (c *Composer) upVolume(ctx context.Context, shortName string) error { // shortName is like "db_data", fullName is like "compose-wordpress_db_data" fullName := vol.Name + // FIXME: this is racy. By the time we get below to creating the volume, there is no guarantee that things are still fine + // Furthermore, volStore.Get no longer errors if the volume already exists (docker behavior), so, the purpose of this + // call needs to be assessed (it might still error if the name is malformed, or if there is a filesystem error) volExists, err := c.VolumeExists(fullName) if err != nil { return err diff --git a/pkg/lockutil/lockutil_unix.go b/pkg/lockutil/lockutil_unix.go index 64e9867b044..9b3d9d98932 100644 --- a/pkg/lockutil/lockutil_unix.go +++ b/pkg/lockutil/lockutil_unix.go @@ -32,18 +32,18 @@ func WithDirLock(dir string, fn func() error) error { return err } defer dirFile.Close() - if err := Flock(dirFile, unix.LOCK_EX); err != nil { + if err := flock(dirFile, unix.LOCK_EX); err != nil { return fmt.Errorf("failed to lock %q: %w", dir, err) } defer func() { - if err := Flock(dirFile, unix.LOCK_UN); err != nil { + if err := flock(dirFile, unix.LOCK_UN); err != nil { log.L.WithError(err).Errorf("failed to unlock %q", dir) } }() return fn() } -func Flock(f *os.File, flags int) error { +func flock(f *os.File, flags int) error { fd := int(f.Fd()) for { err := unix.Flock(fd, flags) @@ -52,3 +52,27 @@ func Flock(f *os.File, flags int) error { } } } + +func Lock(dir string) (*os.File, error) { + dirFile, err := os.Open(dir) + if err != nil { + return nil, err + } + + if err = flock(dirFile, unix.LOCK_EX); err != nil { + return nil, err + } + + return dirFile, nil +} + +func Unlock(locked *os.File) error { + defer func() { + _ = locked.Close() + }() + + if err := flock(locked, unix.LOCK_UN); err != nil { + return err + } + return nil +} diff --git a/pkg/lockutil/lockutil_windows.go b/pkg/lockutil/lockutil_windows.go index 85658167443..ee73261170d 100644 --- a/pkg/lockutil/lockutil_windows.go +++ b/pkg/lockutil/lockutil_windows.go @@ -43,3 +43,24 @@ func WithDirLock(dir string, fn func() error) error { }() return fn() } + +func Lock(dir string) (*os.File, error) { + dirFile, err := os.OpenFile(dir+".lock", os.O_CREATE, 0644) + if err != nil { + return err + } + // see https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx + // 1 lock immediately + if err = windows.LockFileEx(windows.Handle(dirFile.Fd()), 1, 0, 1, 0, &windows.Overlapped{}); err != nil { + return fmt.Errorf("failed to lock %q: %w", dir, err) + } + return dirFile, nil +} + +func Unlock(locked *os.File) error { + defer func() { + _ = locked.Close() + }() + + return windows.UnlockFileEx(windows.Handle(locked.Fd()), 0, 1, 0, &windows.Overlapped{}) +} diff --git a/pkg/mountutil/mountutil.go b/pkg/mountutil/mountutil.go index f8e184033b0..6223a6d5501 100644 --- a/pkg/mountutil/mountutil.go +++ b/pkg/mountutil/mountutil.go @@ -17,14 +17,12 @@ package mountutil import ( - "errors" "fmt" "os" "path/filepath" "runtime" "strings" - "github.com/containerd/containerd/errdefs" "github.com/containerd/containerd/identifiers" "github.com/containerd/containerd/oci" "github.com/containerd/containerd/pkg/userns" @@ -203,16 +201,11 @@ func handleAnonymousVolumes(s string, volStore volumestore.VolumeStore) (volumeS func handleNamedVolumes(source string, volStore volumestore.VolumeStore) (volumeSpec, error) { var res volumeSpec res.Name = source - vol, err := volStore.Get(res.Name, false) + + // Create returns an existing volume or creates a new one if necessary. + vol, err := volStore.Create(res.Name, nil) if err != nil { - if errors.Is(err, errdefs.ErrNotFound) { - vol, err = volStore.Create(res.Name, nil) - if err != nil { - return res, fmt.Errorf("failed to create volume %q: %w", res.Name, err) - } - } else { - return res, fmt.Errorf("failed to get volume %q: %w", res.Name, err) - } + return res, fmt.Errorf("failed to get volume %q: %w", res.Name, err) } // src is now an absolute path res.Type = Volume diff --git a/pkg/mountutil/mountutilmock/volumestore.mock.go b/pkg/mountutil/mountutilmock/volumestore.mock.go index efed1cfc219..695742f85b7 100644 --- a/pkg/mountutil/mountutilmock/volumestore.mock.go +++ b/pkg/mountutil/mountutilmock/volumestore.mock.go @@ -106,16 +106,26 @@ func (m *MockVolumeStoreMockRecorder) Remove(names any) *gomock.Call { return m.mock.ctrl.RecordCallWithMethodType(m.mock, "Remove", reflect.TypeOf((*MockVolumeStore)(nil).Remove), names) } -// Dir mocks the Dir method of VolumeStore -func (m *MockVolumeStore) Dir() string { +// Lock mocks the Lock method of VolumeStore +func (m *MockVolumeStore) Lock() error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Dir") - ret0, _ := ret[0].(string) - return ret0 + return nil } -// Dir indicates an expected call of Dir -func (m *MockVolumeStoreMockRecorder) Dir() *gomock.Call { +// Lock indicates an expected call of Lock +func (m *MockVolumeStoreMockRecorder) Lock() *gomock.Call { m.mock.ctrl.T.Helper() - return m.mock.ctrl.RecordCallWithMethodType(m.mock, "Dir", reflect.TypeOf((*MockVolumeStore)(nil).Dir)) + return m.mock.ctrl.RecordCallWithMethodType(m.mock, "Lock", reflect.TypeOf((*MockVolumeStore)(nil).Lock)) +} + +// Unlock mocks the Unlock method of VolumeStore +func (m *MockVolumeStore) Unlock() error { + m.ctrl.T.Helper() + return nil +} + +// Unlock indicates an expected call of Unlock +func (m *MockVolumeStoreMockRecorder) Unlock() *gomock.Call { + m.mock.ctrl.T.Helper() + return m.mock.ctrl.RecordCallWithMethodType(m.mock, "Unlock", reflect.TypeOf((*MockVolumeStore)(nil).Unlock)) } diff --git a/pkg/mountutil/volumestore/volumestore.go b/pkg/mountutil/volumestore/volumestore.go index 8093212a7b7..a9a50a0f8e3 100644 --- a/pkg/mountutil/volumestore/volumestore.go +++ b/pkg/mountutil/volumestore/volumestore.go @@ -31,21 +31,33 @@ import ( "github.com/containerd/nerdctl/v2/pkg/strutil" ) -// Path returns a string like `/var/lib/nerdctl/1935db59/volumes/default`. -func Path(dataStore, ns string) (string, error) { - if dataStore == "" || ns == "" { - return "", errdefs.ErrInvalidArgument - } - volStore := filepath.Join(dataStore, "volumes", ns) - return volStore, nil +const ( + dataDirName = "_data" + volumeJSONFileName = "volume.json" +) + +// VolumeStore allows manipulating containers' volumes +// Every method is protected by a file lock, and is safe to use concurrently. +// If you need to use multiple methods successively (for example: List, then Remove), you should instead optin +// for an explicit durable lock, by first calling `Lock` then `defer Unlock`. +// This is also true (and important to do) for any operation that is going to inspect containers before going for +// creation or removal of volumes. +type VolumeStore interface { + Create(name string, labels []string) (*native.Volume, error) + Get(name string, size bool) (*native.Volume, error) + List(size bool) (map[string]native.Volume, error) + Remove(names []string) (removed []string, warns []error, err error) + Lock() error + Unlock() error } // New returns a VolumeStore func New(dataStore, ns string) (VolumeStore, error) { - volStoreDir, err := Path(dataStore, ns) - if err != nil { - return nil, err + if dataStore == "" || ns == "" { + return nil, errdefs.ErrInvalidArgument } + volStoreDir := filepath.Join(dataStore, "volumes", ns) + if err := os.MkdirAll(volStoreDir, 0700); err != nil { return nil, err } @@ -55,175 +67,246 @@ func New(dataStore, ns string) (VolumeStore, error) { return vs, nil } -// DataDirName is "_data" -const DataDirName = "_data" +type volumeStore struct { + dir string + locked *os.File +} -const volumeJSONFileName = "volume.json" +// Lock should be called when you need an exclusive lock on the volume store for an extended period of time +// spanning multiple atomic method calls. +// Be sure to defer Unlock to release it. +func (vs *volumeStore) Lock() error { + if vs.locked != nil { + return fmt.Errorf("cannot lock already locked volume store %q", vs.dir) + } -type VolumeStore interface { - Dir() string - Create(name string, labels []string) (*native.Volume, error) - // Get may return ErrNotFound - Get(name string, size bool) (*native.Volume, error) - List(size bool) (map[string]native.Volume, error) - Remove(names []string) (removed []string, warns []error, err error) -} + dirFile, err := lockutil.Lock(vs.dir) + if err != nil { + return err + } -type volumeStore struct { - // dir is a string like `/var/lib/nerdctl/1935db59/volumes/default`. - // dir is guaranteed to exist. - dir string + vs.locked = dirFile + return nil } -func (vs *volumeStore) Dir() string { - return vs.dir +// Unlock should be called once done (see Lock) to release the persistent lock on the store +func (vs *volumeStore) Unlock() error { + if vs.locked == nil { + return fmt.Errorf("cannot unlock already unlocked volume store %q", vs.dir) + } + + defer func() { + vs.locked = nil + }() + + if err := lockutil.Unlock(vs.locked); err != nil { + return fmt.Errorf("failed to unlock volume store %q: %w", vs.dir, err) + } + return nil } +// Create will create a new volume, or return an existing one if there is one already by that name +// Besides a possible locking error, it might return ErrInvalidArgument, hard filesystem errors, json errors func (vs *volumeStore) Create(name string, labels []string) (*native.Volume, error) { if err := identifiers.Validate(name); err != nil { - return nil, fmt.Errorf("malformed volume name: %w", err) + return nil, fmt.Errorf("malformed volume name: %w (%w)", err, errdefs.ErrInvalidArgument) } volPath := filepath.Join(vs.dir, name) - volDataPath := filepath.Join(volPath, DataDirName) + volDataPath := filepath.Join(volPath, dataDirName) volFilePath := filepath.Join(volPath, volumeJSONFileName) - fn := func() (err error) { - if err := os.Mkdir(volPath, 0700); err != nil { + + vol := &native.Volume{} + + fn := func() error { + // Failures that are not os.ErrExist must exit here + if err := os.Mkdir(volPath, 0700); err != nil && !errors.Is(err, os.ErrExist) { return err } - defer func() { - if err != nil { - os.Remove(volPath) - } - }() - if err := os.Mkdir(volDataPath, 0755); err != nil { + if err := os.Mkdir(volDataPath, 0755); err != nil && !errors.Is(err, os.ErrExist) { return err } - defer func() { - if err != nil { - os.Remove(volDataPath) - } - }() - type volumeOpts struct { + volOpts := struct { Labels map[string]string `json:"labels"` - } - - labelsMap := strutil.ConvertKVStringsToMap(labels) + }{} - volOpts := volumeOpts{ - Labels: labelsMap, + if len(labels) > 0 { + volOpts.Labels = strutil.ConvertKVStringsToMap(labels) } + // Failure here must exit, no need to clean-up labelsJSON, err := json.MarshalIndent(volOpts, "", " ") if err != nil { return err } - defer func() { - if err != nil { - if _, statErr := os.Stat(volFilePath); statErr != nil && !os.IsNotExist(statErr) { - log.L.Warnf("failed to stat volume file: %v", statErr) - return - } else if statErr == nil { - os.Remove(volFilePath) - } + // If it does not exist + if _, err = os.Stat(volFilePath); err != nil { + // Any other stat error than "not exists", hard exit + if !errors.Is(err, os.ErrNotExist) { + return err + } + // Error was does not exist, so, write it + if err = os.WriteFile(volFilePath, labelsJSON, 0644); err != nil { + return err } - }() - return os.WriteFile(volFilePath, labelsJSON, 0644) + } else { + log.L.Warnf("volume %q already exists and will be returned as-is", name) + } + + // At this point, we either have a volume, or created a new one successfully + vol.Name = name + vol.Mountpoint = volDataPath + + return nil } - if err := lockutil.WithDirLock(vs.dir, fn); err != nil && !errors.Is(err, os.ErrExist) { + var err error + if vs.locked == nil { + err = lockutil.WithDirLock(vs.dir, fn) + } else { + err = fn() + } + if err != nil { return nil, err } - // If other new actions that might fail are added below, we should move the cleanup function out of fn. - vol := &native.Volume{ - Name: name, - Mountpoint: volDataPath, - } return vol, nil } +// Get retrieves a native volume from the store +// Besides a possible locking error, it might return ErrInvalidArgument, ErrNotFound, or a filesystem error func (vs *volumeStore) Get(name string, size bool) (*native.Volume, error) { if err := identifiers.Validate(name); err != nil { - return nil, fmt.Errorf("malformed name %s: %w", name, err) + return nil, fmt.Errorf("malformed volume name %q: %w", name, err) } - dataPath := filepath.Join(vs.dir, name, DataDirName) - if _, err := os.Stat(dataPath); err != nil { - if os.IsNotExist(err) { - return nil, fmt.Errorf("volume %q not found: %w", name, errdefs.ErrNotFound) + volPath := filepath.Join(vs.dir, name) + volDataPath := filepath.Join(volPath, dataDirName) + volFilePath := filepath.Join(volPath, volumeJSONFileName) + + vol := &native.Volume{} + + fn := func() error { + if _, err := os.Stat(volDataPath); err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%q does not exist in the volume store: %w", name, errdefs.ErrNotFound) + } + return fmt.Errorf("filesystem error reading %q from the volume store: %w", name, err) } - return nil, err - } - volFilePath := filepath.Join(vs.dir, name, volumeJSONFileName) - volumeDataBytes, err := os.ReadFile(volFilePath) - if err != nil { - if !os.IsNotExist(err) { - return nil, err - } // on else, volume.json does not exists should not be blocking for inspect operation + volumeDataBytes, err := os.ReadFile(volFilePath) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%q labels file does not exist in the volume store: %w", name, errdefs.ErrNotFound) + } + return fmt.Errorf("filesystem error reading %q from the volume store: %w", name, err) + } + + vol.Name = name + vol.Mountpoint = volDataPath + vol.Labels = labels(volumeDataBytes) + + if size { + vol.Size, err = volumeSize(vol) + if err != nil { + return fmt.Errorf("failed reading volume size for %q from the volume store: %w", name, err) + } + } + return nil } - entry := native.Volume{ - Name: name, - Mountpoint: dataPath, - Labels: Labels(volumeDataBytes), + var err error + if vs.locked == nil { + err = lockutil.WithDirLock(vs.dir, fn) + } else { + err = fn() } - if size { - entry.Size, err = Size(&entry) - if err != nil { - return nil, err - } + if err != nil { + return nil, err } - return &entry, nil + + return vol, nil } +// List retrieves all known volumes from the store. +// Besides a possible locking error, it might return ErrNotFound (indicative that the store is in a broken state), or a filesystem error func (vs *volumeStore) List(size bool) (map[string]native.Volume, error) { - dEnts, err := os.ReadDir(vs.dir) - if err != nil { - return nil, err + res := map[string]native.Volume{} + + fn := func() error { + dirEntries, err := os.ReadDir(vs.dir) + if err != nil { + return fmt.Errorf("filesystem error while trying to list volumes from the volume store: %w", err) + } + + for _, dirEntry := range dirEntries { + name := dirEntry.Name() + vol, err := vs.Get(name, size) + if err != nil { + return err + } + res[name] = *vol + } + return nil } - res := make(map[string]native.Volume, len(dEnts)) - for _, dEnt := range dEnts { - name := dEnt.Name() - vol, err := vs.Get(name, size) + var err error + // Since we are calling Get, we need to acquire a global lock + if vs.locked == nil { + err = vs.Lock() if err != nil { - return res, err + return nil, err } - res[name] = *vol + defer vs.Unlock() + } + err = fn() + if err != nil { + return nil, err } return res, nil } +// Remove will remove one or more containers +// Besides a possible locking error, it might return hard filesystem errors +// Any other failure (ErrInvalidArgument, ErrNotFound) is a soft error that will be added the `warns` func (vs *volumeStore) Remove(names []string) (removed []string, warns []error, err error) { fn := func() error { for _, name := range names { + // Invalid name, soft error if err := identifiers.Validate(name); err != nil { - warns = append(warns, fmt.Errorf("malformed volume name: %w", err)) + warns = append(warns, fmt.Errorf("malformed volume name: %w (%w)", err, errdefs.ErrInvalidArgument)) continue } dir := filepath.Join(vs.dir, name) - if _, err := os.Stat(dir); os.IsNotExist(err) { - warns = append(warns, fmt.Errorf("no such volume: %s", name)) - continue + // Does not exist, soft error + if _, err := os.Stat(dir); err != nil { + if os.IsNotExist(err) { + warns = append(warns, fmt.Errorf("no such volume: %s (%w)", name, errdefs.ErrNotFound)) + continue + } + return fmt.Errorf("filesystem error while trying to remove volumes from the volume store: %w", err) } - // This is a hard filesystem error. Exit on this. + // Hard filesystem error, hard error, and stop here if err := os.RemoveAll(dir); err != nil { - return err + return fmt.Errorf("filesystem error while trying to remove volumes from the volume store: %w", err) } + // Otherwise, add it the list of successfully removed removed = append(removed, name) } return nil } - if err := lockutil.WithDirLock(vs.dir, fn); err != nil { - return nil, nil, err + if vs.locked == nil { + err = lockutil.WithDirLock(vs.dir, fn) + } else { + err = fn() } - return removed, warns, nil + + return removed, warns, err } -func Labels(b []byte) *map[string]string { +// Private helpers +func labels(b []byte) *map[string]string { type volumeOpts struct { Labels *map[string]string `json:"labels,omitempty"` } @@ -234,7 +317,7 @@ func Labels(b []byte) *map[string]string { return vo.Labels } -func Size(volume *native.Volume) (int64, error) { +func volumeSize(volume *native.Volume) (int64, error) { var size int64 var walkFn = func(_ string, info os.FileInfo, err error) error { if err != nil {