Merge pull request #53 from jakobmoellerdev/release-4.15-OCPBUGS-30032

[release-4.15] OCPBUGS-30298: fix: round down/up req/limit bytes to 4096 sector for sizes

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"

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

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

lvmd/command/lvm.go

@@ -9,6 +9,7 @@ import (
 	"os/exec"
 	"path"
 
+	"github.com/topolvm/topolvm"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -22,6 +23,10 @@ var Containerized bool = false
 // 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)
+
 // wrapExecCommand calls cmd with args but wrapped to run
 // on the host
 func wrapExecCommand(cmd string, args ...string) *exec.Cmd {
@@ -210,6 +215,11 @@ func (g *VolumeGroup) ListVolumes() []*LogicalVolume {
 // lvcreateOptions are additional arguments to pass to lvcreate.
 func (g *VolumeGroup) CreateVolume(ctx context.Context, name string, size uint64, tags []string, stripe uint, stripeSize string,
 	lvcreateOptions []string) (*LogicalVolume, error) {
+
+	if size%uint64(topolvm.MinimumSectorSize) != 0 {
+		return nil, ErrNoMultipleOfSectorSize
+	}
+
 	lvcreateArgs := []string{"-n", name, "-L", fmt.Sprintf("%vb", size), "-W", "y", "-y"}
 	for _, tag := range tags {
 		lvcreateArgs = append(lvcreateArgs, "--addtag")
@@ -318,6 +328,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
 	}