[manila-csi-plugin] snapshot caps check (kubernetes#1592)

* go.mod: bumped github.com/gophercloud/gophercloud to v0.19.0

* removed hardcoded checks for cephfs, added checks for caps in parent share

This commit removes all hard-coded checks for CephFS shares,
preventing them from being snapshotted. Instead, manila-csi
now checks for snapshot_support and create_share_from_snapshot
capabilities.

* share: added handling for creating_from_snapshot status

* docs: added a note about snapshots

* go.sum: added k8s.io/code-generator

* go.mod fix 2: added github.com/gophercloud/gophercloud v0.19.0

Author: gman0

docs/manila-csi-plugin/using-manila-csi-plugin.md

@@ -21,7 +21,9 @@
 
 # CSI Manila driver
 
-The CSI Manila driver is able to create, expand and mount OpenStack Manila shares. Snapshots and recovering shares from snapshots is supported as well (support for CephFS snapshots will be added soon).
+The CSI Manila driver is able to create, expand, snapshot, restore and mount OpenStack Manila shares.
+
+Currently supported Manila backends are NFS and native CephFS.
 
 ## Configuration
 

go.mod

@@ -6,7 +6,7 @@ require (
 	github.com/MichaelTJones/walk v0.0.0-20161122175330-4748e29d5718 // indirect
 	github.com/container-storage-interface/spec v1.3.0
 	github.com/golang/protobuf v1.4.3
-	github.com/gophercloud/gophercloud v0.15.1-0.20210105012856-e34a44dc6580
+	github.com/gophercloud/gophercloud v0.19.0
 	github.com/gophercloud/utils v0.0.0-20200423144003-7c72efc7435d
 	github.com/gorilla/mux v1.8.0
 	github.com/hashicorp/go-version v1.2.0

go.sum

@@ -426,6 +426,8 @@ github.com/gophercloud/gophercloud v0.1.0/go.mod h1:vxM41WHh5uqHVBMZHzuwNOHh8XEo
 github.com/gophercloud/gophercloud v0.6.1-0.20191122030953-d8ac278c1c9d/go.mod h1:ozGNgr9KYOVATV5jsgHl/ceCDXGuguqOZAzoQ/2vcNM=
 github.com/gophercloud/gophercloud v0.15.1-0.20210105012856-e34a44dc6580 h1:EWFhxn19tw6t3LjFe95Kli30sBI02+fPYBqMvXmCvtU=
 github.com/gophercloud/gophercloud v0.15.1-0.20210105012856-e34a44dc6580/go.mod h1:VX0Ibx85B60B5XOrZr6kaNwrmPUzcmMpwxvQ1WQIIWM=
+github.com/gophercloud/gophercloud v0.19.0 h1:zzaIh8W2K5M4AkJhPV1z6O4Sp0FOObzXm61NUmFz3Kw=
+github.com/gophercloud/gophercloud v0.19.0/go.mod h1:wRtmUelyIIv3CSSDI47aUwbs075O6i+LY+pXsKCBsb4=
 github.com/gophercloud/utils v0.0.0-20200423144003-7c72efc7435d h1:fduaPzWwIfvOMLuHk2Al3GZH0XbUqG8MbElPop+Igzs=
 github.com/gophercloud/utils v0.0.0-20200423144003-7c72efc7435d/go.mod h1:ehWUbLQJPqS0Ep+CxeD559hsm9pthPXadJNKwZkp43w=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8=
@@ -884,6 +886,7 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U
 golang.org/x/crypto v0.0.0-20191202143827-86a70503ff7e/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20201002170205-7f63de1d35b0/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
+golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I=
 golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83 h1:/ZScEX8SfEmUGRHs0gxpqteO5nfNW6axyZbBdw9A12g=
 golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I=
 golang.org/x/exp v0.0.0-20180321215751-8460e604b9de/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=

pkg/csi/manila/controllerserver.go

@@ -45,7 +45,7 @@ var (
 	pendingSnapshots = sync.Map{}
 )
 
-func getVolumeCreator(source *csi.VolumeContentSource, shareOpts *options.ControllerVolumeContext, compatOpts *options.CompatibilityOptions, shareTypeCaps capabilities.ManilaCapabilities) (volumeCreator, error) {
+func getVolumeCreator(source *csi.VolumeContentSource, shareOpts *options.ControllerVolumeContext, compatOpts *options.CompatibilityOptions) (volumeCreator, error) {
 	if source == nil {
 		return &blankVolume{}, nil
 	}
@@ -55,11 +55,6 @@ func getVolumeCreator(source *csi.VolumeContentSource, shareOpts *options.Contro
 	}
 
 	if source.GetSnapshot() != nil {
-		if tryCompatForVolumeSource(compatOpts, shareOpts, source, shareTypeCaps) != nil {
-			klog.Infof("share type %s does not advertise create_share_from_snapshot_support capability, compatibility mode is available", shareOpts.Type)
-			return &blankVolume{}, nil
-		}
-
 		return &volumeFromSnapshot{}, nil
 	}
 
@@ -121,7 +116,7 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 
 	// Retrieve an existing share or create a new one
 
-	volCreator, err := getVolumeCreator(req.GetVolumeContentSource(), shareOpts, cs.d.compatOpts, shareTypeCaps)
+	volCreator, err := getVolumeCreator(req.GetVolumeContentSource(), shareOpts, cs.d.compatOpts)
 	if err != nil {
 		return nil, err
 	}
@@ -148,15 +143,6 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 		return nil, status.Errorf(codes.Internal, "failed to grant access to volume %s: %v", share.Name, err)
 	}
 
-	// Check if compatibility layer is needed and can be used
-	if compatLayer := tryCompatForVolumeSource(cs.d.compatOpts, shareOpts, req.GetVolumeContentSource(), shareTypeCaps); compatLayer != nil {
-		if err = compatLayer.SupplementCapability(cs.d.compatOpts, share, accessRight, req, cs.d.fwdEndpoint, manilaClient, cs.d.csiClientBuilder); err != nil {
-			// An error occurred, the user must clean the share manually
-			// TODO needs proper monitoring
-			return nil, err
-		}
-	}
-
 	var accessibleTopology []*csi.Topology
 	if cs.d.withTopology {
 		// All requisite/preferred topologies are considered valid. Nodes from those zones are required to be able to reach the storage.
@@ -202,12 +188,6 @@ func (cs *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 }
 
 func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) {
-	if cs.d.shareProto == "CEPHFS" {
-		// Restoring shares from CephFS snapshots needs special handling that's not implemented yet.
-		// TODO: Creating CephFS snapshots is forbidden until CephFS restoration is in place.
-		return nil, status.Errorf(codes.InvalidArgument, "the driver doesn't support snapshotting CephFS shares yet")
-	}
-
 	if err := validateCreateSnapshotRequest(req); err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
@@ -246,6 +226,17 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 			sourceShare.ShareProto, req.GetSourceVolumeId(), cs.d.shareProto)
 	}
 
+	// In order to satisfy CSI spec requirements around CREATE_DELETE_SNAPSHOT
+	// and the ability to populate volumes with snapshot contents, parent share
+	// must advertise snapshot_support and create_share_from_snapshot_support
+	// capabilities.
+
+	if !sourceShare.SnapshotSupport || !sourceShare.CreateShareFromSnapshotSupport {
+		return nil, status.Errorf(codes.InvalidArgument,
+			"cannot create snapshot %s for volume %s: parent share must advertise snapshot_support and create_share_from_snapshot_support capabilities",
+			req.GetName(), req.GetSourceVolumeId())
+	}
+
 	// Retrieve an existing snapshot or create a new one
 
 	snapshot, err := getOrCreateSnapshot(req.GetName(), sourceShare.ID, manilaClient)
@@ -258,11 +249,11 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 			return nil, status.Errorf(codes.NotFound, "failed to create snapshot %s for volume %s because the volume doesn't exist: %v", req.GetName(), req.GetSourceVolumeId(), err)
 		}
 
-		return nil, status.Errorf(codes.Internal, "failed to create a snapshot %s of volume  %s: %v", req.GetName(), req.GetSourceVolumeId(), err)
+		return nil, status.Errorf(codes.Internal, "failed to create snapshot %s of volume  %s: %v", req.GetName(), req.GetSourceVolumeId(), err)
 	}
 
 	if err = verifySnapshotCompatibility(snapshot, req); err != nil {
-		return nil, status.Errorf(codes.AlreadyExists, "a snapshot named %s already exists, but is incompatible with the request: %v", req.GetName(), err)
+		return nil, status.Errorf(codes.AlreadyExists, "snapshot %s already exists, but is incompatible with the request: %v", req.GetName(), err)
 	}
 
 	// Check for snapshot status, determine whether it's ready
