Repository: topolvm/topolvm
Pull request #844 (2024-02-28 17:11:11)

Introduce 4096 byte Sector Size Minimum for Rounding LV Sizes on Creation and Resizing
This PR adds a 4096 Minimum Size limitation for all Logical Volumes that are created. The reason is that if one decides to use byte level precision on a PVC, that byte amount is now passed through to lvcreate. However if this byte-amount is not a multiple of the sector size (for most machines either 512, 1024 or 4096) then LVM will throw an error like

```
Size is not a multiple of 512. Try using 221920768 or 221921280.
  Invalid argument for --virtualsize: 221920847b 
```

In this case, we should round down in order to allow for a PVC creation with slightly different Size than the one requested as this is much better than getting rejected the PVC scheduling. 

The code is now changed to automatically round down or up (based on wether the limit or the request bytes are used and which one is larger). For edge cases during rounding we introduce new error cases.

Note that even though the 4KiB limit here is important for validation, the size might in the end still be rounded up to the nearest physical extent size (4MiB by default)

daichimukai: Hi @jakobmoellerdev,

Thank you for your PR! Looks like a reasonable change to me. Before we start a review of the implementation, let me discuss the implementation strategy.

I agree that we introduce LV size limitation to fit into the 4096 byte alignment. But I'm not sure it would be reasonable to implement this logic in lvmd.

The lvmd creates LVs with the size decided by the topolvm-controller based on the `capacity_range` field of the CSI's CreateVolume RPC request. According to the spec, `capacity_range` must be honored if specified. I'm worried that we will be violating this spec if lvmd rounds down the requested size. (For example, consider the case where the request in `capacity_range` is 4MiB+1B.)

>  // This field is OPTIONAL. This allows the CO to specify the capacity
>  // requirement of the volume to be provisioned. If not specified, the
>  // Plugin MAY choose an implementation-defined capacity range. If
>  // specified it MUST always be honored, even when creating volumes
>  // from a source; which MAY force some backends to internally extend
>  // the volume after creating it.
>  CapacityRange capacity_range = 2;

https://github.com/container-storage-interface/spec/blob/886cd4877954568f9722cbe14d46b9fc16379d62/spec.md#createvolume

So, I'll suggest the following strategy:

1. The topolvm-controller creates LogicalVolume CR with the size which is the request of `capacity_range` rounded **up** to fit into the 4096 byte alignment.
2. (optional) The lvmd checks if the requested LV size is aligned with the 4096 byte alignment.

Additionally, we might need to round up the size when the topolvm-controller adds requests to Pods.
jakobmoellerdev: Thanks for the input. Agreed that we can change the approach to make it better fit into the CSI Spec.

I just checked CapacityRange and it seems like there are two components to it: RequiredBytes and LimitBytes.

I suggest we use your approach then and do the following logic:

