Repository: ceph/ceph-csi
Pull request #1325 (2020-08-06 08:00:21)

rbd: add exclusive-lock and journaling image features for rbd image
The current rbd only supports the layering feature for the rbd image. Add exclusive-lock and journaling image features for the rbd.

Signed-off-by: woohhan <woohyung_han@tmax.co.kr>
Co-authored-by: Madhu Rajanna <madhupr007@gmail.com>

This will abandon the original PR
Closes: #684
updates  #676

Madhu-1: added not-ready-to-review so that I need to test it out first.
Madhu-1: @nixpanic any idea how to load `nbd` modules to minikube VM?
Madhu-1: /retest ci/centos/mini-e2e-helm/k8s-1.18.5
humblec: @Madhu-1 as discussed deferring this to 3.2.0 :+1: 
nixpanic: /retest ci/centos/mini-e2e
nixpanic: /retest ci/centos/mini-e2e-helm
Madhu-1: @humblec @nixpanic PTAL
Madhu-1: /test all
Madhu-1: @mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

mergify[bot]: This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏
nixpanic: @Madhu-1, this needs a rebase...
stale[bot]: This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in a month if no further activity occurs. Thank you for your contributions.

salanki: Is it time to merge this now that kernel support is prevalent?
Madhu-1: @humblec @nixpanic PTAL
humblec: @Madhu-1 looks like other PR merge cause a conflict.
Madhu-1: > @Madhu-1 looks like other PR merge cause a conflict.

@humblec looks like we merged the Newly created PR first. That would cause a lot of issues(resolve merge conflict and rebase) to the PR's which are pending review for a long time.
humblec: > > @Madhu-1 looks like other PR merge cause a conflict.
> 
> @humblec looks like we merged the Newly created PR first. That would cause a lot of issues(resolve merge conflict and rebase) to the PR's which are pending review for a long time.