@@ -307,12 +298,6 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 }
 
 func (cs *controllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequest) (*csi.DeleteSnapshotResponse, error) {
-	if cs.d.shareProto == "CEPHFS" {
-		// Restoring shares from CephFS snapshots needs special handling that's not implemented yet.
-		// TODO: Deleting CephFS snapshots is forbidden until CephFS restoration is in place.
-		return nil, status.Errorf(codes.InvalidArgument, "the driver doesn't support CephFS snapshots yet")
-	}
-
 	if err := validateDeleteSnapshotRequest(req); err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}

pkg/csi/manila/share.go

@@ -33,16 +33,31 @@ const (
 	waitForAvailableShareTimeout = 3
 	waitForAvailableShareRetries = 10
 
-	shareCreating      = "creating"
-	shareDeleting      = "deleting"
-	shareExtending     = "extending"
-	shareError         = "error"
-	shareErrorDeleting = "error_deleting"
-	shareAvailable     = "available"
+	shareCreating             = "creating"
+	shareCreatingFromSnapshot = "creating_from_snapshot"
+	shareDeleting             = "deleting"
+	shareExtending            = "extending"
+	shareError                = "error"
+	shareErrorDeleting        = "error_deleting"
+	shareErrorExtending       = "extending_error"
+	shareAvailable            = "available"
 
 	shareDescription = "provisioned-by=manila.csi.openstack.org"
 )
 
