topolvm · satoru-takeuchi · Jan 23, 2025 · Dec 20, 2024 · Dec 19, 2024 · Dec 19, 2024
@@ -23,9 +23,6 @@ issues:
       text: "SA1019: (.*)\\.(Get)?SizeGb is deprecated: Marked as deprecated in pkg/lvmd/proto/lvmd.proto."
 
     # FIXME: remove this once we have fixed the issues
-    - path: "internal/lvmd/*"
-      linters:
-        - gocyclo
     - path: "test/e2e/*"
       linters:
         - gocyclo

@@ -2,7 +2,6 @@ package command
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"regexp"
@@ -11,16 +10,14 @@ import (
 
 	"github.com/go-logr/logr/funcr"
 	"github.com/go-logr/logr/testr"
-	"github.com/topolvm/topolvm"
 	"github.com/topolvm/topolvm/internal/lvmd/testutils"
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-func Test_lvm_command(t *testing.T) {
+func TestCallLVMStreamed(t *testing.T) {
 	testutils.RequireRoot(t)
 
-	t.Run("simple lvm version should succeed with stream", func(t *testing.T) {
+	t.Run("command output should be read from the returned stream", func(t *testing.T) {
 		ctx := log.IntoContext(context.Background(), testr.New(t))
 		dataStream, err := callLVMStreamed(ctx, verbosityLVMStateNoUpdate, "version")
 		if err != nil {
@@ -39,40 +36,7 @@ func Test_lvm_command(t *testing.T) {
 		}
 	})
 
-	t.Run("simple lvm vgs should return not found but other failures should not", func(t *testing.T) {
-		var messages []string
-		funcLog := funcr.New(func(_, args string) {
-			messages = append(messages, args)
-		}, funcr.Options{
-			Verbosity: 9,
-		})
-
-		ctx := log.IntoContext(context.Background(), funcLog)
-
-		err := callLVM(ctx, "vgs", "non-existing-vg")
-		if err == nil {
-			t.Fatal(err, "vg should not exist")
-		}
-
-		if !IsLVMNotFound(err) {
-			t.Fatal("error should be not found")
-		}
-
-		if len(messages) != 1 || !strings.Contains(messages[0], "invoking command") {
-			t.Fatal("there should be nothing in stdout except the invoking command log")
-		}
-
-		err = callLVM(ctx, "foobar")
-		if err == nil {
-			t.Fatal(err, "command should not be recognized")
-		}
-
-		if IsLVMNotFound(err) {
-			t.Fatal("error should not be not found")
-		}
-	})
-
-	t.Run("simple lvm vgcreate with non existing device should fail and show logs", func(t *testing.T) {
+	t.Run("The Close error should be LVMError and the error message is accessible from it when lvm command fails", func(t *testing.T) {
 		// fakeDeviceName is a string that does not exist on the system (or rather is highly unlikely to exist)
 		// it is used to test the error handling of the callLVMStreamed function
 		fakeDeviceName := "/dev/does-not-exist"
@@ -111,8 +75,48 @@ func Test_lvm_command(t *testing.T) {
 			t.Fatal("No device found message not contained in error")
 		}
 	})
+}
+
+func TestCallLVM(t *testing.T) {
+	testutils.RequireRoot(t)
 
-	t.Run("callLVM should succeed for non-json based calls", func(t *testing.T) {
+	t.Run("The LVMNotFound error should be happened if a given VG does not exists", func(t *testing.T) {
+		var messages []string
+		funcLog := funcr.New(func(_, args string) {
+			messages = append(messages, args)
+		}, funcr.Options{
+			Verbosity: 9,
+		})
+
+		ctx := log.IntoContext(context.Background(), funcLog)
+
+		err := callLVM(ctx, "vgs", "non-existing-vg")
+		if err == nil {
+			t.Fatal(err, "vg should not exist")
+		}
+
+		if !IsLVMNotFound(err) {
+			t.Fatal("error should be not found")
+		}
+
+		if len(messages) != 1 || !strings.Contains(messages[0], "invoking command") {
+			t.Fatal("there should be nothing in stdout except the invoking command log")
+		}
+	})
+
+	t.Run("The returned error should not be LVMNotFound if a generic error happens", func(t *testing.T) {
+		ctx := log.IntoContext(context.Background(), testr.New(t))
+		err := callLVM(ctx, "foobar")
+		if err == nil {
+			t.Fatal(err, "command should not be recognized")
+		}
+
+		if IsLVMNotFound(err) {
+			t.Fatal("error should not be not found")
+		}
+	})
+
+	t.Run("command output should be logged", func(t *testing.T) {
 		var messages []string
 		funcLog := funcr.New(func(_, args string) {
 			messages = append(messages, args)
@@ -147,82 +151,4 @@ func Test_lvm_command(t *testing.T) {
 			t.Fatalf("version from stdout was not logged, existing logs: %v", messages)
 		}
 	})
-
-	t.Run("lv creation", func(t *testing.T) {
-		ctx := ctrl.LoggerInto(context.Background(), testr.New(t))
-		vgName := "lvm_command_test"
-		loop, err := testutils.MakeLoopbackDevice(ctx, vgName)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		err = testutils.MakeLoopbackVG(ctx, vgName, loop)
-		if err != nil {
-			t.Fatal(err)
-		}
-		defer func() { _ = testutils.CleanLoopbackVG(vgName, []string{loop}, []string{vgName}) }()
-
-		vg, err := FindVolumeGroup(ctx, vgName)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		t.Run("create volume with multiple of Sector Size is fine", func(t *testing.T) {
-			err = vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize), []string{"tag"}, 0, "", nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-			vol, err := vg.FindVolume(ctx, "test1")
-			if err != nil {
-				t.Fatal(err)
-			}
-			if vol.Size()%uint64(topolvm.MinimumSectorSize) != 0 {
-				t.Fatalf("expected size to be multiple of sector size %d, got %d", uint64(topolvm.MinimumSectorSize), vol.Size())
-			}
-			if err := vg.RemoveVolume(ctx, "test1"); err != nil {
-				t.Fatal(err)
-			}
-		})
-
-		t.Run("create volume with size not multiple of sector Size to get rejected", func(t *testing.T) {
-			err = vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize)+1, []string{"tag"}, 0, "", nil)
-			if !errors.Is(err, ErrNoMultipleOfSectorSize) {
-				t.Fatalf("expected error to be %v, got %v", ErrNoMultipleOfSectorSize, err)
-			}
-		})
-
-		t.Run("create cached volume and it should not classified as thin volume.", func(t *testing.T) {
-			// create the cachedevice
-			cache_vg_name := "CACHEDEVICE"
-			cache_loop, err := testutils.MakeLoopbackDevice(ctx, cache_vg_name)
-			if err != nil {
-				t.Fatal(err)
-			}
-			err = testutils.MakeLoopbackVG(ctx, cache_vg_name, cache_loop)
-			if err != nil {
-				t.Fatal(err)
-			}
-			// ensure cache device cleanup
-			defer func() { _ = testutils.CleanLoopbackVG(cache_vg_name, []string{cache_loop}, []string{cache_vg_name}) }()
-			vg, err := FindVolumeGroup(ctx, cache_vg_name)
-			if err != nil {
-				t.Fatal(err)
-			}
-			// create cached LV
-			err = vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize), []string{"tag"}, 0, "", []string{"--type", "writecache", "--cachesize", "10M", "--cachedevice", cache_loop})
-			if err != nil {
-				t.Fatal(err)
-			}
-			vol, err := vg.FindVolume(ctx, "test1")
-			if err != nil {
-				t.Fatal(err)
-			}
-			if vol.IsThin() {
-				t.Fatal("Expected test1 to not be thin (eval lv_attr instead of pool)")
-			}
-			if vol.attr[0] != byte(VolumeTypeCached) {
-				t.Fatal("Created a LV but without writecache?")
-			}
-		})
-	})
 }
@@ -0,0 +1,117 @@
+package command
+
+import (
+	"context"
+	"errors"
+	"path"
+	"strconv"
+	"testing"
+
+	"github.com/go-logr/logr/testr"
+	"github.com/topolvm/topolvm"
+	"github.com/topolvm/topolvm/internal/lvmd/testutils"
+	ctrl "sigs.k8s.io/controller-runtime"
+)
+
+func TestVG_CreateVolume(t *testing.T) {
+	ctx := ctrl.LoggerInto(context.Background(), testr.New(t))
+	vg, _ := setupVG(ctx, t, 2)
+
+	t.Run("create volume with multiple of Sector Size is fine", func(t *testing.T) {
+		err := vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize), []string{"tag"}, 0, "", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		vol, err := vg.FindVolume(ctx, "test1")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if vol.Size()%uint64(topolvm.MinimumSectorSize) != 0 {
+			t.Fatalf("expected size to be multiple of sector size %d, got %d", uint64(topolvm.MinimumSectorSize), vol.Size())
+		}
+		if err := vg.RemoveVolume(ctx, "test1"); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("create volume with size not multiple of sector Size to get rejected", func(t *testing.T) {
+		err := vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize)+1, []string{"tag"}, 0, "", nil)
+		if !errors.Is(err, ErrNoMultipleOfSectorSize) {
+			t.Fatalf("expected error to be %v, got %v", ErrNoMultipleOfSectorSize, err)
+		}
+	})
+
+	t.Run("create volume with stripe is fine", func(t *testing.T) {
+		err := vg.CreateVolume(ctx, "test2", 1<<30, nil, 2, "4k", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		_, err = vg.FindVolume(ctx, "test2")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		err = vg.CreateVolume(ctx, "test3", 1<<30, nil, 2, "4M", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		_, err = vg.FindVolume(ctx, "test3")
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+}
+
+func TestLogicalVolume_IsThin(t *testing.T) {
+	ctx := ctrl.LoggerInto(context.Background(), testr.New(t))
+	vg, loops := setupVG(ctx, t, 1)
+
+	t.Run("A cached volume should not classified as thin volume.", func(t *testing.T) {
+		// create cached LV
+		err := vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize), []string{"tag"}, 0, "", []string{"--type", "writecache", "--cachesize", "10M", "--cachedevice", loops[0]})
+		if err != nil {
+			t.Fatal(err)
+		}
+		vol, err := vg.FindVolume(ctx, "test1")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if vol.IsThin() {
+			t.Fatal("Expected test1 to not be thin (eval lv_attr instead of pool)")
+		}
+		if vol.attr[0] != byte(VolumeTypeCached) {
+			t.Fatal("Created a LV but without writecache?")
+		}
+	})
+}
+
+func setupVG(ctx context.Context, t *testing.T, pvCount int) (*VolumeGroup, []string) {
+	testutils.RequireRoot(t)
+
+	vgName := t.Name()
+
+	var loops []string
+	var files []string
+	for i := 0; i < pvCount; i++ {
+		file := path.Join(t.TempDir(), vgName+strconv.Itoa(i))
+		loop, err := testutils.MakeLoopbackDevice(ctx, file)
+		if err != nil {
+			t.Fatal(err)
+		}
+		loops = append(loops, loop)
+		files = append(files, file)
+	}
+
+	err := testutils.MakeLoopbackVG(ctx, vgName, loops...)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = testutils.CleanLoopbackVG(vgName, loops, files) })
+
+	vg, err := FindVolumeGroup(ctx, vgName)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return vg, loops
+}