Repository: ceph/ceph-csi
Pull request #2885 (2022-02-17 06:58:58)

rbd: update image features
* 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: @dryomov PTAL
Madhu-1: @idryomov PTAL
Madhu-1: /retest ci/centos/mini-e2e-helm/k8s-1.22
Madhu-1: > /retest ci/centos/mini-e2e-helm/k8s-1.22

Failure in deployment is not caused by this PR. https://github.com/ceph/ceph-csi/pull/2886 should fix it.
Madhu-1: /retest ci/centos/k8s-e2e-external-storage/1.21
Madhu-1: /retest ci/centos/mini-e2e-helm/k8s-1.22
Madhu-1: /retest ci/centos/mini-e2e-helm/k8s-1.23
Madhu-1: > /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: > /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: > /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]
```

Patch:
diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml
index 81d6df623..5f404a10e 100644
--- a/charts/ceph-csi-rbd/values.yaml
+++ b/charts/ceph-csi-rbd/values.yaml
@@ -279,10 +279,10 @@ storageClass:
   # eg: pool: replicapool
   pool: replicapool
 
-  # (required) RBD image features, CSI creates image with image-format 2
-  # CSI RBD currently supports `layering`, `journaling`, `exclusive-lock`,
-  # `object-map`, `fast-diff` features. If `journaling` is enabled, must
-  # enable `exclusive-lock` too.
+  # (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.
   # imageFeatures: layering,journaling,exclusive-lock,object-map,fast-diff
   imageFeatures: "layering"
 
diff --git a/docs/deploy-rbd.md b/docs/deploy-rbd.md
index bdb9d30b1..4afbe71fc 100644
--- a/docs/deploy-rbd.md
+++ b/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. |
 | `tryOtherMounters`                                                                                  | no                   | Specifies whether to try other mounters in case if the current mounter fails to mount the rbd image for any reason                                                                                                                                                                                                                                                                                                                                                                                                                |
 | `mapOptions`                                                                                        | no                   | Map options to use when mapping rbd image. See [krbd](https://docs.ceph.com/docs/master/man/8/rbd/#kernel-rbd-krbd-options) and [nbd](https://docs.ceph.com/docs/master/man/8/rbd-nbd/#options) options.                                                                                                                                                                                                                                                                                                                          |
 | `unmapOptions`                                                                                      | no                   | Unmap options to use when unmapping rbd image. See [krbd](https://docs.ceph.com/docs/master/man/8/rbd/#kernel-rbd-krbd-options) and [nbd](https://docs.ceph.com/docs/master/man/8/rbd-nbd/#options) options.                                                                                                                                                                                                                                                                                                                      |
diff --git a/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml
index 2d6e0927c..b38c27c2f 100644
--- a/examples/rbd/storageclass.yaml
+++ b/examples/rbd/storageclass.yaml
@@ -29,10 +29,10 @@ parameters:
    # eg: pool: rbdpool
    pool: <rbd-pool-name>
 
-   # (required) RBD image features, CSI creates image with image-format 2
-   # CSI RBD currently supports `layering`, `journaling`, `exclusive-lock`,
-   # `object-map`, `fast-diff` features. If `journaling` is enabled, must
-   # enable `exclusive-lock` too.
+   # (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.
    # imageFeatures: layering,journaling,exclusive-lock,object-map,fast-diff
    imageFeatures: "layering"
 
diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
index f1b6d1c57..c6451dd92 100644
--- a/internal/rbd/controllerserver.go
+++ b/internal/rbd/controllerserver.go
@@ -115,8 +115,8 @@ func (cs *ControllerServer) parseVolCreateRequest(
 			"multi node access modes are only supported on rbd `block` type volumes")
 	}
 
-	if imageFeatures, ok := req.GetParameters()["imageFeatures"]; checkImageFeatures(imageFeatures, ok, true) {
-		return nil, status.Error(codes.InvalidArgument, "missing required parameter imageFeatures")
+	if imageFeatures, ok := req.GetParameters()["imageFeatures"]; !checkValidImageFeatures(imageFeatures, ok) {
+		return nil, status.Error(codes.InvalidArgument, "empty imageFeatures parameter")
 	}
 
 	// if it's NOT SINGLE_NODE_WRITER, and it's BLOCK we'll set the parameter to ignore the in-use checks
diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go
index a1029f9ce..affe42fab 100644
--- a/internal/rbd/nodeserver.go
+++ b/internal/rbd/nodeserver.go
@@ -215,12 +215,33 @@ func populateRbdVol(
 		rv.RbdImageName = imageAttributes.ImageName
 	}
 
+	err = rv.Connect(cr)
+	if err != nil {
+		log.ErrorLog(ctx, "failed to connect to volume %s: %v", rv, err)
+
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+	// in case of any error call Destroy for cleanup.
+	defer func() {
+		if err != nil {
+			rv.Destroy()
+		}
+	}()
+	// get the image details from the ceph cluster.
+	err = rv.getImageInfo()
+	if err != nil {
+		log.ErrorLog(ctx, "failed to get image details %s: %v", rv, err)
+
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
 	if req.GetVolumeContext()["mounter"] == rbdDefaultMounter &&
-		!isKrbdFeatureSupported(ctx, req.GetVolumeContext()["imageFeatures"]) {
+		!isKrbdFeatureSupported(ctx, strings.Join(rv.ImageFeatureSet.Names(), ",")) {
 		if !parseBoolOption(ctx, req.GetVolumeContext(), tryOtherMounters, false) {
 			log.ErrorLog(ctx, "unsupported krbd Feature, set `tryOtherMounters:true` or fix krbd driver")
+			err = errors.New("unsupported krbd Feature")
 
-			return nil, status.Errorf(codes.Internal, "unsupported krbd Feature")
+			return nil, status.Error(codes.Internal, err.Error())
 		}
 		// fallback to rbd-nbd,
 		rv.Mounter = rbdNbdMounter
@@ -299,24 +320,10 @@ func (ns *NodeServer) NodeStageVolume(
 	}
 
 	isStaticVol := parseBoolOption(ctx, req.GetVolumeContext(), staticVol, false)
-
-	// throw error when imageFeatures parameter is missing or empty
-	// for backward compatibility, ignore error for non-static volumes from older cephcsi version
-	if imageFeatures, ok := req.GetVolumeContext()["imageFeatures"]; checkImageFeatures(imageFeatures, ok, isStaticVol) {
-		return nil, status.Error(codes.InvalidArgument, "missing required parameter imageFeatures")
-	}
-
 	rv, err := populateRbdVol(ctx, req, cr, req.GetSecrets())
 	if err != nil {
 		return nil, err
 	}
-
-	err = rv.Connect(cr)
-	if err != nil {
-		log.ErrorLog(ctx, "failed to connect to volume %s: %v", rv, err)
-
-		return nil, status.Error(codes.Internal, err.Error())
-	}
 	defer rv.Destroy()
 
 	if isHealer {
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index 71eb98685..562d22815 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -209,6 +209,9 @@ var supportedFeatures = map[string]imageFeature{
 		needRbdNbd: true,
 		dependsOn:  []string{librbd.FeatureNameExclusiveLock},
 	},
+	librbd.FeatureNameDeepFlatten: {
+		needRbdNbd: false,
+	},
 }
 
 // GetKrbdSupportedFeatures load the module if needed and return supported
@@ -468,11 +471,10 @@ func (ri *rbdImage) isInUse() (bool, error) {
 	return len(watchers) > defaultWatchers, nil
 }
 
-// checkImageFeatures check presence of imageFeatures parameter. It returns true when
-// there imageFeatures is missing or empty, skips missing parameter for non-static volumes
-// for backward compatibility.
-func checkImageFeatures(imageFeatures string, ok, static bool) bool {
-	return static && (!ok || imageFeatures == "")
+// 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 == "")
 }
 
 // isNotMountPoint checks whether MountPoint does not exists and


Test patch:
diff --git a/e2e/rbd.go b/e2e/rbd.go
index 2a071ead7..fa9020025 100644
--- a/e2e/rbd.go
+++ b/e2e/rbd.go
@@ -795,6 +795,122 @@ var _ = Describe("RBD", func() {
 					}
 				})
 
+			By("create PVC with layering,deep-flatten image-features and bind it to an app",
+				func() {
+					err := deleteResource(rbdExamplePath + "storageclass.yaml")
+					if err != nil {
+						e2elog.Failf("failed to delete storageclass: %v", err)
+					}
+					err = createRBDStorageClass(
+						f.ClientSet,
+						f,
+						defaultSCName,
+						nil,
+						map[string]string{
+							"imageFeatures": "layering,deep-flatten",
+						},
+						deletePolicy)
+					if err != nil {
+						e2elog.Failf("failed to create storageclass: %v", err)
+					}
+					// set up PVC
+					pvc, err := loadPVC(pvcPath)
+					if err != nil {
+						e2elog.Failf("failed to load PVC: %v", err)
+					}
+					pvc.Namespace = f.UniqueName
+					err = createPVCAndvalidatePV(f.ClientSet, pvc, deployTimeout)
+					if err != nil {
+						e2elog.Failf("failed to create PVC: %v", err)
+					}
+					// validate created backend rbd images
+					validateRBDImageCount(f, 1, defaultRBDPool)
+
+					if util.CheckKernelSupport(kernelRelease, deepFlattenSupport) {
+						app, aErr := loadApp(appPath)
+						if aErr != nil {
+							e2elog.Failf("failed to load application: %v", aErr)
+						}
+						app.Namespace = f.UniqueName
+						err = createApp(f.ClientSet, app, deployTimeout)
+						if err != nil {
+							e2elog.Failf("failed to create application: %v", err)
+						}
+						// delete pod as we should not create snapshot for in-use pvc
+						err = deletePod(app.Name, app.Namespace, f.ClientSet, deployTimeout)
+						if err != nil {
+							e2elog.Failf("failed to delete application: %v", err)
+						}
+
+					}
+					// clean up after ourselves
+					err = deletePVCAndValidatePV(f.ClientSet, pvc, deployTimeout)
+					if err != nil {
+						e2elog.Failf("failed to delete PVC: %v", err)
+					}
+					// validate created backend rbd images
+					validateRBDImageCount(f, 0, defaultRBDPool)
+				})
+
+			By("create PVC with layering,deep-flatten image-features and bind it to an app",
+				func() {
+					err := deleteResource(rbdExamplePath + "storageclass.yaml")
+					if err != nil {
+						e2elog.Failf("failed to delete storageclass: %v", err)
+					}
+					err = createRBDStorageClass(
+						f.ClientSet,
+						f,
+						defaultSCName,
+						nil,
+						map[string]string{
+							"imageFeatures": "",
+						},
+						deletePolicy)
+					if err != nil {
+						e2elog.Failf("failed to create storageclass: %v", err)
+					}
+					// set up PVC
+					pvc, err := loadPVC(pvcPath)
+					if err != nil {
+						e2elog.Failf("failed to load PVC: %v", err)
+					}
+					pvc.Namespace = f.UniqueName
+					err = createPVCAndvalidatePV(f.ClientSet, pvc, deployTimeout)
+					if err != nil {
+						e2elog.Failf("failed to create PVC: %v", err)
+					}
+					// validate created backend rbd images
+					validateRBDImageCount(f, 1, defaultRBDPool)
+
+					// checking the minimal kernel version for fast-diff as its
+					// higher kernel version than other default image features.
+					if util.CheckKernelSupport(kernelRelease, fastDiffSupport) {
+						app, aErr := loadApp(appPath)
+						if aErr != nil {
+							e2elog.Failf("failed to load application: %v", aErr)
+						}
+						app.Namespace = f.UniqueName
+						err = createApp(f.ClientSet, app, deployTimeout)
+						if err != nil {
+							e2elog.Failf("failed to create application: %v", err)
+						}
+						// delete pod as we should not create snapshot for in-use pvc
+						err = deletePod(app.Name, app.Namespace, f.ClientSet, deployTimeout)
+						if err != nil {
+							e2elog.Failf("failed to delete application: %v", err)
+						}
+
+					}
+					// clean up after ourselves
+					err = deletePVCAndValidatePV(f.ClientSet, pvc, deployTimeout)
+					if err != nil {
+						e2elog.Failf("failed to delete PVC: %v", err)
+					}
+					// validate created backend rbd images
+					validateRBDImageCount(f, 0, defaultRBDPool)
+				})
+
 			By("create PVC with journaling,fast-diff image-features and bind it to an app using rbd-nbd mounter",
 				func() {
 					if util.CheckKernelSupport(kernelRelease, fastDiffSupport) {
diff --git a/e2e/rbd_helper.go b/e2e/rbd_helper.go
index 290eab933..52bcdc176 100644
--- a/e2e/rbd_helper.go
+++ b/e2e/rbd_helper.go
@@ -62,6 +62,18 @@ var fastDiffSupport = []util.KernelVersion{
 	}, // standard 5.3+ versions
 }
 
+// nolint:gomnd // numbers specify Kernel versions.
+var deepFlattenSupport = []util.KernelVersion{
+	{
+		Version:      5,
+		PatchLevel:   1,
+		SubLevel:     0,
+		ExtraVersion: 0,
+		Distribution: "",
+		Backport:     false,
+	}, // standard 5.1+ versions
+}
+
 // To use `io-timeout=0` we need
 // www.mail-archive.com/linux-block@vger.kernel.org/msg38060.html
 // nolint:gomnd // numbers specify Kernel versions.
@@ -137,6 +149,10 @@ func createRBDStorageClass(
 	sc.Parameters["clusterID"] = fsID
 	for k, v := range parameters {
 		sc.Parameters[k] = v
+		// if any values are empty remove it from the map
+		if v == "" {
+			delete(sc.Parameters, k)
+		}
 	}
 	sc.Namespace = cephCSINamespace
 
diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go
index 6007106ba..8ab310da0 100644
--- a/internal/rbd/rbd_util_test.go
+++ b/internal/rbd/rbd_util_test.go
@@ -152,6 +152,14 @@ func TestValidateImageFeatures(t *testing.T) {
 			true,
 			"invalid feature ayering",
 		},
+		{
+			"deep-flatten",
+			&rbdVolume{
+				Mounter: rbdDefaultMounter,
+			},
+			false,
+			"",
+		},
 	}
 
 	for _, test := range tests {
@@ -346,3 +354,35 @@ func TestIsKrbdFeatureSupported(t *testing.T) {
 		})
 	}
 }
+
+func Test_checkValidImageFeatures(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name          string
+		imageFeatures string
+		ok            bool
+		want          bool
+	}{
+		{
+			name:          "test for valid image features",
+			imageFeatures: "layering,exclusive-lock,object-map,fast-diff,deep-flatten",
+			ok:            true,
+			want:          true,
+		},
+		{
+			name:          "test for empty image features",
+			imageFeatures: "",
+			ok:            true,
+			want:          false,
+		},
+	}
+	for _, tt := range tests {
+		tc := tt
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			if got := checkValidImageFeatures(tc.imageFeatures, tc.ok); got != tc.want {
+				t.Errorf("checkValidImageFeatures() = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}