ahh, :(.. we could have added `DNM` with that.. 
Madhu-1: > > > @Madhu-1 looks like other PR merge cause a conflict.
> > 
> > 
> > @humblec looks like we merged the Newly created PR first. That would cause a lot of issues(resolve merge conflict and rebase) to the PR's which are pending review for a long time.
> 
> ahh, :(.. we could have added `DNM` with that..

I was expecting old PR's will get reviewed first
Madhu-1: /test ci/centos/mini-e2e-helm/k8s-1.19
Madhu-1: >Warning: Permanently added 'n62.dusty.ci.centos.org,172.19.2.126' (ECDSA) to the list of known hosts.
Error from server: etcdserver: request timed out

E2E failure
humblec: @nixpanic ptal ? 
nixpanic: /retest ci/centos/upgrade-tests-cephfs
nixpanic: > /retest ci/centos/upgrade-tests-cephfs

etcdserver timeout while deploying rook

Patch:
diff --git a/docs/deploy-rbd.md b/docs/deploy-rbd.md
index 29f641058..3df9e1a72 100644
--- a/docs/deploy-rbd.md
+++ b/docs/deploy-rbd.md
@@ -48,22 +48,23 @@ make image-cephcsi
 
 **Available volume parameters:**
 
-| Parameter                                                                                           | Required             | Description                                                                                                                                                                                                             |
-| --------------------------------------------------------------------------------------------------- | -------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `clusterID`                                                                                         | yes                  | String representing a Ceph cluster, must be unique across all Ceph clusters in use for provisioning, cannot be greater than 36 bytes in length, and should remain immutable for the lifetime of the Ceph cluster in use |
-| `pool`                                                                                              | yes                  | Ceph pool into which the RBD image shall be created                                                                                                                                                                     |
-| `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 only `layering` feature. 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) which is why they are unsupported here. deep-flatten is added for cloned images. |
-| `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.            |
-| `csi.storage.k8s.io/provisioner-secret-name`, `csi.storage.k8s.io/node-stage-secret-name`           | yes (for Kubernetes) | name of the Kubernetes Secret object containing Ceph client credentials. Both parameters should have the same value                                                                                                     |
-| `csi.storage.k8s.io/provisioner-secret-namespace`, `csi.storage.k8s.io/node-stage-secret-namespace` | yes (for Kubernetes) | namespaces of the above Secret objects                                                                                                                                                                                  |
-| `mounter`                                                                                           | no                   | if set to `rbd-nbd`, use `rbd-nbd` on nodes that have `rbd-nbd` and `nbd` kernel modules to map rbd images                                                                                                              |
-| `encrypted`                                                                                         | no                   | disabled by default, use `"true"` to enable LUKS encryption on PVC and `"false"` to disable it. **Do not change for existing storageclasses**                                                                           |
-| `encryptionKMSID`                                                                                   | no                   | required if encryption is enabled and a kms is used to store passphrases                                                                                                                                                |
-| `thickProvision`                                                                                    | no                   | if set to `"true"`, newly created RBD images will be completely allocated by writing zeros to it                                                                                                                        |
+| Parameter                                                                                           | Required             | Description                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            |
+| --------------------------------------------------------------------------------------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `clusterID`                                                                                         | yes                  | String representing a Ceph cluster, must be unique across all Ceph clusters in use for provisioning, cannot be greater than 36 bytes in length, and should remain immutable for the lifetime of the Ceph cluster in use                                                                                                                                                                                                                                                                                |
+| `pool`                                                                                              | yes                  | Ceph pool into which the RBD image shall be created                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
+| `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` 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. |
+|                                                                                                     |
+| `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.                                                                                                                                                                                                                                                                                           |
+| `csi.storage.k8s.io/provisioner-secret-name`, `csi.storage.k8s.io/node-stage-secret-name`           | yes (for Kubernetes) | name of the Kubernetes Secret object containing Ceph client credentials. Both parameters should have the same value                                                                                                                                                                                                                                                                                                                                                                                    |
+| `csi.storage.k8s.io/provisioner-secret-namespace`, `csi.storage.k8s.io/node-stage-secret-namespace` | yes (for Kubernetes) | namespaces of the above Secret objects                                                                                                                                                                                                                                                                                                                                                                                                                                                                 |
+| `mounter`                                                                                           | no                   | if set to `rbd-nbd`, use `rbd-nbd` on nodes that have `rbd-nbd` and `nbd` kernel modules to map rbd images                                                                                                                                                                                                                                                                                                                                                                                             |
+| `encrypted`                                                                                         | no                   | disabled by default, use `"true"` to enable LUKS encryption on PVC and `"false"` to disable it. **Do not change for existing storageclasses**                                                                                                                                                                                                                                                                                                                                                          |
+| `encryptionKMSID`                                                                                   | no                   | required if encryption is enabled and a kms is used to store passphrases                                                                                                                                                                                                                                                                                                                                                                                                                               |
+| `thickProvision`                                                                                    | no                   | if set to `"true"`, newly created RBD images will be completely allocated by writing zeros to it                                                                                                                                                                                                                                                                                                                                                                                                       |
 
 **NOTE:** An accompanying CSI configuration file, needs to be provided to the
 running pods. Refer to [Creating CSI configuration](../examples/README.md#creating-csi-configuration)
diff --git a/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml
index 362780121..ced2eaafe 100644
--- a/examples/rbd/storageclass.yaml
+++ b/examples/rbd/storageclass.yaml
@@ -33,7 +33,9 @@ parameters:
    # creation (thin provisioning is the default).
    thickProvision: "false"
    # (required) RBD image features, CSI creates image with image-format 2
-   # CSI RBD currently supports only `layering` feature.
+   # CSI RBD currently supports `layering`, `journaling`, `exclusive-lock`
+   # features. If `journaling` is enabled, must enable `exclusive-lock` too.
+   # imageFeatures: layering,journaling,exclusive-lock
    imageFeatures: layering
 
    # (optional) mapOptions is a comma-separated list of map options.
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index f99009046..eea687db4 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -46,6 +46,7 @@ const (
 	rbdImageWatcherFactor    = 1.4
 	rbdImageWatcherSteps     = 10
 	rbdDefaultMounter        = "rbd"
+	rbdNbdMounter            = "rbd-nbd"
 
 	// Output strings returned during invocation of "ceph rbd task add remove <imagespec>" when
 	// command is not supported by ceph manager. Used to check errors and recover when the command
@@ -142,8 +143,27 @@ type rbdSnapshot struct {
 	SizeBytes      int64
 }
 
+// imageFeature represents required image features and value.
+type imageFeature struct {
+	// needRbdNbd indicates whether this image feature requires an rbd-nbd mounter
+	needRbdNbd bool
+	// dependsOn is the image features required for this imageFeature
+	dependsOn []string
+}
+
 var (
-	supportedFeatures = sets.NewString(librbd.FeatureNameLayering)
+	supportedFeatures = map[string]imageFeature{
+		librbd.FeatureNameLayering: {
+			needRbdNbd: false,
+		},
+		librbd.FeatureNameExclusiveLock: {
+			needRbdNbd: true,
+		},
+		librbd.FeatureNameJournaling: {
+			needRbdNbd: true,
+			dependsOn:  []string{librbd.FeatureNameExclusiveLock},
+		},
+	}
 )
 
 // Connect an rbdVolume to the Ceph cluster.
@@ -889,29 +909,19 @@ func genVolFromVolumeOptions(ctx context.Context, volOptions, credentials map[st
 	if err != nil {
 		return nil, err
 	}
+	if rbdVol.Mounter, ok = volOptions["mounter"]; !ok {
+		rbdVol.Mounter = rbdDefaultMounter
+	}
 	// if no image features is provided, it results in empty string
 	// which disable all RBD image features as we expected
-
-	imageFeatures, found := volOptions["imageFeatures"]
-	if found {
-		arr := strings.Split(imageFeatures, ",")
-		for _, f := range arr {
-			if !supportedFeatures.Has(f) {
-				return nil, fmt.Errorf("invalid feature %q for volume csi-rbdplugin, supported"+
-					" features are: %v", f, supportedFeatures)
-			}
-		}
-		rbdVol.imageFeatureSet = librbd.FeatureSetFromNames(arr)
+	if err = rbdVol.validateImageFeatures(volOptions["imageFeatures"]); err != nil {
+		util.ErrorLog(ctx, "failed to validate image features %v", err)
+		return nil, err
 	}
 
-	util.ExtendedLog(ctx, "setting disableInUseChecks on rbd volume to: %v", disableInUseChecks)
+	util.ExtendedLog(ctx, "setting disableInUseChecks: %t image features: %v mounter: %s", disableInUseChecks, rbdVol.imageFeatureSet.Names(), rbdVol.Mounter)
 	rbdVol.DisableInUseChecks = disableInUseChecks
 
-	rbdVol.Mounter, ok = volOptions["mounter"]
-	if !ok {
-		rbdVol.Mounter = rbdDefaultMounter
-	}
-
 	err = rbdVol.initKMS(ctx, volOptions, credentials)
 	if err != nil {
 		return nil, err
@@ -920,6 +930,29 @@ func genVolFromVolumeOptions(ctx context.Context, volOptions, credentials map[st
 	return rbdVol, nil
 }
 
+func (rv *rbdVolume) validateImageFeatures(imageFeatures string) error {
+	arr := strings.Split(imageFeatures, ",")
+	featureSet := sets.NewString(arr...)
+	for _, f := range arr {
+		sf, found := supportedFeatures[f]
+		if !found {
+			return fmt.Errorf("invalid feature %s", f)
+		}
+
+		for _, r := range sf.dependsOn {
+			if !featureSet.Has(r) {
+				return fmt.Errorf("feature %s requires %s to be set", f, r)
+			}
+		}
+
+		if sf.needRbdNbd && rv.Mounter != rbdNbdMounter {
+			return fmt.Errorf("feature %s requires rbd-nbd for mounter", f)
+		}
+	}
+	rv.imageFeatureSet = librbd.FeatureSetFromNames(arr)
+	return nil
+}
+
 func genSnapFromOptions(ctx context.Context, rbdVol *rbdVolume, snapOptions map[string]string) (*rbdSnapshot, error) {
 	var err error
 


Test patch:
diff --git a/e2e/rbd.go b/e2e/rbd.go
index f710413eb..d32606cad 100644
--- a/e2e/rbd.go
+++ b/e2e/rbd.go
@@ -467,6 +467,18 @@ var _ = Describe("RBD", func() {
 				}
 			})
 
+			// TODO: enable this test when we support rbd-nbd mounter in E2E.
+			// nbd module should be present on the host machine to run use the
+			// rbd-nbd mounter.
+
+			// By("create a PVC and Bind it to an app with journaling/exclusive-lock image-features and rbd-nbd mounter", func() {
+			// 	deleteResource(rbdExamplePath + "storageclass.yaml")
+			// 	createRBDStorageClass(f.ClientSet, f, nil, map[string]string{"imageFeatures": "layering,journaling,exclusive-lock", "mounter": "rbd-nbd"})
+			// 	validatePVCAndAppBinding(pvcPath, appPath, f)
+			// 	deleteResource(rbdExamplePath + "storageclass.yaml")
+			// 	createRBDStorageClass(f.ClientSet, f, nil, make(map[string]string))
+			// })
+
 			By("create a PVC clone and bind it to an app", func() {
 				// snapshot beta is only supported from v1.17+
 				if k8sVersionGreaterEquals(f.ClientSet, 1, 17) {
diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go
index a6637a9c5..b8122585a 100644
--- a/internal/rbd/rbd_util_test.go
+++ b/internal/rbd/rbd_util_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	librbd "github.com/ceph/go-ceph/rbd"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestHasSnapshotFeature(t *testing.T) {
@@ -42,3 +43,86 @@ func TestHasSnapshotFeature(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateImageFeatures(t *testing.T) {
+	tests := []struct {
+		imageFeatures string
+		rbdVol        *rbdVolume
+		isErr         bool
+		errMsg        string
+	}{
+		{
+			"layering",
+			&rbdVolume{
+				Mounter: rbdDefaultMounter,
+			},
+			false,
+			"",
+		},
+		{
+			"layering",
+			&rbdVolume{
+				Mounter: rbdNbdMounter,
+			},
+			false,
+			"",
+		},
+		{
+			"layering,exclusive-lock,journaling",
+			&rbdVolume{
+				Mounter: rbdNbdMounter,
+			},
+			false,
+			"",
+		},
+		{
+			"layering,journaling",
+			&rbdVolume{
+				Mounter: rbdNbdMounter,
+			},
+			true,
+			"feature journaling requires exclusive-lock to be set",
+		},
+		{
+			"layering,exclusive-lock,journaling",
+			&rbdVolume{
+				Mounter: rbdDefaultMounter,
+			},
+			true,
+			"feature exclusive-lock requires rbd-nbd for mounter",
+		},
+		{
+			"layering,exclusive-lock,journaling",
+			&rbdVolume{
+				Mounter: rbdDefaultMounter,
+			},
+			true,
+			"feature exclusive-lock requires rbd-nbd for mounter",
+		},
+		{
+			"layering,exclusive-loc,journaling",
+			&rbdVolume{
+				Mounter: rbdNbdMounter,
+			},
+			true,
+			"invalid feature exclusive-loc",
+		},
+		{
+			"ayering",
+			&rbdVolume{
+				Mounter: rbdDefaultMounter,
+			},
+			true,
+			"invalid feature ayering",
+		},
+	}
+
+	for _, test := range tests {
+		err := test.rbdVol.validateImageFeatures(test.imageFeatures)
+		if test.isErr {
+			assert.EqualError(t, err, test.errMsg)
+			continue
+		}
+		assert.Nil(t, err)
+	}
+}