rbd: add support for rbd striping #3132

Madhu-1 · 2022-05-24T08:25:40Z

RBD supports creating rbd images with object size, strip unit, and strip count to support striping. This PR adds the support
for the same.

More details about strip at https://docs.ceph.com/en/quincy/man/8/rbd/#striping

fixes: #3124

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

@idryomov Please review.

idryomov · 2022-05-24T08:28:03Z

internal/rbd/rbd_util.go

@@ -96,6 +96,11 @@ type rbdImage struct {
 	// VolSize is the size of the RBD image backing this rbdImage.
 	VolSize int64

+	// image striping configurations.
+	StripCount uint64


Here and everywhere else in the PR, including comments:

Suggested change

StripCount uint64

StripeCount uint64

idryomov · 2022-05-24T08:28:19Z

internal/rbd/rbd_util.go

@@ -96,6 +96,11 @@ type rbdImage struct {
 	// VolSize is the size of the RBD image backing this rbdImage.
 	VolSize int64

+	// image striping configurations.
+	StripCount uint64
+	StripUnit  uint64


Here and everywhere else in the PR, including comments:

Suggested change

StripUnit uint64

StripeUnit uint64

idryomov · 2022-05-24T08:29:27Z

internal/rbd/rbd_util.go

+	// image striping configurations.
+	StripCount uint64
+	StripUnit  uint64
+	ObjectSize int64


Why is ObjectSize signed, while StripeUnit and StripeCount are unsigned?

I was thinking order and objectSize both are the same took it from here https://github.com/ceph/go-ceph/blob/master/rbd/rbd.go#L153-L155. after looking at https://github.com/ceph/ceph/blob/master/src/librbd/api/Image.cc#L577-L587 keeping it as unit64

idryomov · 2022-05-24T08:38:17Z

internal/rbd/rbd_util.go

+
+	if pOpts.ObjectSize != 0 {
+		logMsg += fmt.Sprintf(", object size %d", pOpts.ObjectSize)
+		err := options.SetUint64(librbd.RbdImageOptionOrder, uint64(pOpts.ObjectSize))


This is wrong: RbdImageOptionOrder expects a log-base-2 of ObjectSize, e.g. 22 for 4M.

idryomov · 2022-05-24T08:43:01Z

internal/rbd/rbd_util.go

+		if err != nil {
+			return fmt.Errorf("failed to set object size: %w", err)
+		}
+	}


Can image create/clone options setup code be factored out, so that setting RbdImageOptionDataPool, RbdImageOptionFeatures, RbdImageOptionStripeCount, etc is done in one place?

idryomov · 2022-05-24T08:52:18Z

e2e/rbd_helper.go

+		return err
+	}
+
+	if imgInfo.Order != objectSize {


Don't confuse image order and image object size. They refer to the same value, but it is reported differently for historical reasons. object_size is the object size in bytes, order is log-base-2(object_size).

Thanks, Ilya. fixed now.

Rakshith-R · 2022-05-24T09:33:37Z

examples/rbd/storageclass.yaml

+   # Image stripping, Refer https://docs.ceph.com/en/latest/man/8/rbd/#striping
+   # For more details
+   # (optional) stripe unit in bytes.
+   # stripUnit: <>
+   # (optional) objects to stripe over before looping.
+   # stripCount: <>
+   # (optional) The object size in bytes.
+   # objectSize: <>


Add these options to https://github.com/ceph/ceph-csi/blob/devel/docs/deploy-rbd.md#configuration Available volume parameters: too ?

Madhu-1 · 2022-05-24T10:15:31Z

@idryomov made requested changes, PTAL

yati1998 · 2022-05-24T10:20:19Z

e2e/rbd.go

+				validateOmapCount(f, 1, rbdType, defaultRBDPool, volumesType)
+				err = validateStripe(f, pvc, stripeUnit, stripeCount, objectSize)
+				if err != nil {
+					e2elog.Failf("failed to validate stripe %v", err)


e2elog.Failf("failed to validate stripe %v", err) -> e2elog.Failf("failed to validate stripe: %v", err) to follow similar log format.

yati1998 · 2022-05-24T10:20:38Z

e2e/rbd.go

+				validateOmapCount(f, 1, rbdType, defaultRBDPool, snapsType)
+				err = validateStripe(f, pvcClone, stripeUnit, stripeCount, objectSize)
+				if err != nil {
+					e2elog.Failf("failed to validate stripe for clone %v", err)


Same here as well

yati1998 · 2022-05-24T10:21:10Z

e2e/rbd.go

+				validateOmapCount(f, 2, rbdType, defaultRBDPool, volumesType)
+				err = validateStripe(f, pvcClone, stripeUnit, stripeCount, objectSize)
+				if err != nil {
+					e2elog.Failf("failed to validate stripe for clone %v", err)


yati1998 · 2022-05-24T10:22:21Z

e2e/rbd_helper.go

+		fmt.Sprintf("rbd info %s %s --format json", rbdOptions(poolName), imageName),
+		rookNamespace)
+	if err != nil {
+		return imgInfo, fmt.Errorf("failed to get rbd info %w", err)


fmt.Errorf("failed to get rbd info %w", err) -> fmt.Errorf("failed to get rbd info: %w", err)

yati1998 · 2022-05-24T10:22:29Z

e2e/rbd_helper.go

+		return imgInfo, fmt.Errorf("failed to get rbd info %w", err)
+	}
+	if stdErr != "" {
+		return imgInfo, fmt.Errorf("failed to get rbd info %v", stdErr)


Same as above

e2e/rbd_helper.go

yati1998

LGTM

Madhu-1 · 2022-05-25T09:05:58Z

@Mergifyio rebase

mergify · 2022-05-25T09:06:11Z

rebase

✅ Branch has been successfully rebased

idryomov · 2022-05-25T10:44:07Z

e2e/rbd.go

+			By("validate rbd image stripe", func() {
+				stripeUnit := 4096
+				stripeCount := 8
+				objectSize := 4096


I would pick an object size that would have actually resulted in striping going on, had this test also issued some I/O against the image. For example, 131072.

No, am not running I/O. do I need to run IO before doing this check?

idryomov · 2022-05-25T10:47:54Z

e2e/rbd_helper.go

+type imageInfo struct {
+	ObjectUUID  string `json:"name"`
+	Size        int64  `json:"size"`
+	Format      int64  `json:"format"`


Why int64 instead of int? format is really either 1 or 2.

got it from auto-generated code from the rbd info --format=json. fixed now

idryomov · 2022-05-25T10:50:49Z

e2e/rbd_helper.go

+
+// imageInfo strongly typed JSON spec for image info.
+type imageInfo struct {
+	ObjectUUID  string `json:"name"`


Isn't name in rbd info output just the image name? Just wondering what ObjectUUID is supposed to stand for and whether ImageName might be a better name for this field.

got it from auto-generated code from the rbd info --format=json. fixed now

idryomov · 2022-05-25T10:55:16Z

internal/rbd/rbd_util.go

+	}
+
+	if val, ok := options["objectSize"]; ok {
+		objSize, err := strconv.ParseFloat(val, 64)


Why ParseFloat instead of ParseUint as above?

math.Log2 expects the input to the ParseFloat. if I do ParseUnit I need to do the conversion again. Replacing it with ParseUint and doing the internal conversion.

idryomov · 2022-05-25T10:59:25Z

internal/rbd/rbd_util.go

+			return fmt.Errorf("failed to parse objectSize %s: %w", val, err)
+		}
+		size := math.Log2(objSize)
+		ri.ObjectSize = uint64(size)


If you want to do this here, than the field should be named Order. But I would stick to ObjectSize (i.e. the object size in bytes) and convert to order only when setting the actual option on image creation/cloning.

idryomov · 2022-05-25T11:01:24Z

internal/rbd/controllerserver.go

+
+	if stripeUnit == "" && stripeCount != "" {
+		return errors.New("stripeUnit must be specified when stripeCount is specified")
+	}


Additional validation could be performed here on objectSize: it should be a whole power of 2, as you are computing the log later (rounding the log up or down is not acceptable).

Madhu-1 · 2022-05-26T06:06:11Z

@idryomov comments are addressed PTAL.

idryomov

A lot more validation could be done in validateStriping, but it would be repeating the same in librbd.

Madhu-1 · 2022-05-26T11:26:16Z

A lot more validation could be done in validateStriping, but it would be repeating the same in librbd.

Thanks, @idryomov, yes I left more validation for librbd to avoid repetition.

Madhu-1 · 2022-05-30T06:59:18Z

@nixpanic @humblec PTAL

mergify · 2022-06-09T10:39:11Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot · 2022-06-09T11:34:48Z

/retest ci/centos/mini-e2e-helm/k8s-1.22

ceph-csi-bot · 2022-06-09T11:34:48Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.22" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T11:34:49Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-06-09T11:34:50Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T11:34:50Z

@Mergifyio requeue

mergify · 2022-06-09T11:35:20Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot · 2022-06-09T12:47:33Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-06-09T12:47:34Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T12:47:35Z

/retest ci/centos/mini-e2e-helm/k8s-1.22

ceph-csi-bot · 2022-06-09T12:47:35Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.22" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T12:47:36Z

@Mergifyio requeue

mergify · 2022-06-09T12:48:05Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot · 2022-06-09T14:35:12Z

/retest ci/centos/mini-e2e-helm/k8s-1.22

ceph-csi-bot · 2022-06-09T14:35:13Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.22" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T14:35:14Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-06-09T14:35:15Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T14:35:16Z

@Mergifyio requeue

mergify · 2022-06-09T14:35:31Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

Madhu-1 · 2022-06-09T14:36:23Z

@Mergifyio rebase

RBD supports creating rbd images with object size, stripe unit and stripe count to support striping. This PR adds the support for the same. More details about striping at https://docs.ceph.com/en/quincy/man/8/rbd/#striping fixes: ceph#3124 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

mergify · 2022-06-09T14:36:52Z

rebase

✅ Branch has been successfully rebased

ceph-csi-bot · 2022-06-09T16:44:56Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-06-09T16:44:57Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-06-09T16:44:57Z

@Mergifyio requeue

mergify · 2022-06-09T16:45:28Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

mergify bot added the component/rbd Issues related to RBD label May 24, 2022

Madhu-1 force-pushed the fix-3124 branch from 2597f68 to 3a289cd Compare May 24, 2022 08:31

idryomov requested changes May 24, 2022

View reviewed changes

Rakshith-R reviewed May 24, 2022

View reviewed changes

Madhu-1 force-pushed the fix-3124 branch 2 times, most recently from c986cc1 to 3d75a36 Compare May 24, 2022 10:13

Madhu-1 force-pushed the fix-3124 branch from 3d75a36 to 40e87be Compare May 24, 2022 10:19

Madhu-1 requested a review from Rakshith-R May 24, 2022 10:22

yati1998 requested changes May 24, 2022

View reviewed changes

Madhu-1 force-pushed the fix-3124 branch from 40e87be to 317e632 Compare May 24, 2022 10:30

Madhu-1 requested a review from yati1998 May 24, 2022 10:30

yati1998 reviewed May 24, 2022

View reviewed changes

Madhu-1 force-pushed the fix-3124 branch 3 times, most recently from bb09404 to e1b64bf Compare May 25, 2022 09:05

Madhu-1 force-pushed the fix-3124 branch 2 times, most recently from 187746c to 2df170d Compare May 25, 2022 09:06

idryomov requested changes May 25, 2022

View reviewed changes

Madhu-1 force-pushed the fix-3124 branch 2 times, most recently from 4bf656d to b2a7c1d Compare May 25, 2022 14:39

Madhu-1 requested a review from a team May 26, 2022 06:06

idryomov approved these changes May 26, 2022

View reviewed changes

Madhu-1 requested a review from yati1998 May 30, 2022 06:58

Madhu-1 requested a review from humblec May 30, 2022 06:59

Madhu-1 force-pushed the fix-3124 branch from b5ff581 to d5e996a Compare June 9, 2022 14:36

mergify bot merged commit 4b57cc3 into ceph:devel Jun 9, 2022

travisn mentioned this pull request Jul 18, 2022

Enable striping in rook rook/rook#1960

Closed

KingJ mentioned this pull request Nov 5, 2023

helm: Allow templating of RBD striping parameters #4229

Merged

6 tasks

rbd: add support for rbd striping #3132

rbd: add support for rbd striping #3132

Conversation

Madhu-1 commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idryomov May 24, 2022 • edited Loading

Choose a reason for hiding this comment

idryomov May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yati1998 left a comment

Choose a reason for hiding this comment

Madhu-1 commented May 25, 2022

mergify bot commented May 25, 2022

✅ Branch has been successfully rebased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented May 26, 2022

idryomov left a comment

Choose a reason for hiding this comment

Madhu-1 commented May 26, 2022

Madhu-1 commented May 30, 2022

mergify bot commented Jun 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

mergify bot commented Jun 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

mergify bot commented Jun 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

mergify bot commented Jun 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

Madhu-1 commented Jun 9, 2022

mergify bot commented Jun 9, 2022

✅ Branch has been successfully rebased

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

ceph-csi-bot commented Jun 9, 2022

mergify bot commented Jun 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

idryomov May 24, 2022 •

edited

Loading

idryomov May 24, 2022 •

edited

Loading