Repository: ceph/ceph-csi
Pull request #1178 (2020-06-18 12:25:45)

rbd: use librbd.FeatureSet for features
# Describe what this PR does #

go-ceph v0.3 adds constants for ImageFeature values and their names.
Instead of hardcoding "layering" in several places, use the constant
given by librbd.

The rbdVolume.ImageFeatures does not seem to be used anywhere after the
conversion. Stashing the image metadata does include the ImageFeatures
as these are retrieved when getting the image information. It is safe to
drop ImageFeatures altogether and only use the imageFeatureSet instead.


nixpanic: @phlogistonjohn some go-ceph changes for you to review :grin: 
humblec: > Nice reduction in code. :-)

Indeed, we need more :)
Madhu-1: @humblec PTAL

Patch:
diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
index 1ac3f95fb..060399e33 100644
--- a/internal/rbd/controllerserver.go
+++ b/internal/rbd/controllerserver.go
@@ -532,7 +532,7 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 	}
 
 	// Check if source volume was created with required image features for snaps
-	if !hasSnapshotFeature(rbdVol.ImageFeatures) {
+	if !rbdVol.hasSnapshotFeature() {
 		return nil, status.Errorf(codes.InvalidArgument, "volume(%s) has not snapshot feature(layering)", req.GetSourceVolumeId())
 	}
 
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index 9bbf19499..e3c54a9ad 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -89,7 +89,7 @@ type rbdVolume struct {
 	JournalPool         string
 	Pool                string `json:"pool"`
 	DataPool            string
-	ImageFeatures       string `json:"imageFeatures"`
+	imageFeatureSet     librbd.FeatureSet
 	AdminID             string `json:"adminId"`
 	UserID              string `json:"userId"`
 	Mounter             string `json:"mounter"`
@@ -132,7 +132,7 @@ type rbdSnapshot struct {
 }
 
 var (
-	supportedFeatures = sets.NewString("layering")
+	supportedFeatures = sets.NewString(librbd.FeatureNameLayering)
 )
 
 // Connect an rbdVolume to the Ceph cluster
@@ -190,11 +190,10 @@ func createImage(ctx context.Context, pOpts *rbdVolume, cr *util.Credentials) er
 		}
 	}
 	klog.V(4).Infof(util.Log(ctx, logMsg),
-		pOpts, volSzMiB, pOpts.ImageFeatures, pOpts.Monitors)
+		pOpts, volSzMiB, pOpts.imageFeatureSet.Names(), pOpts.Monitors)
 
-	if pOpts.ImageFeatures != "" {
-		features := imageFeaturesToUint64(ctx, pOpts.ImageFeatures)
-		err := options.SetUint64(librbd.RbdImageOptionFeatures, features)
+	if pOpts.imageFeatureSet != 0 {
+		err := options.SetUint64(librbd.RbdImageOptionFeatures, uint64(pOpts.imageFeatureSet))
 		if err != nil {
 			return errors.Wrapf(err, "failed to set image features")
 		}
@@ -637,7 +636,7 @@ func genVolFromVolumeOptions(ctx context.Context, volOptions, credentials map[st
 					" features are: %v", f, supportedFeatures)
 			}
 		}
-		rbdVol.ImageFeatures = imageFeatures
+		rbdVol.imageFeatureSet = librbd.FeatureSetFromNames(arr)
 	}
 
 	klog.V(3).Infof(util.Log(ctx, "setting disableInUseChecks on rbd volume to: %v"), disableInUseChecks)
@@ -691,29 +690,9 @@ func genSnapFromOptions(ctx context.Context, rbdVol *rbdVolume, snapOptions map[
 	return rbdSnap
 }
 
-func hasSnapshotFeature(imageFeatures string) bool {
-	arr := strings.Split(imageFeatures, ",")
-	for _, f := range arr {
-		if f == "layering" {
-			return true
-		}
-	}
-	return false
-}
-
-// imageFeaturesToUint64 takes the comma separated image features and converts
-// them to a RbdImageOptionFeatures value.
-func imageFeaturesToUint64(ctx context.Context, imageFeatures string) uint64 {
-	features := uint64(0)
-
-	for _, f := range strings.Split(imageFeatures, ",") {
-		if f == "layering" {
-			features |= librbd.RbdFeatureLayering
-		} else {
-			klog.Warningf(util.Log(ctx, "rbd: image feature %s not recognized, skipping"), f)
-		}
-	}
-	return features
+// hasSnapshotFeature checks if Layering is enabled for this image
+func (rv *rbdVolume) hasSnapshotFeature() bool {
+	return (uint64(rv.imageFeatureSet) & librbd.FeatureLayering) == librbd.FeatureLayering
 }
 
 func protectSnapshot(ctx context.Context, pOpts *rbdSnapshot, cr *util.Credentials) error {
@@ -841,8 +820,7 @@ func (rv *rbdVolume) getImageInfo() error {
 	if err != nil {
 		return err
 	}
-	fs := librbd.FeatureSet(features)
-	rv.ImageFeatures = strings.Join(fs.Names(), ",")
+	rv.imageFeatureSet = librbd.FeatureSet(features)
 
 	return nil
 }


Test patch:
diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go
index 3ae563549..8b458d617 100644
--- a/internal/rbd/rbd_util_test.go
+++ b/internal/rbd/rbd_util_test.go
@@ -17,7 +17,10 @@ limitations under the License.
 package rbd
 
 import (
+	"strings"
 	"testing"
+
+	librbd "github.com/ceph/go-ceph/rbd"
 )
 
 func TestIsLegacyVolumeID(t *testing.T) {
@@ -48,8 +51,11 @@ func TestHasSnapshotFeature(t *testing.T) {
 		{"foo,layering,bar", true},
 	}
 
+	rv := rbdVolume{}
+
 	for _, test := range tests {
-		if got := hasSnapshotFeature(test.features); got != test.hasFeature {
+		rv.imageFeatureSet = librbd.FeatureSetFromNames(strings.Split(test.features, ","))
+		if got := rv.hasSnapshotFeature(); got != test.hasFeature {
 			t.Errorf("hasSnapshotFeature(%s) = %t, want %t", test.features, got, test.hasFeature)
 		}
 	}