1. [CreateVolume](https://github.com/topolvm/topolvm/blob/94b7f88fa74c391368e8906ceb6649aa52d29302/driver/controller.go#L114) verifies `req.GetCapacityRange().GetRequiredBytes() % 4096`, if it does not solve to 0, round up to nearest 4096 sector. If that new rounded value is larger than req.GetCapacityRange().GetLimitBytes(), throw `status.Error(codes.InvalidArgument, "capacity range is invalid, request was rounded up to nearest 4096 sector but cannot fit into capacity limit")` 
2. LVMD verifies that `size % 4096 != 0` and if it is, we should throw an error


Towards your point of

> Additionally, we might need to round up the size when the topolvm-controller adds requests to Pods.

Could you tell me what needs to change and why here? Then I will implement accordingly. Not sure what this means right now (maybe you mean the resource extensions on the Pod)
daichimukai: > >  Additionally, we might need to round up the size when the topolvm-controller adds requests to Pods.
> 
> Could you tell me what needs to change and why here? Then I will implement accordingly. Not sure what this means right now (maybe you mean the resource extensions on the Pod)

I changed my mind. Let's leave it as is. When the scheduler decides a node to which allocate Pods, it would be better to know the accurate size of the LVs to create as much as possible. However, since the discrepancy is at most 4096 bytes, we can leave it as it is until it becomes a problem.
jakobmoellerdev: > Take care of the case where requestByte == 0 and limitByte < 4096. It is not allowed now. It might be better not to use early returns and force return values always pass the "round up logic".

I believe this is covered already:

```
if requestBytes == 0 {
   if limitBytes > 0 && topolvm.DefaultSize > limitBytes { 
      return limitBytes, nil 
   } 
}
```

translates to 

```
if requestBytes == 0 & limitBytes > 0 && limitBytes < 4096
   return limitBytes
```

daichimukai: @jakobmoellerdev
Thank you, LGTM. Please squash commits into one commit.

Patch:
diff --git a/.github/workflows/helm-release.yaml b/.github/workflows/helm-release.yaml
index 0cb07aa8..bb0f1654 100644
--- a/.github/workflows/helm-release.yaml
+++ b/.github/workflows/helm-release.yaml
@@ -19,7 +19,7 @@ jobs:
       # This version is written in `charts/topolvm/README.md`,
       # so do not update it if not necessary.
       - name: "Install Helm"
-        uses: azure/setup-helm@v4
+        uses: azure/setup-helm@v3
         with:
           version: "v3.5.0"
 
diff --git a/constants.go b/constants.go
index 59afd8ce..531d85ba 100644
--- a/constants.go
+++ b/constants.go
@@ -94,6 +94,13 @@ const DefaultSizeGb = 1
 // DefaultSize is DefaultSizeGb in bytes
 const DefaultSize = int64(DefaultSizeGb << 30)
 
+// MinimumSectorSize is the minimum size in bytes for volumes (PVC or generic ephemeral volumes).
+// It is derived from the usual sector size of 512,1024 or 4096 bytes for logical volumes.
+// While Sector Sizes of 512 are common, using 4096 is safe
+// As it also aligns with 512 and 1024 byte sectors, and is the default for most modern disks.
+// Going lower than this size will cause validation issues on volume creation for the user.
+const MinimumSectorSize = int64(4096)
+
 // Label key that indicates The controller/user who created this resource
 // https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
 const CreatedbyLabelKey = "app.kubernetes.io/created-by"
diff --git a/internal/driver/controller.go b/internal/driver/controller.go
index 7340d882..81b49e72 100644
--- a/internal/driver/controller.go
+++ b/internal/driver/controller.go
@@ -430,19 +430,52 @@ func convertRequestCapacityBytes(requestBytes, limitBytes int64) (int64, error)
 		)
 	}
 
-	// if requestBytes is 0, default to max(1Gi, limitBytes>0)
-	// 1. if limitBytes is 0, default to 1Gi
-	// 2. if limitBytes is greater than 1Gi, default to 1Gi
-	// 3. if limitBytes is less than 1Gi, default to limitBytes
+	// if requestBytes is 0 and
+	// 1. if limitBytes == 0, default to 1Gi
+	// 2. if limitBytes >= 1Gi, default to 1Gi
+	// 3. if limitBytes < 1Gi, default to rounded-down 4096 multiple of limitBytes.
+	//    if rounded-down 4096 multiple of limitBytes == 0, return error
 	if requestBytes == 0 {
-		if limitBytes > 0 && topolvm.DefaultSize > limitBytes {
-			return limitBytes, nil
+		// if there is no limit or the limit is bigger or equal to the default, use the default
+		if limitBytes == 0 || limitBytes >= topolvm.DefaultSize {
+			return topolvm.DefaultSize, nil
+		}
+
+		roundedLimit := roundDown(limitBytes, topolvm.MinimumSectorSize)
+		if roundedLimit == 0 {
+			return 0, fmt.Errorf("requested capacity is 0, because it defaulted to the limit (%d) and was rounded down to the nearest sector size (%d). "+
+				"specify the limit to be at least %d bytes", limitBytes, topolvm.MinimumSectorSize, topolvm.MinimumSectorSize)
+		}
+
+		return roundedLimit, nil
+	}
+
+	if requestBytes%topolvm.MinimumSectorSize != 0 {
+		// round up to the nearest multiple of the sector size
+		requestBytes = roundUp(requestBytes, topolvm.MinimumSectorSize)
+		// after rounding up, we might overshoot the limit
+		if limitBytes > 0 && requestBytes > limitBytes {
+			return 0, fmt.Errorf(
+				"requested capacity rounded to nearest sector size (%d) exceeds limit capacity, "+
+					"either specify a lower request or a higher limit: request=%d limit=%d",
+				topolvm.MinimumSectorSize, requestBytes, limitBytes,
+			)
 		}
-		return topolvm.DefaultSize, nil
 	}
+
 	return requestBytes, nil
 }
 
