diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml index 90c09dcbb..5f404a10e 100644 --- a/charts/ceph-csi-rbd/values.yaml +++ b/charts/ceph-csi-rbd/values.yaml @@ -279,7 +279,7 @@ storageClass: # eg: pool: replicapool pool: replicapool - # (required) RBD image features, CSI creates image with image-format 2 CSI + # (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. diff --git a/docs/deploy-rbd.md b/docs/deploy-rbd.md index a800a7367..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`, `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. | +| `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/e2e/rbd.go b/e2e/rbd.go index ccce3093d..fa9020025 100644 --- a/e2e/rbd.go +++ b/e2e/rbd.go @@ -852,6 +852,65 @@ var _ = Describe("RBD", func() { 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 4777b2545..52bcdc176 100644 --- a/e2e/rbd_helper.go +++ b/e2e/rbd_helper.go @@ -149,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/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml index dd858e861..b38c27c2f 100644 --- a/examples/rbd/storageclass.yaml +++ b/examples/rbd/storageclass.yaml @@ -29,7 +29,7 @@ parameters: # eg: pool: rbdpool pool: - # (required) RBD image features, CSI creates image with image-format 2 CSI + # (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. 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 f515d12ce..562d22815 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -471,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 diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go index 16820b8c7..8ab310da0 100644 --- a/internal/rbd/rbd_util_test.go +++ b/internal/rbd/rbd_util_test.go @@ -354,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) + } + }) + } +}