feat: report RESOURCE_EXHAUSTED when max volume

Also fix already mount error code
linode · Nov 25, 2024 · efdefae · efdefae
1 parent 154621f
commit efdefae
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 13 deletions.
diff --git a/internal/driver/controllerserver_helper.go b/internal/driver/controllerserver_helper.go
@@ -10,8 +10,6 @@ import (
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/linode/linodego"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 
 	linodevolumes "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-volumes"
 	"github.com/linode/linode-blockstorage-csi-driver/pkg/logger"
@@ -667,13 +665,17 @@ func (cs *ControllerServer) attachVolume(ctx context.Context, volumeID, linodeID
 		PersistAcrossBoots: &persist,
 	})
 	if err != nil {
-		code := codes.Internal // Default error code is Internal.
-		// Check if the error indicates that the volume is already attached.
+		// https://github.com/container-storage-interface/spec/blob/master/spec.md#controllerpublishvolume-errors
 		var apiErr *linodego.Error
-		if errors.As(err, &apiErr) && strings.Contains(apiErr.Message, "is already attached") {
-			code = codes.Unavailable // Allow a retry if the volume is already attached: race condition can occur here
+		if errors.As(err, &apiErr) {
+			switch {
+			case strings.Contains(apiErr.Message, "is already attached"):
+				return errAlreadyAttached
+			case strings.Contains(apiErr.Message, "Maximum number of block storage volumes are attached to this Linode"):
+				return errMaxAttachments
+			}
 		}
-		return status.Errorf(code, "attach volume: %v", err)
+		return errInternal("attach volume: %v", err)
 	}
 	return nil // Return nil if the volume is successfully attached.
 }
diff --git a/internal/driver/controllerserver_helper_test.go b/internal/driver/controllerserver_helper_test.go
@@ -1111,7 +1111,7 @@ func TestAttachVolume(t *testing.T) {
 			setupMocks: func() {
 				mockClient.EXPECT().AttachVolume(gomock.Any(), 789, gomock.Any()).Return(nil, &linodego.Error{Message: "Volume 789 is already attached"})
 			},
-			expectedError: status.Error(codes.Unavailable, "attach volume: [000] Volume 789 is already attached"),
+			expectedError: errAlreadyAttached,
 		},
 		{
 			name:     "API error",
@@ -1120,7 +1120,7 @@ func TestAttachVolume(t *testing.T) {
 			setupMocks: func() {
 				mockClient.EXPECT().AttachVolume(gomock.Any(), 202, gomock.Any()).Return(nil, errors.New("API error"))
 			},
-			expectedError: status.Error(codes.Internal, "attach volume: API error"),
+			expectedError: errInternal("attach volume: API error"),
 		},
 	}
 

diff --git a/internal/driver/errors.go b/internal/driver/errors.go
@@ -34,6 +34,10 @@ var (
 	// attachments allowed for the instance, call errMaxVolumeAttachments.
 	errMaxAttachments = status.Error(codes.ResourceExhausted, "max number of volumes already attached to instance")
 
+	// errAlreadyAttached is used to indicate that a volume is already attached
+	// to a Linode instance.
+	errAlreadyAttached = status.Error(codes.FailedPrecondition, "volume is already attached")
+
 	// errResizeDown indicates a request would result in a volume being resized
 	// to be smaller than it currently is.
 	//
@@ -61,10 +65,6 @@ func errRegionMismatch(gotRegion, wantRegion string) error {
 	return status.Errorf(codes.InvalidArgument, "source volume is in region %q, needs to be in region %q", gotRegion, wantRegion)
 }
 
-func errMaxVolumeAttachments(numAttachments int) error {
-	return status.Errorf(codes.ResourceExhausted, "max number of volumes (%d) already attached to instance", numAttachments)
-}
-
 func errInstanceNotFound(linodeID int) error {
 	return status.Errorf(codes.NotFound, "linode instance %d not found", linodeID)
 }