Repository: ceph/ceph-csi
Pull request #3033 (2022-04-14 09:57:50)

rbd: fix bug with missing supported_features 
<!-- 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 #

* continue running rbd driver when /sys/bus/rbd/supported_features file is
missing, do not bail out.
* handle when krbdFeatures is zero


## Related issues ##

Fixes: #2678 

---

<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>


Madhu-1: @mergifyio rebase
mergify[bot]: > rebase

#### ✅ Branch has been successfully rebased



<!--
DO NOT EDIT
-*- Mergify Payload -*-
{"command": "rebase", "conclusion": "success"}
-*- Mergify Payload End -*-
-->

pkalever: @Mergifyio rebase
Madhu-1: /retest ci/centos/upgrade-tests-rbd
mergify[bot]: > rebase

#### ✅ Branch has been successfully rebased



<!--
DO NOT EDIT
-*- Mergify Payload -*-
{"command": "rebase", "conclusion": "success"}
-*- Mergify Payload End -*-
-->

pkalever: @Madhu-1 @nixpanic PTAL.
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.23
pkalever: /retest ci/centos/mini-e2e/k8s-1.23
pkalever: /retest ci/centos/upgrade-tests-cephfs
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.22
pkalever: @Mergifyio rebase
mergify[bot]: > rebase

#### ✅ Branch has been successfully rebased



<!--
DO NOT EDIT
-*- Mergify Payload -*-
{"command": "rebase", "conclusion": "success"}
-*- Mergify Payload End -*-
-->

pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.23
pkalever: Diff b/w previous version and this version.
V2: Updated this PR resolving a merge conflict in noderserver.go because of https://github.com/ceph/ceph-csi/pull/3080
pkalever: /retest ci/centos/k8s-e2e-external-storage/1.22
pkalever: /retest ci/centos/mini-e2e/k8s-1.23
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.22
```
May 11 14:03:25.573: FAIL: backend images not matching kubernetes resource count,image count 23 kubernetes resource count 21

```
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.22
humblec: @pkalever linter is faililng.. 
pkalever: /restest ci/centos/mini-e2e-helm/k8s-1.21
/restest ci/centos/mini-e2e-helm/k8s-1.22
/restest ci/centos/mini-e2e-helm/k8s-1.23
```
May 13 10:31:50.222: FAIL: backend images not matching kubernetes resource count,image count 11 kubernetes resource count 10
```
pkalever: /restest ci/centos/mini-e2e-helm/k8s-1.21
pkalever: /restest ci/centos/mini-e2e-helm/k8s-1.22
pkalever: /restest ci/centos/mini-e2e-helm/k8s-1.23
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.21


pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.22
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.23
pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.21


pkalever: /retest ci/centos/mini-e2e-helm/k8s-1.21
nixpanic: /retest ci/centos/mini-e2e-helm/k8s-1.21
nixpanic: /retest ci/centos/mini-e2e-helm/k8s-1.23
nixpanic: /retest ci/centos/mini-e2e-helm/k8s-1.21
nixpanic: /retest ci/centos/mini-e2e-helm/k8s-1.21
nixpanic: /retest ci/centos/mini-e2e-helm/k8s-1.21

