Repository: ceph/ceph-csi
Pull request #2556 (2021-10-05 12:27:16)

rbd: detect krbd features in runtime and fallback to nbd
<!-- Please take a look at our [Contributing](https://github.com/ceph/ceph-csi/blob/devel/docs/development-guide.md#Code-contribution-workflow)
documentation before submitting a Pull Request!
Thank you for contributing to ceph-csi! -->

# Describe what this PR does #

Currently, we recognize and warn for the provided image features based on our prior intelligence at ceph-csi (i.e based on supportedFeatures map and validateImageFeatures) at image/PV creation time. It might be very much possible that the cluster is heterogeneous i.e. the PV creation and application container might both be on different nodes with different kernel versions (krbd driver versions).

This PR adds a mechanism to check for the supported krbd features during mount time, if the krbd driver doesn't have the specified image feature then it will fall back to rbd-nbd mounter.


## Is there anything that requires special attention ##

This PR also provides an option to disable the auto fallback to nbd mounter via storageclass `fallbackToNBD` parameter.

## Related issues ##

Fixes: #478 

---

<details>
<summary>Show available bot commands</summary>

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

- `/retest ci/centos/<job-name>`: retest the `<job-name>` after unrelated
  failure (please report the failure too!)
- `/retest all`: run this in case the CentOS CI failed to start/report any test
  progress or results

</details>


nixpanic: Where you mention "realtime", you mean "runtime". Please change the comments accordingly.
pkalever: /retest all
mergify[bot]: **Command `rebase`: success**
> **Branch already up to date**
> 

pkalever: /retest all
pkalever: @ceph/ceph-csi-contributors 
Is there any known issue with the CI?
1. the tests are failing at build artifacts itself
2. the go-test, go-sec, commitlint, codespell ...etc tests are not even got triggered
Madhu-1: /retest all
Madhu-1: @mergifyio rebase
Madhu-1: @pkalever jenkins is back again. can you please force push to start Github actions?
pkalever: > @pkalever jenkins is back again. can you please force push to start Github actions?

Done now, thanks @Madhu-1 
nixpanic: ```
=== CONT  TestIsKrbdFeatureSupported/supported_feature
W1006 06:27:58.852937    8434 rbd_util.go:230] modprobe failed with error an error (exec: "modprobe": executable file not found in $PATH) occurred while running modprobe args: [rbd]
    rbd_util_test.go:321: isKrbdFeatureSupported(layering) got(err) = false(an error (exec: "modprobe": executable file not found in $PATH) occurred while running modprobe args: [rbd]), expected(err) = layering(<nil>)
```
the **go-test** job fails due to this.


I suggest something like
```go
var krbdSupportedFeatures uint64

func getKrbdSupportedFeatures() (uint64, error) {
    if krbdSupportedFeatures != 0 {
        return krbdSupportedFeatures, nil
    }

    // detect features from /sys/... and set krbdSupportedFeatures
}

// unit testing
func TestIsKrbdFeatureSupported() {
    krbdSupportedFeatures = 0x12345

    supported := isKrbdFeatureSupported("layering")
}
```
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.21
pkalever: /retest mini-e2e-helm_k8s-1.21
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.21
pkalever: @Madhu-1 @nixpanic PTAL. Thanks!
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.22

Patch:
diff --git a/charts/ceph-csi-rbd/README.md b/charts/ceph-csi-rbd/README.md
index c048a6d73..117facbe7 100644
--- a/charts/ceph-csi-rbd/README.md
+++ b/charts/ceph-csi-rbd/README.md
@@ -151,6 +151,7 @@ charts and their default values.
 | `storageClass.pool`                            | Ceph pool into which the RBD image shall be created                                                                                                  | `replicapool`                                      |
 | `storageClass.thickProvision`                  | Specifies whether thick provision should be enabled                                                                                                  | `false`                                            |
 | `storageclass.imageFeatures`                   | Specifies RBD image features                                                                                                                         | `layering`                                         |
+| `storageclass.tryOtherMounters`                | Specifies whether to try other mounters in case if the current mounter fails to mount the rbd image for any reason                                   | `false`                                            |
 | `storageClass.mounter`                         | Specifies RBD mounter                                                                                                                                | `""`                                               |
 | `storageClass.cephLogDir`                      | ceph client log location, it is the target bindmount path used inside container                                                                      | `"/var/log/ceph"`                                  |
 | `storageClass.cephLogStrategy`                 | ceph client log strategy, available options `remove` or `compress` or `preserve`                                                                     | `"remove"`                                         |
diff --git a/charts/ceph-csi-rbd/templates/storageclass.yaml b/charts/ceph-csi-rbd/templates/storageclass.yaml
index caba90305..e4da5166f 100644
--- a/charts/ceph-csi-rbd/templates/storageclass.yaml
+++ b/charts/ceph-csi-rbd/templates/storageclass.yaml
@@ -19,6 +19,9 @@ parameters:
   pool: {{ .Values.storageClass.pool }}
   imageFeatures: {{ .Values.storageClass.imageFeatures }}
   thickProvision: {{ .Values.storageClass.thickProvision | quote}}
+{{- if .Values.storageClass.tryOtherMounters }}
+  tryOtherMounters: {{ .Values.storageClass.tryOtherMounters | quote}}
+{{- end }}
 {{- if .Values.storageClass.mounter }}
   mounter: {{ .Values.storageClass.mounter }}
 {{- end }}
diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml
index d0ae4f92e..2bf97d554 100644
--- a/charts/ceph-csi-rbd/values.yaml
+++ b/charts/ceph-csi-rbd/values.yaml
@@ -289,6 +289,15 @@ storageClass:
   # imageFeatures: layering,journaling,exclusive-lock
   imageFeatures: layering
 
+  # (optional) Specifies whether to try other mounters in case if the current
+  # mounter fails to mount the rbd image for any reason. True means fallback
+  # to next mounter, default is set to false.
+  # Note: tryOtherMounters is currently useful to fallback from krbd to rbd-nbd
+  # in case if any of the specified imageFeatures is not supported by krbd
+  # driver on node scheduled for application pod launch, but in the future this
+  # should work with any mounter type.
+  # tryOtherMounters: false
+
   # (optional) uncomment the following to use rbd-nbd as mounter
   # on supported nodes
   # mounter: rbd-nbd
diff --git a/docs/deploy-rbd.md b/docs/deploy-rbd.md
index f8fa2dea1..2dd798c1e 100644
--- a/docs/deploy-rbd.md
+++ b/docs/deploy-rbd.md
@@ -56,7 +56,7 @@ make image-cephcsi
 | `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. |
-|                                                                                                     |
+| `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.                                                                                                                                                                                                                                                                                           |
 | `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                                                                                                                                                                                                                                                                                                                                                                                    |
diff --git a/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml
index a53aace7c..baba539b2 100644
--- a/examples/rbd/storageclass.yaml
+++ b/examples/rbd/storageclass.yaml
@@ -39,6 +39,15 @@ parameters:
    # imageFeatures: layering,journaling,exclusive-lock
    imageFeatures: layering
 
+   # (optional) Specifies whether to try other mounters in case if the current
+   # mounter fails to mount the rbd image for any reason. True means fallback
+   # to next mounter, default is set to false.
+   # Note: tryOtherMounters is currently useful to fallback from krbd to rbd-nbd
+   # in case if any of the specified imageFeatures is not supported by krbd
+   # driver on node scheduled for application pod launch, but in the future this
+   # should work with any mounter type.
+   # tryOtherMounters: false
+
    # (optional) mapOptions is a comma-separated list of map options.
    # For krbd options refer
    # https://docs.ceph.com/docs/master/man/8/rbd/#kernel-rbd-krbd-options
diff --git a/internal/rbd/driver.go b/internal/rbd/driver.go
index b5c2943eb..cdb2ba1e4 100644
--- a/internal/rbd/driver.go
+++ b/internal/rbd/driver.go
@@ -157,6 +157,15 @@ func (r *Driver) Run(conf *util.Config) {
 		if err != nil {
 			log.FatalLogMsg("failed to start node server, err %v\n", err)
 		}
+		var attr string
+		attr, err = getKrbdSupportedFeatures()
+		if err != nil {
+			log.FatalLogMsg(err.Error())
+		}
+		krbdFeatures, err = hexStringToInteger(attr)
+		if err != nil {
+			log.FatalLogMsg(err.Error())
+		}
 	}
 
 	if conf.IsControllerServer {
diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go
index 052deca24..b14a381b9 100644
--- a/internal/rbd/nodeserver.go
+++ b/internal/rbd/nodeserver.go
@@ -67,6 +67,10 @@ const (
 	xfsReflinkUnset int = iota
 	xfsReflinkNoSupport
 	xfsReflinkSupport
+
+	staticVol        = "staticVolume"
+	volHealerCtx     = "volumeHealerContext"
+	tryOtherMounters = "tryOtherMounters"
 )
 
 var (
@@ -98,34 +102,20 @@ var (
 	xfsHasReflink = xfsReflinkUnset
 )
 
-// isHealerContext checks if the request is been made from volumeHealer.
-func isHealerContext(parameters map[string]string) bool {
-	var err error
-	healerContext := false
-
-	val, ok := parameters["volumeHealerContext"]
-	if ok {
-		if healerContext, err = strconv.ParseBool(val); err != nil {
-			return false
-		}
-	}
-
-	return healerContext
-}
-
-// isStaticVolume checks if the volume is static.
-func isStaticVolume(parameters map[string]string) bool {
-	var err error
-	staticVol := false
+// parseBoolOption checks if parameters contain option and parse it. If it is
+// empty or not set return default.
+// nolint:unparam // currently defValue is always false, this can change in the future
+func parseBoolOption(ctx context.Context, parameters map[string]string, optionName string, defValue bool) bool {
+	boolVal := defValue
 
-	val, ok := parameters["staticVolume"]
-	if ok {
-		if staticVol, err = strconv.ParseBool(val); err != nil {
-			return false
+	if val, ok := parameters[optionName]; ok {
+		var err error
+		if boolVal, err = strconv.ParseBool(val); err != nil {
+			log.ErrorLog(ctx, "failed to parse value of %q: %q", optionName, val)
 		}
 	}
 
-	return staticVol
+	return boolVal
 }
 
 // healerStageTransaction attempts to attach the rbd Image with previously
@@ -189,7 +179,7 @@ func populateRbdVol(
 	}
 
 	rv.ThickProvision = isThickProvisionRequest(req.GetVolumeContext())
-	isStaticVol := isStaticVolume(req.GetVolumeContext())
+	isStaticVol := parseBoolOption(ctx, req.GetVolumeContext(), staticVol, false)
 	// get rbd image name from the volume journal
 	// for static volumes, the image name is actually the volume ID itself
 	if isStaticVol {
@@ -226,10 +216,27 @@ func populateRbdVol(
 		rv.RbdImageName = imageAttributes.ImageName
 	}
 
+	krbdSupported := false
+	if req.GetVolumeContext()["mounter"] != rbdNbdMounter {
+		krbdSupported = isKrbdFeatureSupported(ctx, req.GetVolumeContext()["imageFeatures"])
+		if !krbdSupported && !parseBoolOption(ctx, req.GetVolumeContext(), tryOtherMounters, false) {
+			log.ErrorLog(ctx, "unsupported krbd Feature, set `tryOtherMounters:true` or fix krbd driver")
+
+			return nil, status.Errorf(codes.Internal, "unsupported krbd Feature")
+		}
+	}
+	if krbdSupported {
+		rv.MapOptions = req.GetVolumeContext()["mapOptions"]
+		rv.UnmapOptions = req.GetVolumeContext()["unmapOptions"]
+		rv.Mounter = req.GetVolumeContext()["mounter"]
+	} else {
+		// fallback to rbd-nbd,
+		// ignore the mapOptions and unmapOptions as they are meant for krbd use.
+		rv.Mounter = rbdNbdMounter
+	}
+
 	rv.VolID = volID
-	rv.MapOptions = req.GetVolumeContext()["mapOptions"]
-	rv.UnmapOptions = req.GetVolumeContext()["unmapOptions"]
-	rv.Mounter = req.GetVolumeContext()["mounter"]
+
 	rv.LogDir = req.GetVolumeContext()["cephLogDir"]
 	if rv.LogDir == "" {
 		rv.LogDir = defaultLogDir
@@ -287,7 +294,7 @@ func (ns *NodeServer) NodeStageVolume(
 	stagingParentPath := req.GetStagingTargetPath()
 	stagingTargetPath := stagingParentPath + "/" + volID
 
-	isHealer := isHealerContext(req.GetVolumeContext())
+	isHealer := parseBoolOption(ctx, req.GetVolumeContext(), volHealerCtx, false)
 	if !isHealer {
 		var isNotMnt bool
 		// check if stagingPath is already mounted
@@ -301,7 +308,7 @@ func (ns *NodeServer) NodeStageVolume(
 		}
 	}
 
-	isStaticVol := isStaticVolume(req.GetVolumeContext())
+	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
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index 08d44bad3..3dff543fd 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -91,6 +91,9 @@ const (
 	migImageNamePrefix = "image-"
 	// prefix in the handle for monitors field.
 	migMonPrefix = "mons-"
+
+	// krbd attribute file to check supported features.
+	krbdSupportedFeaturesFile = "/sys/bus/rbd/supported_features"
 )
 
 // rbdImage contains common attributes and methods for the rbdVolume and
@@ -197,17 +200,85 @@ type migrationVolID struct {
 	clusterID string
 }
 
-var supportedFeatures = map[string]imageFeature{
-	librbd.FeatureNameLayering: {
-		needRbdNbd: false,
-	},
-	librbd.FeatureNameExclusiveLock: {
-		needRbdNbd: true,
-	},
-	librbd.FeatureNameJournaling: {
-		needRbdNbd: true,
-		dependsOn:  []string{librbd.FeatureNameExclusiveLock},
-	},
+var (
+	supportedFeatures = map[string]imageFeature{
+		librbd.FeatureNameLayering: {
+			needRbdNbd: false,
+		},
+		librbd.FeatureNameExclusiveLock: {
+			needRbdNbd: true,
+		},
+		librbd.FeatureNameJournaling: {
+			needRbdNbd: true,
+			dependsOn:  []string{librbd.FeatureNameExclusiveLock},
+		},
+	}
+
+	krbdFeatures uint64 // krbd features supported by the loaded driver.
+)
+
+// getKrbdSupportedFeatures load the module if needed and return supported
+// features attribute as a string.
+func getKrbdSupportedFeatures() (string, error) {
+	// check if the module is loaded or compiled in
+	_, err := os.Stat(krbdSupportedFeaturesFile)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			log.ErrorLogMsg("stat on %q failed: %v", krbdSupportedFeaturesFile, err)
+
+			return "", err
+		}
+		// try to load the module
+		_, _, err = util.ExecCommand(context.TODO(), "modprobe", rbdDefaultMounter)
+		if err != nil {
+			log.ErrorLogMsg("modprobe failed: %v", err)
+
+			return "", err
+		}
+	}
+	val, err := ioutil.ReadFile(krbdSupportedFeaturesFile)
+	if err != nil {
+		log.ErrorLogMsg("reading file %q failed: %v", krbdSupportedFeaturesFile, err)
+
+		return "", err
+	}
+
+	return strings.TrimSuffix(string(val), "\n"), nil
+}
+
+// hexStringToInteger convert hex value to uint.
+func hexStringToInteger(hexString string) (uint64, error) {
+	// trim 0x prefix
+	numberStr := strings.TrimPrefix(strings.ToLower(hexString), "0x")
+
+	output, err := strconv.ParseUint(numberStr, 16, 64)
+	if err != nil {
+		log.ErrorLogMsg("converting string %q to integer failed: %v", numberStr, err)
+
+		return 0, err
+	}
+
+	return output, nil
+}
+
+// isKrbdFeatureSupported checks if a given Image Feature is supported by krbd
+// driver or not.
+func isKrbdFeatureSupported(ctx context.Context, imageFeatures string) bool {
+	arr := strings.Split(imageFeatures, ",")
+	log.UsefulLog(ctx, "checking for ImageFeatures: %v", arr)
+	imageFeatureSet := librbd.FeatureSetFromNames(arr)
+
+	supported := true
+	for _, featureName := range imageFeatureSet.Names() {
+		if (uint64(librbd.FeatureSetFromNames(strings.Split(featureName, " "))) & krbdFeatures) == 0 {
+			supported = false
+			log.ErrorLog(ctx, "krbd feature %q not supported", featureName)
+
+			break
+		}
+	}
+
+	return supported
 }
 
 // Connect an rbdVolume to the Ceph cluster.


Test patch:
diff --git a/internal/rbd/nodeserver_test.go b/internal/rbd/nodeserver_test.go
index 4046c0905..871a6ec76 100644
--- a/internal/rbd/nodeserver_test.go
+++ b/internal/rbd/nodeserver_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package rbd
 
 import (
+	"context"
 	"testing"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
@@ -56,3 +57,51 @@ func TestGetStagingPath(t *testing.T) {
 		t.Errorf("getStagingTargetPath() = %s, got %s", stagingPath, expect)
 	}
 }
+
+func TestParseBoolOption(t *testing.T) {
+	t.Parallel()
+	ctx := context.TODO()
+	optionName := "myOption"
+	defaultValue := false
+
+	tests := []struct {
+		name         string
+		scParameters map[string]string
+		expect       bool
+	}{
+		{
+			name:         "myOption => true",
+			scParameters: map[string]string{optionName: "true"},
+			expect:       true,
+		},
+		{
+			name:         "myOption => false",
+			scParameters: map[string]string{optionName: "false"},
+			expect:       false,
+		},
+		{
+			name:         "myOption => empty",
+			scParameters: map[string]string{optionName: ""},
+			expect:       defaultValue,
+		},
+		{
+			name:         "myOption => not-parsable",
+			scParameters: map[string]string{optionName: "non-boolean"},
+			expect:       defaultValue,
+		},
+		{
+			name:         "myOption => not-set",
+			scParameters: map[string]string{},
+			expect:       defaultValue,
+		},
+	}
+
+	for _, tt := range tests {
+		tc := tt
+		val := parseBoolOption(ctx, tc.scParameters, optionName, defaultValue)
+		if val != tc.expect {
+			t.Errorf("parseBoolOption(%v) returned: %t, expected: %t",
+				tc.scParameters, val, tc.expect)
+		}
+	}
+}
diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go
index eeb94c69e..f00bc038c 100644
--- a/internal/rbd/rbd_util_test.go
+++ b/internal/rbd/rbd_util_test.go
@@ -283,3 +283,42 @@ func TestStrategicActionOnLogFile(t *testing.T) {
 		})
 	}
 }
+
+func TestIsKrbdFeatureSupported(t *testing.T) {
+	t.Parallel()
+	ctx := context.TODO()
+
+	tests := []struct {
+		name        string
+		featureName string
+		isSupported bool
+	}{
+		{
+			name:        "supported feature",
+			featureName: "layering",
+			isSupported: true,
+		},
+		{
+			name:        "not supported feature",
+			featureName: "journaling",
+			isSupported: false,
+		},
+	}
+	for _, tt := range tests {
+		tc := tt
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			var err error
+			krbdSupportedFeaturesAttr := "0x1"
+			krbdFeatures, err = hexStringToInteger(krbdSupportedFeaturesAttr) // initialize krbdFeatures
+			if err != nil {
+				t.Errorf("hexStringToInteger(%s) failed", krbdSupportedFeaturesAttr)
+			}
+			supported := isKrbdFeatureSupported(ctx, tc.featureName)
+			if supported != tc.isSupported {
+				t.Errorf("isKrbdFeatureSupported(%s) returned supported status, expected: %t, got: %t",
+					tc.featureName, tc.isSupported, supported)
+			}
+		})
+	}
+}