rbd: update image features #2885

Madhu-1 · 2022-02-17T06:58:58Z

Add support for deep-flatten image feature

As deep-flatten is long supported in ceph and it's enabled by default in the librbd, providing an option to enable it in cephcsi for the rbd images we are creating.

Make image features as an optional parameter

Makes the rbd images features in the storageclass as optional so that default image features of librbd can be used. and also kept the option for users to specify the image features in the storageclass.

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

Madhu-1 · 2022-02-17T07:00:28Z

@dryomov PTAL

Madhu-1 · 2022-02-17T10:36:50Z

@idryomov PTAL

Madhu-1 · 2022-02-17T10:42:41Z

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

Madhu-1 · 2022-02-17T10:48:58Z

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

Failure in deployment is not caused by this PR. #2886 should fix it.

nixpanic · 2022-02-28T07:37:38Z

internal/rbd/nodeserver.go

@@ -154,6 +154,7 @@ func populateRbdVol(
 	volID := req.GetVolumeId()
 	isBlock := req.GetVolumeCapability().GetBlock() != nil
 	disableInUseChecks := false
+	rv := &rbdVolume{}


Why create an empty rbdVolume, and not assign nil to it? This changes the return values in error cases. Returning nil has my preference, as using the volume after an error is prevented that way. (Defined behaviour, panic, instead of undefined behaviour with empty values.)

This is to the proper cleanup of connection we are already doing it in other places also

ceph-csi/internal/rbd/rbd_util.go

Line 1048 in eb40fbc

func GenVolFromVolID(

if we don't want to do it, i need to handle Destroy in defer in the called function.

nixpanic · 2022-02-28T07:41:52Z

internal/rbd/nodeserver.go

 	rv, err := populateRbdVol(ctx, req, cr, req.GetSecrets())
+	defer rv.Destroy()


This seems to be the only caller, thanks for adding the .Destroy(). Maybe only do this when no error occurred and do not return a (potential) incomplete rv, that will keep the function cleaner to use (or document the behaviour with a clear warning?).

done. Do internal cleanup in case of error and caller will Destroy when no error is returned.

as deep-flatten is long supported in ceph and its enabled by default in the librbd, providing an option to enable it in cephcsi for the rbd images we are creating. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

nixpanic · 2022-02-28T09:12:24Z

internal/rbd/nodeserver.go

+	}
+	// in case of any error call Destroy for cleanup.
+	defer func() {
+		if err != nil {


On line 243 there is no err, but an error is returned never the less. Destroy() will not be called in that case. Better create an error instance there, log it, and return it.

Yes, correct. update err instance should be fine now.

Makes the rbd images features in the storageclass as optional so that default image features of librbd can be used. and also kept the option to user to specify the image features in the storageclass. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Rakshith-R

lgtm

Madhu-1 · 2022-02-28T11:28:29Z

/retest ci/centos/k8s-e2e-external-storage/1.21

Madhu-1 · 2022-02-28T11:28:41Z

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

Madhu-1 · 2022-02-28T11:28:48Z

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

Madhu-1 · 2022-02-28T11:29:23Z

/retest ci/centos/k8s-e2e-external-storage/1.21

Error from server (InternalError): error when creating "https://raw.githubusercontent.com/rook/rook/v1.8.2/deploy/examples/common.yaml": Internal error occurred: resource quota evaluation timed out

Madhu-1 · 2022-02-28T11:30:34Z

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

known issue

Feb 28 10:22:45.847: INFO: ExecWithOptions: execute(POST https://192.168.39.39:8443/api/v1/namespaces/rook-ceph/pods/rook-ceph-tools-6f56cdd85d-2wlhf/exec?command=%2Fbin%2Fsh&command=-c&command=rbd+ls+--format%3Djson+--pool%3Dreplicapool&container=rook-ceph-tools&container=rook-ceph-tools&stderr=true&stdout=true %!s(MISSING))
Feb 28 10:22:46.242: FAIL: backend images not matching kubernetes resource count,image count 22 kubernetes resource count 21
backend image Info:
 [csi-vol-45ad5df3-9880-11ec-9a38-ea1ae0eb97cb csi-vol-47a1dfd4-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-47bfed6f-9880-11ec-9a38-ea1ae0eb97cb csi-vol-47bfed6f-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-47de6588-9880-11ec-9a38-ea1ae0eb97cb csi-vol-47de6588-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-47fcfaaf-9880-11ec-9a38-ea1ae0eb97cb csi-vol-47fcfaaf-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-481bb98b-9880-11ec-9a38-ea1ae0eb97cb csi-vol-481bb98b-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-483a0d0e-9880-11ec-9a38-ea1ae0eb97cb csi-vol-483a0d0e-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-4aee7e8c-9880-11ec-9a38-ea1ae0eb97cb csi-vol-4aee7e8c-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-4b2be115-9880-11ec-9a38-ea1ae0eb97cb csi-vol-4b2be115-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-4c279433-9880-11ec-9a38-ea1ae0eb97cb csi-vol-4c279433-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-4cf4484f-9880-11ec-9a38-ea1ae0eb97cb csi-vol-4cf4484f-9880-11ec-9a38-ea1ae0eb97cb-temp csi-vol-50559fa6-9880-11ec-9a38-ea1ae0eb97cb csi-vol-50559fa6-9880-11ec-9a38-ea1ae0eb97cb-temp]

Madhu-1 · 2022-02-28T11:33:27Z

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

known issue

eb 28 10:02:48.373: FAIL: backend images not matching kubernetes resource count,image count 22 kubernetes resource count 21
backend image Info:
 [csi-vol-6ae1aea3-987d-11ec-af69-e6034101a69c csi-vol-7845bb0d-987d-11ec-af69-e6034101a69c csi-vol-7845bb0d-987d-11ec-af69-e6034101a69c-temp csi-vol-785dbe0d-987d-11ec-af69-e6034101a69c csi-vol-785dbe0d-987d-11ec-af69-e6034101a69c-temp csi-vol-789db9d5-987d-11ec-af69-e6034101a69c-temp csi-vol-78b9260c-987d-11ec-af69-e6034101a69c csi-vol-78b9260c-987d-11ec-af69-e6034101a69c-temp csi-vol-79c8cb13-987d-11ec-af69-e6034101a69c csi-vol-79c8cb13-987d-11ec-af69-e6034101a69c-temp csi-vol-79f2e321-987d-11ec-af69-e6034101a69c csi-vol-79f2e321-987d-11ec-af69-e6034101a69c-temp csi-vol-7e2d5e9b-987d-11ec-af69-e6034101a69c csi-vol-7e2d5e9b-987d-11ec-af69-e6034101a69c-temp csi-vol-7e33fad0-987d-11ec-af69-e6034101a69c csi-vol-7e33fad0-987d-11ec-af69-e6034101a69c-temp csi-vol-7e7f5bf5-987d-11ec-af69-e6034101a69c csi-vol-7e7f5bf5-987d-11ec-af69-e6034101a69c-temp csi-vol-7e9d1847-987d-11ec-af69-e6034101a69c csi-vol-7e9d1847-987d-11ec-af69-e6034101a69c-temp csi-vol-83ca8ea2-987d-11ec-af69-e6034101a69c csi-vol-83ca8ea2-987d-11ec-af69-e6034101a69c-temp]

idryomov

A bit late to the party, but I thought I'd comment anyway since this was sitting in one of many open tabs...

idryomov · 2022-02-17T10:49:06Z

charts/ceph-csi-rbd/values.yaml

+  # (optional) RBD image features, CSI creates image with image-format 2 CSI
+  # RBD currently supports `layering`, `journaling`, `exclusive-lock`,
+  # `object-map`, `fast-diff`, `deep-flatten` features. If `journaling` is
+  # enabled, must enable `exclusive-lock` too.


object-map and fast-diff also require exclusive-lock

fast-diff requires object-map

I'd avoid trying to spell out image feature dependencies here and just link to https://docs.ceph.com/en/latest/rbd/rbd-config-ref/#image-features.

#2909 should fix this one.

idryomov · 2022-02-28T13:45:31Z

examples/rbd/storageclass.yaml

+   # (required) RBD image features, CSI creates image with image-format 2 CSI
+   # RBD currently supports `layering`, `journaling`, `exclusive-lock`,
+   # `object-map`, `fast-diff`, `deep-flatten` features. If `journaling` is
+   # enabled, must enable `exclusive-lock` too.


idryomov · 2022-02-28T13:48:35Z

docs/deploy-rbd.md

@@ -56,7 +56,7 @@ make image-cephcsi
 | `dataPool`                                                                                          | no                   | Ceph pool used for the data of the RBD images.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
 | `volumeNamePrefix`                                                                                  | no                   | Prefix to use for naming RBD images (defaults to `csi-vol-`).                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |
 | `snapshotNamePrefix`                                                                                | no                   | Prefix to use for naming RBD snapshot images (defaults to `csi-snap-`).                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
-| `imageFeatures`                                                                                     | yes                  | RBD image features. CSI RBD currently supports `layering`, `journaling`, `exclusive-lock`, `object-map`, `fast-diff` features. If `journaling` is enabled, must enable `exclusive-lock` too. See [man pages](http://docs.ceph.com/docs/master/man/8/rbd/#cmdoption-rbd-image-feature) Note that the required support for [object-map and fast-diff were added in 5.3 and journaling does not have KRBD support yet](https://docs.ceph.com/en/latest/rbd/rbd-config-ref/#image-features). deep-flatten is added for cloned images. |
+| `imageFeatures`                                                                                     | no                  | RBD image features. CSI RBD currently supports `layering`, `journaling`, `exclusive-lock`, `object-map`, `fast-diff`, `deep-flatten` features. If `journaling` is enabled, must enable `exclusive-lock` too. See [man pages](http://docs.ceph.com/docs/master/man/8/rbd/#cmdoption-rbd-image-feature) Note that the required support for [object-map and fast-diff were added in 5.3, deep-flatten was added in 5.1 and journaling does not have KRBD support yet](https://docs.ceph.com/en/latest/rbd/rbd-config-ref/#image-features). deep-flatten is added for cloned images. |


idryomov · 2022-02-28T13:55:18Z

internal/rbd/rbd_util.go

+// checkValidImageFeatures check presence of imageFeatures parameter. It returns false when
+// there imageFeatures is present and empty.
+func checkValidImageFeatures(imageFeatures string, ok bool) bool {
+	return !(ok && imageFeatures == "")


Technically, on the RBD side, an empty image features string is valid and means "create an image with no features enabled". However disallowing it in the CSI layer is probably fine -- no one should be doing that these days.

Yes, correct. this is extra validation to make sure the key imageFeature set in the storageclass is not empty as its optional user can choose to not set this key and value or set both.

mergify bot added the component/rbd Issues related to RBD label Feb 17, 2022

Madhu-1 requested review from a team February 17, 2022 07:00

Madhu-1 requested a review from ShyamsundarR February 17, 2022 07:00

Madhu-1 mentioned this pull request Feb 17, 2022

update imagefeature in rbd storageclass red-hat-storage/ocs-operator#1512

Merged

Madhu-1 force-pushed the default-image-features branch 2 times, most recently from 051b802 to d9d0e64 Compare February 17, 2022 08:43

nixpanic reviewed Feb 28, 2022

View reviewed changes

nixpanic requested changes Feb 28, 2022

View reviewed changes

rbd: add support for deep-flatten image feature

4fe1878

as deep-flatten is long supported in ceph and its enabled by default in the librbd, providing an option to enable it in cephcsi for the rbd images we are creating. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Madhu-1 force-pushed the default-image-features branch from d9d0e64 to 3741430 Compare February 28, 2022 07:48

Madhu-1 requested a review from nixpanic February 28, 2022 07:49

nixpanic requested changes Feb 28, 2022

View reviewed changes

Madhu-1 force-pushed the default-image-features branch from 3741430 to 2daf456 Compare February 28, 2022 09:22

Madhu-1 requested a review from nixpanic February 28, 2022 09:22

nixpanic approved these changes Feb 28, 2022

View reviewed changes

Madhu-1 requested a review from a team February 28, 2022 09:31

Rakshith-R approved these changes Feb 28, 2022

View reviewed changes

Madhu-1 added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Feb 28, 2022

mergify bot merged commit d5c98f8 into ceph:devel Feb 28, 2022

Madhu-1 mentioned this pull request Feb 28, 2022

BUG 2059080: rbd update image features red-hat-storage/ceph-csi#81

Merged

idryomov reviewed Feb 28, 2022

View reviewed changes

Madhu-1 mentioned this pull request Mar 1, 2022

doc: remove mention of image feature dependency #2909

Merged

rbd: update image features #2885

rbd: update image features #2885

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

nixpanic Feb 28, 2022

Madhu-1 Feb 28, 2022

nixpanic Feb 28, 2022

Madhu-1 Feb 28, 2022

nixpanic Feb 28, 2022

Madhu-1 Feb 28, 2022

Rakshith-R left a comment

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

idryomov left a comment

idryomov Feb 17, 2022

Madhu-1 Mar 1, 2022

idryomov Feb 28, 2022

idryomov Feb 28, 2022

idryomov Feb 28, 2022

Madhu-1 Mar 1, 2022

		rv, err := populateRbdVol(ctx, req, cr, req.GetSecrets())
		defer rv.Destroy()

rbd: update image features #2885

rbd: update image features #2885

Conversation

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 2022

Madhu-1 commented Feb 17, 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

Choose a reason for hiding this comment

Rakshith-R left a comment

Choose a reason for hiding this comment

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

Madhu-1 commented Feb 28, 2022

idryomov left a 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