diff --git a/cmd/nerdctl/volume_remove_linux_test.go b/cmd/nerdctl/volume_remove_linux_test.go index 35a478e530e..6023381c782 100644 --- a/cmd/nerdctl/volume_remove_linux_test.go +++ b/cmd/nerdctl/volume_remove_linux_test.go @@ -21,18 +21,193 @@ import ( "testing" "github.com/containerd/nerdctl/v2/pkg/testutil" + "gotest.tools/v3/icmd" ) func TestVolumeRemove(t *testing.T) { t.Parallel() + base := testutil.NewBase(t) - tID := testutil.Identifier(t) - base.Cmd("volume", "create", tID).AssertOK() - base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID), "--name", tID, testutil.CommonImage).AssertOK() - defer base.Cmd("rm", "-f", tID).Run() + malformed := "malformed volume name" + notFound := "no such volume" + requireArg := "requires at least 1 arg" + inUse := "is in use" + if base.Target == testutil.Docker { + malformed = "no such volume" + } + + 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", "rm") + }, + 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", "rm", "∞") + }, + 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", "rm", "doesnotexist") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: notFound, + } + }, + }, + { + description: "busy volume", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "rm", tID) + }, + 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 { + return base.Cmd("volume", "rm", tID) + }, + tearUp: func(tID string) { + base.Cmd("volume", "create", tID).AssertOK() + base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID), "--name", tID, testutil.CommonImage).AssertOK() + base.Cmd("rm", "-f", tID).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{ + Out: tID, + } + }, + }, + { + description: "dangling volume", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "rm", tID) + }, + tearUp: func(tID string) { + base.Cmd("volume", "create", tID).AssertOK() + }, + tearDown: func(tID string) { + base.Cmd("volume", "rm", "-f", tID).Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + Out: tID, + } + }, + }, + { + "part success multi remove", + func(tID string) *testutil.Cmd { + return base.Cmd("volume", "rm", "invalid∞", "nonexistent", tID) + }, + func(tID string) { + base.Cmd("volume", "create", tID).AssertOK() + }, + func(tID string) { + base.Cmd("volume", "rm", "-f", tID).Run() + }, + func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Out: tID, + Err: notFound, + } + }, + }, + { + description: "success multi-remove", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "rm", tID+"-1", tID+"-2") + }, + tearUp: func(tID string) { + base.Cmd("volume", "create", tID+"-1").AssertOK() + base.Cmd("volume", "create", tID+"-2").AssertOK() + }, + tearDown: func(tID string) { + base.Cmd("volume", "rm", "-f", tID+"-1", tID+"-2").Run() + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + Out: tID + "-1\n" + tID + "-2", + } + }, + }, + { + description: "failing multi-remove", + command: func(tID string) *testutil.Cmd { + return base.Cmd("volume", "rm", "nonexist1", "nonexist2") + }, + expected: func(tID string) icmd.Expected { + return icmd.Expected{ + ExitCode: 1, + Err: notFound, + } + }, + }, + } + + for _, test := range testCases { + currentTest := test + t.Run(currentTest.description, func(tt *testing.T) { + tt.Parallel() + + tID := testutil.Identifier(tt) + + if currentTest.tearDown != nil { + currentTest.tearDown(tID) + tt.Cleanup(func() { + currentTest.tearDown(tID) + }) + } + if currentTest.tearUp != nil { + currentTest.tearUp(tID) + } - base.Cmd("volume", "rm", tID).AssertFail() - base.Cmd("rm", "-f", tID).AssertOK() - base.Cmd("volume", "rm", tID).AssertOK() + cmd := currentTest.command(tID) + cmd.Assert(currentTest.expected(tID)) + }) + } } diff --git a/pkg/cmd/container/remove.go b/pkg/cmd/container/remove.go index 04c022e719b..b6fb87c93bb 100644 --- a/pkg/cmd/container/remove.go +++ b/pkg/cmd/container/remove.go @@ -157,7 +157,7 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions return err } defer func() { - if _, err := volStore.Remove(anonVolumes); err != nil { + if _, errs, err := volStore.Remove(anonVolumes); err != nil || len(errs) > 0 { log.G(ctx).WithError(err).Warnf("failed to remove anonymous volumes %v", anonVolumes) } }() diff --git a/pkg/cmd/volume/prune.go b/pkg/cmd/volume/prune.go index d8161be8bfa..f9dff99f428 100644 --- a/pkg/cmd/volume/prune.go +++ b/pkg/cmd/volume/prune.go @@ -57,7 +57,7 @@ func Prune(ctx context.Context, client *containerd.Client, options types.VolumeP } removeNames = append(removeNames, volume.Name) } - removedNames, err := volStore.Remove(removeNames) + 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 e25a3ff19a8..509957bb978 100644 --- a/pkg/cmd/volume/rm.go +++ b/pkg/cmd/volume/rm.go @@ -19,9 +19,11 @@ package volume import ( "context" "encoding/json" + "errors" "fmt" "github.com/containerd/containerd" + "github.com/containerd/log" "github.com/containerd/nerdctl/v2/pkg/api/types" "github.com/containerd/nerdctl/v2/pkg/inspecttypes/dockercompat" "github.com/containerd/nerdctl/v2/pkg/labels" @@ -44,23 +46,28 @@ func Remove(ctx context.Context, client *containerd.Client, volumes []string, op var volumenames []string // nolint: prealloc for _, name := range volumes { - volume, err := volStore.Get(name, false) - if err != nil { - return err - } - if _, ok := usedVolumes[volume.Name]; ok { + if _, ok := usedVolumes[name]; ok { return fmt.Errorf("volume %q is in use", name) } volumenames = append(volumenames, name) } - removedNames, err := volStore.Remove(volumenames) + // if err is set, this is a hard filesystem error + removedNames, warns, err := volStore.Remove(volumenames) if err != nil { return err } + // Otherwise, output on stdout whatever was successful for _, name := range removedNames { fmt.Fprintln(options.Stdout, name) } - return err + // Log the rest + for _, volErr := range warns { + log.G(ctx).Warn(volErr) + } + if len(warns) > 0 { + return errors.New("some volumes could not be removed") + } + return nil } func usedVolumes(ctx context.Context, containers []containerd.Container) (map[string]struct{}, error) { diff --git a/pkg/mountutil/mountutilmock/volumestore.mock.go b/pkg/mountutil/mountutilmock/volumestore.mock.go index b5c933cd125..efed1cfc219 100644 --- a/pkg/mountutil/mountutilmock/volumestore.mock.go +++ b/pkg/mountutil/mountutilmock/volumestore.mock.go @@ -92,12 +92,12 @@ func (m *MockVolumeStoreMockRecorder) List(size any) *gomock.Call { } // Remove mocks the Remove method of VolumeStore -func (m *MockVolumeStore) Remove(names []string) ([]string, error) { +func (m *MockVolumeStore) Remove(names []string) (removed []string, warns []error, err error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Remove", names) ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) - return ret0, ret1 + return ret0, []error{}, ret1 } // Remove indicates an expected call of Remove diff --git a/pkg/mountutil/volumestore/volumestore.go b/pkg/mountutil/volumestore/volumestore.go index 430791223be..8093212a7b7 100644 --- a/pkg/mountutil/volumestore/volumestore.go +++ b/pkg/mountutil/volumestore/volumestore.go @@ -18,6 +18,7 @@ package volumestore import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -65,7 +66,7 @@ type VolumeStore interface { // Get may return ErrNotFound Get(name string, size bool) (*native.Volume, error) List(size bool) (map[string]native.Volume, error) - Remove(names []string) (removedNames []string, err error) + Remove(names []string) (removed []string, warns []error, err error) } type volumeStore struct { @@ -80,7 +81,7 @@ func (vs *volumeStore) Dir() string { func (vs *volumeStore) Create(name string, labels []string) (*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: %w", err) } volPath := filepath.Join(vs.dir, name) volDataPath := filepath.Join(volPath, DataDirName) @@ -131,12 +132,11 @@ func (vs *volumeStore) Create(name string, labels []string) (*native.Volume, err return os.WriteFile(volFilePath, labelsJSON, 0644) } - if err := lockutil.WithDirLock(vs.dir, fn); err != nil { + if err := lockutil.WithDirLock(vs.dir, fn); err != nil && !errors.Is(err, os.ErrExist) { 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, @@ -196,14 +196,19 @@ func (vs *volumeStore) List(size bool) (map[string]native.Volume, error) { return res, nil } -func (vs *volumeStore) Remove(names []string) ([]string, error) { - var removed []string +func (vs *volumeStore) Remove(names []string) (removed []string, warns []error, err error) { fn := func() error { for _, name := range names { if err := identifiers.Validate(name); err != nil { - return fmt.Errorf("malformed name %s: %w", name, err) + warns = append(warns, fmt.Errorf("malformed volume name: %w", err)) + 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 + } + // This is a hard filesystem error. Exit on this. if err := os.RemoveAll(dir); err != nil { return err } @@ -211,8 +216,11 @@ func (vs *volumeStore) Remove(names []string) ([]string, error) { } return nil } - err := lockutil.WithDirLock(vs.dir, fn) - return removed, err + + if err := lockutil.WithDirLock(vs.dir, fn); err != nil { + return nil, nil, err + } + return removed, warns, nil } func Labels(b []byte) *map[string]string {