cephfs: address review feedback on MDS pinning

Handle several review comments on the VolumeAttributesClass MDS pinning
support:

- Purge the freshly created subvolume when applying the mutable
  parameters fails in CreateVolume, so a failed pin does not leave an
  orphaned subvolume behind. The already-existing subvolume path is left
  untouched on failure.
- Reject unknown mutable parameters in validateMDSPinParameters and
  return InvalidArgument, as recommended by the CSI spec for unsupported
  parameters.
- Return NotFound (instead of InvalidArgument) from
  ControllerModifyVolume when the volume backing the given ID cannot be
  found, and Internal for other resolution errors.
- Log the manager command status and output returned when pinning a
  subvolume.
- Clarify the PinVolume documentation to describe the valid pin setting
  value for each pin type.

Assisted-by: goose <noreply@block.xyz>
Signed-off-by: Ismael Puerto Freire <ismaelpf@inditex.com>

Author: ismaelpuerto

internal/cephfs/controllerserver.go

@@ -413,7 +413,8 @@ func (cs *cephfsControllerServer) CreateVolume(
 		}
 
 		// apply MutableParameters (e.g. MDS pinning) from a
-		// VolumeAttributesClass, if any were requested.
+		// VolumeAttributesClass, if any were requested. The subvolume already
+		// existed before this request, so it must not be purged on failure.
 		if len(req.GetMutableParameters()) != 0 {
 			if err = applyMutableParameters(ctx, volClient, vID.FsSubvolName,
 				req.GetMutableParameters()); err != nil {
@@ -496,10 +497,17 @@ func (cs *cephfsControllerServer) CreateVolume(
 		vID.FsSubvolName, requestName)
 
 	// apply MutableParameters (e.g. MDS pinning) from a VolumeAttributesClass,
-	// if any were requested.
+	// if any were requested. On failure the freshly created subvolume must be
+	// purged to avoid leaving an orphaned subvolume behind (the deferred
+	// UndoVolReservation only cleans up the reservation/OMAP entry).
 	if len(req.GetMutableParameters()) != 0 {
 		if err = applyMutableParameters(ctx, volClient, vID.FsSubvolName,
 			req.GetMutableParameters()); err != nil {
+			if purgeErr := volClient.PurgeVolume(ctx, true); purgeErr != nil {
+				log.ErrorLog(ctx, "failed to purge subvolume %s after applying mutable "+
+					"parameters failed: %v", vID.FsSubvolName, purgeErr)
+			}
+
 			return nil, err
 		}
 	}
@@ -1514,17 +1522,25 @@ var mdsPinParams = []string{paramMDSPinExport, paramMDSPinDistributed, paramMDSP
 // returns the CephFS pin type and setting to apply.
 //
 // The three pin parameters (mds-pin-export, mds-pin-distributed,
-// mds-pin-random) are mutually exclusive: at most one may be set. It returns:
+// mds-pin-random) are mutually exclusive: at most one may be set. Any parameter
+// key that is not a recognized MDS pin parameter is rejected, as recommended by
+// the CSI spec for unsupported mutable parameters. It returns:
 //   - ("", "", nil)               when no MDS pin parameter is present
 //   - (pinType, pinSetting, nil)  when exactly one is present with a valid value
-//   - ("", "", err)               when more than one is present, or the value
-//     is invalid for the given pin type
+//   - ("", "", err)               when an unknown parameter is present, more
+//     than one is present, or the value is invalid for the given pin type
 func validateMDSPinParameters(params map[string]string) (string, string, error) {
 	var (
 		selected string
 		setting  string
 	)
 
+	for key := range params {
+		if _, ok := mdsPinParamToType[key]; !ok {
+			return "", "", fmt.Errorf("unsupported mutable parameter %q", key)
+		}
+	}
+
 	for _, key := range mdsPinParams {
 		value, ok := params[key]
 		if !ok {
@@ -1663,7 +1679,13 @@ func (cs *cephfsControllerServer) ControllerModifyVolume(
 	if err != nil {
 		log.ErrorLog(ctx, "validation and extraction of volume options failed: %v", err)
 
-		return nil, status.Error(codes.InvalidArgument, err.Error())
+		// the volume backing the given ID could not be found.
+		if errors.Is(err, cerrors.ErrVolumeNotFound) || errors.Is(err, util.ErrKeyNotFound) ||
+			errors.Is(err, util.ErrPoolNotFound) {
+			return nil, status.Error(codes.NotFound, err.Error())
+		}
+
+		return nil, status.Error(codes.Internal, err.Error())
 	}
 	defer volOptions.Destroy()
 

internal/cephfs/controllerserver_test.go

@@ -40,11 +40,17 @@ func TestValidateMDSPinParameters(t *testing.T) {
 			wantErr:     false,
 		},
 		{
-			name:        "unrelated parameters only",
-			params:      map[string]string{"foo": "bar"},
-			wantType:    "",
-			wantSetting: "",
-			wantErr:     false,
+			name:    "unrelated parameters only",
+			params:  map[string]string{"foo": "bar"},
+			wantErr: true,
+		},
+		{
+			name: "unknown parameter mixed with a valid pin",
+			params: map[string]string{
+				paramMDSPinExport: "1",
+				"foo":             "bar",
+			},
+			wantErr: true,
 		},
 		{
 			name:        "export pin with valid rank",

internal/cephfs/core/volume.go

@@ -91,8 +91,11 @@ type SubVolumeClient interface {
 	ListMetadata() (map[string]string, error)
 
 	// PinVolume sets an MDS pinning policy on the subvolume.
-	// pinType can be "export", "distributed" or "random".
-	// pinSetting is the value for the pin (e.g. MDS rank "2", "true", etc.).
+	// pinType is one of "export", "distributed" or "random".
+	// pinSetting is the value for the pin, its meaning depends on pinType:
+	//   - export:      MDS rank as an integer (e.g. "2"), or "-1" to unpin
+	//   - distributed: "1" to enable or "0" to disable
+	//   - random:      a float in the range 0.0-1.0
 	PinVolume(ctx context.Context, pinType, pinSetting string) error
 }
 
@@ -359,11 +362,14 @@ func (s *subVolumeClient) PinVolume(ctx context.Context, pinType, pinSetting str
 		return fmt.Errorf("failed to marshal pin command for subvolume %s: %w", s.VolID, err)
 	}
 
-	_, _, err = s.conn.MgrCommand([][]byte{buf})
+	out, status, err := s.conn.MgrCommand([][]byte{buf})
 	if err != nil {
 		return fmt.Errorf("failed to pin subvolume %s in fs %s (group %s): %w",
 			s.VolID, s.FsName, s.SubvolumeGroup, err)
 	}
 
+	log.DebugLog(ctx, "cephfs: pinned subvolume %s in fs %s (group %s), type=%s setting=%s: status=%q output=%q",
+		s.VolID, s.FsName, s.SubvolumeGroup, pinType, pinSetting, status, string(out))
+
 	return nil
 }