+var (
+	shareErrorStatuses = map[string]struct{}{
+		shareError:          {},
+		shareErrorDeleting:  {},
+		shareErrorExtending: {},
+	}
+)
+
+func isShareInErrorState(s string) bool {
+	_, found := shareErrorStatuses[s]
+	return found
+}
+
 // getOrCreateShare first retrieves an existing share with name=shareName, or creates a new one if it doesn't exist yet.
 // Once the share is created, an exponential back-off is used to wait till the status of the share is "available".
 func getOrCreateShare(shareName string, createOpts *shares.CreateOpts, manilaClient manilaclient.Interface) (*shares.Share, manilaError, error) {
@@ -75,7 +90,7 @@ func getOrCreateShare(shareName string, createOpts *shares.CreateOpts, manilaCli
 		return share, 0, nil
 	}
 
-	return waitForShareStatus(share.ID, shareCreating, shareAvailable, false, manilaClient)
+	return waitForShareStatus(share.ID, []string{shareCreating, shareCreatingFromSnapshot}, shareAvailable, false, manilaClient)
 }
 
 func deleteShare(shareID string, manilaClient manilaclient.Interface) error {
@@ -101,7 +116,7 @@ func tryDeleteShare(share *shares.Share, manilaClient manilaclient.Interface) {
 		return
 	}
 
-	_, _, err := waitForShareStatus(share.ID, shareDeleting, "", true, manilaClient)
+	_, _, err := waitForShareStatus(share.ID, []string{shareDeleting}, "", true, manilaClient)
 	if err != nil && err != wait.ErrWaitTimeout {
 		klog.Errorf("couldn't retrieve volume %s in a roll-back procedure: %v", share.Name, err)
 	}
@@ -116,7 +131,7 @@ func extendShare(share *shares.Share, newSizeInGiB int, manilaClient manilaclien
 		return err
 	}
 
-	share, manilaErrCode, err := waitForShareStatus(share.ID, shareExtending, shareAvailable, false, manilaClient)
+	share, manilaErrCode, err := waitForShareStatus(share.ID, []string{shareExtending}, shareAvailable, false, manilaClient)
 	if err != nil {
 		if err == wait.ErrWaitTimeout {
 			return status.Errorf(codes.DeadlineExceeded, "deadline exceeded while waiting for volume ID %s to become available", share.Name)
@@ -128,7 +143,7 @@ func extendShare(share *shares.Share, newSizeInGiB int, manilaClient manilaclien
 	return nil
 }
 
-func waitForShareStatus(shareID, currentStatus, desiredStatus string, successOnNotFound bool, manilaClient manilaclient.Interface) (*shares.Share, manilaError, error) {
+func waitForShareStatus(shareID string, validTransientStates []string, desiredStatus string, successOnNotFound bool, manilaClient manilaclient.Interface) (*shares.Share, manilaError, error) {
 	var (
 		backoff = wait.Backoff{
 			Duration: time.Second * waitForAvailableShareTimeout,
@@ -141,6 +156,15 @@ func waitForShareStatus(shareID, currentStatus, desiredStatus string, successOnN
 		err           error
 	)
 
+	isInTransientState := func(s string) bool {
+		for _, val := range validTransientStates {
+			if s == val {
+				return true
+			}
+		}
+		return false
+	}
+
 	return share, manilaErrCode, wait.ExponentialBackoff(backoff, func() (bool, error) {
 		share, err = manilaClient.GetShareByID(shareID)
 
@@ -152,25 +176,24 @@ func waitForShareStatus(shareID, currentStatus, desiredStatus string, successOnN
 			return false, err
 		}
 
-		var isAvailable bool
+		if share.Status == desiredStatus {
+			return true, nil
+		}
+
+		if isInTransientState(share.Status) {
+			return false, nil
+		}
 
-		switch share.Status {
-		case currentStatus:
-			isAvailable = false
-		case desiredStatus:
-			isAvailable = true
-		case shareError:
+		if isShareInErrorState(share.Status) {
 			manilaErrMsg, err := lastResourceError(shareID, manilaClient)
 			if err != nil {
 				return false, fmt.Errorf("share %s is in error state, error description could not be retrieved: %v", shareID, err)
 			}
 
 			manilaErrCode = manilaErrMsg.errCode
-			return false, fmt.Errorf("share %s is in error state: %s", shareID, manilaErrMsg.message)
-		default:
-			return false, fmt.Errorf("share %s is in an unexpected state: wanted either %s or %s, got %s", shareID, currentStatus, desiredStatus, share.Status)
+			return false, fmt.Errorf("share %s is in error state \"%s\": %s", shareID, share.Status, manilaErrMsg.message)
 		}
 
-		return isAvailable, nil
+		return false, fmt.Errorf("share %s is in an unexpected state: wanted either %v or %s, got %s", shareID, validTransientStates, desiredStatus, share.Status)
 	})
 }

pkg/csi/manila/util.go

@@ -19,7 +19,6 @@ package manila
 import (
 	"errors"
 	"fmt"
-	"strconv"
 	"strings"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
@@ -28,7 +27,6 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/sharedfilesystems/v2/snapshots"
 	"google.golang.org/grpc/codes"
 	"k8s.io/cloud-provider-openstack/pkg/csi/manila/capabilities"
-	"k8s.io/cloud-provider-openstack/pkg/csi/manila/compatibility"
 	"k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient"
 	"k8s.io/cloud-provider-openstack/pkg/csi/manila/options"
 	"k8s.io/klog/v2"
@@ -156,18 +154,6 @@ func compareProtocol(protoA, protoB string) bool {
 	return strings.ToUpper(protoA) == strings.ToUpper(protoB)
 }
 
-func tryCompatForVolumeSource(compatOpts *options.CompatibilityOptions, shareOpts *options.ControllerVolumeContext, source *csi.VolumeContentSource, shareTypeCaps capabilities.ManilaCapabilities) compatibility.Layer {
-	if source != nil {
-		if source.GetSnapshot() != nil && shareTypeCaps[capabilities.ManilaCapabilitySnapshot] {
-			if createShareFromSnapshotEnabled, _ := strconv.ParseBool(compatOpts.CreateShareFromSnapshotEnabled); createShareFromSnapshotEnabled {
-				return compatibility.FindCompatibilityLayer(shareOpts.Protocol, capabilities.ManilaCapabilityShareFromSnapshot, shareTypeCaps)
-			}
-		}
-	}
-
-	return nil
-}
-
 //
 // Controller service request validation
 //

pkg/csi/manila/volumesource.go

@@ -67,11 +67,6 @@ type volumeFromSnapshot struct{}
 func (volumeFromSnapshot) create(req *csi.CreateVolumeRequest, shareName string, sizeInGiB int, manilaClient manilaclient.Interface, shareOpts *options.ControllerVolumeContext, shareMetadata map[string]string) (*shares.Share, error) {
 	snapshotSource := req.GetVolumeContentSource().GetSnapshot()
 
-	if shareOpts.Protocol == "CEPHFS" {
-		// TODO: Restoring shares from CephFS snapshots needs special handling.
-		return nil, status.Errorf(codes.InvalidArgument, "restoring CephFS snapshots is not supported yet")
-	}
-
 	if snapshotSource.GetSnapshotId() == "" {
 		return nil, status.Error(codes.InvalidArgument, "snapshot ID cannot be empty")
 	}