Volume QA, "volume remove" (fixes and tests)

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))
+		})
+	}
 }

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)
 			}
 		}()

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
 	}

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) {

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

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,23 +196,31 @@ 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
 			}
 			removed = append(removed, name)
 		}
 		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 {