+// roundUp rounds up the size to the nearest given multiple.
+func roundUp(size int64, multiple int64) int64 {
+	return (size + multiple - 1) / multiple * multiple
+}
+
+// roundDown rounds down the size to the nearest given multiple.
+func roundDown(size int64, multiple int64) int64 {
+	return size - size%multiple
+}
+
 func (s controllerServerNoLocked) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) {
 	ctrlLogger.Info("DeleteVolume called",
 		"volume_id", req.GetVolumeId(),
diff --git a/internal/lvmd/command/lvm.go b/internal/lvmd/command/lvm.go
index 8e2ea64c..90d2aa51 100644
--- a/internal/lvmd/command/lvm.go
+++ b/internal/lvmd/command/lvm.go
@@ -5,6 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"path"
+
+	"github.com/topolvm/topolvm"
 )
 
 const (
@@ -15,6 +17,10 @@ const (
 // ErrNotFound is returned when a VG or LV is not found.
 var ErrNotFound = errors.New("not found")
 
+// ErrNoMultipleOfSectorSize is returned when a volume is requested that is smaller than the minimum sector size.
+var ErrNoMultipleOfSectorSize = fmt.Errorf("cannot create volume as given size "+
+	"is not a multiple of %d and could get rejected", topolvm.MinimumSectorSize)
+
 // VolumeGroup represents a volume group of linux lvm.
 // The state should be considered immutable and will not automatically update.
 // The Update method should be called to refresh the state in case it is known that the state may have changed.
@@ -200,6 +206,11 @@ func (vg *VolumeGroup) convertLV(lv lv) *LogicalVolume {
 // lvcreateOptions are additional arguments to pass to lvcreate.
 func (vg *VolumeGroup) CreateVolume(ctx context.Context, name string, size uint64, tags []string, stripe uint, stripeSize string,
 	lvcreateOptions []string) error {
+
+	if size%uint64(topolvm.MinimumSectorSize) != 0 {
+		return ErrNoMultipleOfSectorSize
+	}
+
 	lvcreateArgs := []string{"lvcreate", "-n", name, "-L", fmt.Sprintf("%vb", size), "-W", "y", "-y"}
 	for _, tag := range tags {
 		lvcreateArgs = append(lvcreateArgs, "--addtag")
@@ -315,6 +326,11 @@ func (t *ThinPool) Resize(ctx context.Context, newSize uint64) error {
 	if t.state.size == newSize {
 		return nil
 	}
+
+	if newSize%uint64(topolvm.MinimumSectorSize) != 0 {
+		return ErrNoMultipleOfSectorSize
+	}
+
 	if err := callLVM(ctx, "lvresize", "-f", "-L", fmt.Sprintf("%vb", newSize), t.state.fullName); err != nil {
 		return err
 	}


Test patch:
diff --git a/internal/driver/controller_test.go b/internal/driver/controller_test.go
index cde27f6d..b605fe17 100644
--- a/internal/driver/controller_test.go
+++ b/internal/driver/controller_test.go
@@ -1,7 +1,10 @@
 package driver
 
 import (
+	"fmt"
 	"testing"
+
+	"github.com/topolvm/topolvm"
 )
 
 func Test_convertRequestCapacityBytes(t *testing.T) {
@@ -29,13 +32,22 @@ func Test_convertRequestCapacityBytes(t *testing.T) {
 		t.Error("should report capacity limit exceeded")
 	}
 
-	v, err := convertRequestCapacityBytes(0, 10)
+	v, err := convertRequestCapacityBytes(0, topolvm.MinimumSectorSize-1)
+	if err == nil {
+		t.Error("should be error")
+	}
+	if err.Error() != "requested capacity is 0, because it defaulted to the limit (4095) and was rounded down to the nearest sector size (4096). specify the limit to be at least 4096 bytes" {
+		t.Errorf("should error if rounded-down limit is 0: %d", v)
+	}
+
+	v, err = convertRequestCapacityBytes(0, topolvm.MinimumSectorSize+1)
 	if err != nil {
 		t.Error("should not be error")
 	}
-	if v != 10 {
-		t.Errorf("should be the limit capacity by default if 0 is supplied and limit is smaller than 1Gi: %d", v)
+	if v != topolvm.MinimumSectorSize {
+		t.Errorf("should be nearest rounded down multiple of sector size if 0 is supplied and limit is larger than sector-size: %d", v)
 	}
+
 	v, err = convertRequestCapacityBytes(0, 2<<30)
 	if err != nil {
 		t.Error("should not be error")
@@ -48,7 +60,7 @@ func Test_convertRequestCapacityBytes(t *testing.T) {
 	if err != nil {
 		t.Error("should not be error")
 	}
-	if v != 1 {
+	if v != topolvm.MinimumSectorSize {
 		t.Errorf("should be resolve capacities < 1Gi without error if there is no limit: %d", v)
 	}
 
@@ -60,12 +72,12 @@ func Test_convertRequestCapacityBytes(t *testing.T) {
 		t.Errorf("should be 1073741824 in byte precision: %d", v)
 	}
 
-	v, err = convertRequestCapacityBytes(1<<30+1, 1<<30+1)
-	if err != nil {
-		t.Error("should not be error")
+	_, err = convertRequestCapacityBytes(1<<30+1, 1<<30+1)
+	if err == nil {
+		t.Error("should be error")
 	}
-	if v != (1<<30)+1 {
-		t.Errorf("should be 1073741825 in byte precision: %d", v)
+	if err.Error() != "requested capacity rounded to nearest sector size (4096) exceeds limit capacity, either specify a lower request or a higher limit: request=1073745920 limit=1073741825" {
+		t.Error("should report capacity limit exceeded after rounding")
 	}
 
 	v, err = convertRequestCapacityBytes(0, 0)
@@ -75,4 +87,36 @@ func Test_convertRequestCapacityBytes(t *testing.T) {
 	if v != 1<<30 {
 		t.Errorf("should be 1073741825 in byte precision: %d", v)
 	}
+
+	v, err = convertRequestCapacityBytes(1, topolvm.MinimumSectorSize*2)
+	if err != nil {
+		t.Error("should not be error")
+	}
+	if v != topolvm.MinimumSectorSize {
+		t.Errorf("should be %d in byte precision: %d", topolvm.MinimumSectorSize, v)
+	}
+
+}
+
+func Test_roundUp(t *testing.T) {
+	testCases := []struct {
+		size     int64
+		multiple int64
+		expected int64
+	}{
+		{12, 4, 12},
+		{11, 4, 12},
+		{13, 4, 16},
+		{0, 4, 0},
+	}
+
+	for _, tc := range testCases {
+		name := fmt.Sprintf("nearest rounded up multiple of %d from %d should be %d", tc.multiple, tc.size, tc.expected)
+		t.Run(name, func(t *testing.T) {
+			rounded := roundUp(tc.size, tc.multiple)
+			if rounded != tc.expected {
+				t.Errorf("%s, but was %d", name, rounded)
+			}
+		})
+	}
 }
diff --git a/internal/lvmd/command/lvm_command_test.go b/internal/lvmd/command/lvm_command_test.go
index 1848a9d8..c78185c2 100644
--- a/internal/lvmd/command/lvm_command_test.go
+++ b/internal/lvmd/command/lvm_command_test.go
@@ -2,6 +2,7 @@ package command
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -10,6 +11,9 @@ 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"
 )
 
@@ -113,4 +117,48 @@ 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 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)
+			}
+		})
+	})
 }