Patch:
diff --git a/internal/rbd/driver/driver.go b/internal/rbd/driver/driver.go
index 9cff40f3d..06a526819 100644
--- a/internal/rbd/driver/driver.go
+++ b/internal/rbd/driver/driver.go
@@ -17,7 +17,9 @@ limitations under the License.
 package rbddriver
 
 import (
+	"errors"
 	"fmt"
+	"os"
 
 	casrbd "github.com/ceph/ceph-csi/internal/csi-addons/rbd"
 	csiaddons "github.com/ceph/ceph-csi/internal/csi-addons/server"
@@ -144,7 +146,7 @@ func (r *Driver) Run(conf *util.Config) {
 		}
 		var attr string
 		attr, err = rbd.GetKrbdSupportedFeatures()
-		if err != nil {
+		if err != nil && !errors.Is(err, os.ErrNotExist) {
 			log.FatalLogMsg(err.Error())
 		}
 		var krbdFeatures uint
diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go
index 64798deb5..ddc84cba1 100644
--- a/internal/rbd/nodeserver.go
+++ b/internal/rbd/nodeserver.go
@@ -144,6 +144,7 @@ func healerStageTransaction(ctx context.Context, cr *util.Credentials, volOps *r
 // this function also receive the credentials and secrets args as it differs in its data.
 // The credentials are used directly by functions like voljournal.Connect() and other functions
 // like genVolFromVolumeOptions() make use of secrets.
+// nolint:gocyclo,cyclop // reduce complexity
 func populateRbdVol(
 	ctx context.Context,
 	req *csi.NodeStageVolumeRequest,
@@ -241,8 +242,15 @@ func populateRbdVol(
 		return nil, status.Error(codes.Internal, err.Error())
 	}
 
-	if rv.Mounter == rbdDefaultMounter &&
-		!isKrbdFeatureSupported(ctx, strings.Join(rv.ImageFeatureSet.Names(), ",")) {
+	features := strings.Join(rv.ImageFeatureSet.Names(), ",")
+	isFeatureExist, err := isKrbdFeatureSupported(ctx, features)
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		log.ErrorLog(ctx, "failed checking krbd features %q: %v", features, err)
+
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
+	if rv.Mounter == rbdDefaultMounter && !isFeatureExist {
 		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")
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index 091a7a2e8..ce60e3ee3 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -192,28 +192,83 @@ type migrationVolID struct {
 	clusterID string
 }
 
-var supportedFeatures = map[string]imageFeature{
-	librbd.FeatureNameLayering: {
-		needRbdNbd: false,
-	},
-	librbd.FeatureNameExclusiveLock: {
-		needRbdNbd: false,
-	},
-	librbd.FeatureNameObjectMap: {
-		needRbdNbd: false,
-		dependsOn:  []string{librbd.FeatureNameExclusiveLock},
-	},
-	librbd.FeatureNameFastDiff: {
-		needRbdNbd: false,
-		dependsOn:  []string{librbd.FeatureNameObjectMap},
-	},
-	librbd.FeatureNameJournaling: {
-		needRbdNbd: true,
-		dependsOn:  []string{librbd.FeatureNameExclusiveLock},
-	},
-	librbd.FeatureNameDeepFlatten: {
-		needRbdNbd: false,
-	},
+var (
+	supportedFeatures = map[string]imageFeature{
+		librbd.FeatureNameLayering: {
+			needRbdNbd: false,
+		},
+		librbd.FeatureNameExclusiveLock: {
+			needRbdNbd: false,
+		},
+		librbd.FeatureNameObjectMap: {
+			needRbdNbd: false,
+			dependsOn:  []string{librbd.FeatureNameExclusiveLock},
+		},
+		librbd.FeatureNameFastDiff: {
+			needRbdNbd: false,
+			dependsOn:  []string{librbd.FeatureNameObjectMap},
+		},
+		librbd.FeatureNameJournaling: {
+			needRbdNbd: true,
+			dependsOn:  []string{librbd.FeatureNameExclusiveLock},
+		},
+		librbd.FeatureNameDeepFlatten: {
+			needRbdNbd: false,
+		},
+	}
+
+	krbdLayeringSupport = []util.KernelVersion{
+		{
+			Version:    3,
+			PatchLevel: 8,
+			SubLevel:   0,
+		},
+	}
+	krbdStripingV2Support = []util.KernelVersion{
+		{
+			Version:    3,
+			PatchLevel: 10,
+			SubLevel:   0,
+		},
+	}
+	krbdExclusiveLockSupport = []util.KernelVersion{
+		{
+			Version:    4,
+			PatchLevel: 9,
+			SubLevel:   0,
+		},
+	}
+	krbdDataPoolSupport = []util.KernelVersion{
+		{
+			Version:    4,
+			PatchLevel: 11,
+			SubLevel:   0,
+		},
+	}
+)
+
+// prepareKrbdFeatureAttrs prepare krbd fearure set based on kernel version.
+// Minimum kernel version should be 3.8, else it will return error.
+func prepareKrbdFeatureAttrs() (uint64, error) {
+	// fetch the current running kernel info
+	release, err := util.GetKernelVersion()
+	if err != nil {
+		return 0, fmt.Errorf("fetching current kernel version failed: %w", err)
+	}
+
+	switch {
+	case util.CheckKernelSupport(release, krbdDataPoolSupport):
+		return librbd.FeatureDataPool, nil
+	case util.CheckKernelSupport(release, krbdExclusiveLockSupport):
+		return librbd.FeatureExclusiveLock, nil
+	case util.CheckKernelSupport(release, krbdStripingV2Support):
+		return librbd.FeatureStripingV2, nil
+	case util.CheckKernelSupport(release, krbdLayeringSupport):
+		return librbd.FeatureLayering, nil
+	}
+	log.ErrorLogMsg("kernel version is too old: %q", release)
+
+	return 0, os.ErrNotExist
 }
 
 // GetKrbdSupportedFeatures load the module if needed and return supported
@@ -238,9 +293,19 @@ func GetKrbdSupportedFeatures() (string, error) {
 	}
 	val, err := os.ReadFile(krbdSupportedFeaturesFile)
 	if err != nil {
-		log.ErrorLogMsg("reading file %q failed: %v", krbdSupportedFeaturesFile, err)
+		if !errors.Is(err, os.ErrNotExist) {
+			log.ErrorLogMsg("reading file %q failed: %v", krbdSupportedFeaturesFile, err)
 
-		return "", err
+			return "", err
+		}
+		attrs, err := prepareKrbdFeatureAttrs()
+		if err != nil {
+			log.ErrorLogMsg("preparing krbd feature attributes failed, %v", err)
+
+			return "", err
+		}
+
+		return strconv.FormatUint(attrs, 16), nil
 	}
 
 	return strings.TrimSuffix(string(val), "\n"), nil
@@ -263,7 +328,12 @@ func HexStringToInteger(hexString string) (uint, error) {
 
 // isKrbdFeatureSupported checks if a given Image Feature is supported by krbd
 // driver or not.
-func isKrbdFeatureSupported(ctx context.Context, imageFeatures string) bool {
+func isKrbdFeatureSupported(ctx context.Context, imageFeatures string) (bool, error) {
+	// return false when /sys/bus/rbd/supported_features is absent and we are
+	// not in a position to prepare krbd feature attributes, i.e. if kernel <= 3.8
+	if krbdFeatures == 0 {
+		return false, os.ErrNotExist
+	}
 	arr := strings.Split(imageFeatures, ",")
 	log.UsefulLog(ctx, "checking for ImageFeatures: %v", arr)
 	imageFeatureSet := librbd.FeatureSetFromNames(arr)
@@ -278,7 +348,7 @@ func isKrbdFeatureSupported(ctx context.Context, imageFeatures string) bool {
 		}
 	}
 
-	return supported
+	return supported, nil
 }
 
 // Connect an rbdVolume to the Ceph cluster.


Test patch:
diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go
index 8ab310da0..e373a9e61 100644
--- a/internal/rbd/rbd_util_test.go
+++ b/internal/rbd/rbd_util_test.go
@@ -18,6 +18,7 @@ package rbd
 
 import (
 	"context"
+	"errors"
 	"io/ioutil"
 	"os"
 	"strings"
@@ -346,8 +347,13 @@ func TestIsKrbdFeatureSupported(t *testing.T) {
 			if err != nil {
 				t.Errorf("HexStringToInteger(%s) failed", krbdSupportedFeaturesAttr)
 			}
-			supported := isKrbdFeatureSupported(ctx, tc.featureName)
-			if supported != tc.isSupported {
+			// In case /sys/bus/rbd/supported_features is absent and we are
+			// not in a position to prepare krbd feature attributes,
+			// isKrbdFeatureSupported returns error ErrNotExist
+			supported, err := isKrbdFeatureSupported(ctx, tc.featureName)
+			if err != nil && !errors.Is(err, os.ErrNotExist) {
+				t.Errorf("isKrbdFeatureSupported(%s) returned error: %v", tc.featureName, err)
+			} else if supported != tc.isSupported {
 				t.Errorf("isKrbdFeatureSupported(%s) returned supported status, expected: %t, got: %t",
 					tc.featureName, tc.isSupported, supported)
 			}