diff --git a/internal/driver/controllerserver.go b/internal/driver/controllerserver.go index 8460d3cf..995b1fe3 100644 --- a/internal/driver/controllerserver.go +++ b/internal/driver/controllerserver.go @@ -202,12 +202,6 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs }, nil } - // Check if the instance can accommodate the volume attachment - if capErr := cs.checkAttachmentCapacity(ctx, instance); capErr != nil { - metrics.RecordMetrics(metrics.ControllerPublishVolumeTotal, metrics.ControllerPublishVolumeDuration, metrics.Failed, functionStartTime) - return resp, capErr - } - // Attach the volume to the specified instance if attachErr := cs.attachVolume(ctx, volumeID, linodeID); attachErr != nil { metrics.RecordMetrics(metrics.ControllerPublishVolumeTotal, metrics.ControllerPublishVolumeDuration, metrics.Failed, functionStartTime) diff --git a/internal/driver/controllerserver_helper.go b/internal/driver/controllerserver_helper.go index 251b6877..5a99bde6 100644 --- 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" @@ -95,31 +93,6 @@ type VolumeParams struct { Region string } -// canAttach indicates whether or not another volume can be attached to the -// Linode with the given ID. -// -// Whether or not another volume can be attached is based on how many instance -// disks and block storage volumes are currently attached to the instance. -func (cs *ControllerServer) canAttach(ctx context.Context, instance *linodego.Instance) (canAttach bool, err error) { - log := logger.GetLogger(ctx) - log.V(4).Info("Checking if volume can be attached", "instance_id", instance.ID) - - // Get the maximum number of volume attachments allowed for the instance - limit, err := cs.maxAllowedVolumeAttachments(ctx, instance) - if err != nil { - return false, err - } - - // List the volumes currently attached to the instance - volumes, err := cs.client.ListInstanceVolumes(ctx, instance.ID, nil) - if err != nil { - return false, errInternal("list instance volumes: %v", err) - } - - // Return true if the number of attached volumes is less than the limit - return len(volumes) < limit, nil -} - // maxAllowedVolumeAttachments calculates the maximum number of volumes that can be attached to a Linode instance, // taking into account the instance's memory and currently attached disks. func (cs *ControllerServer) maxAllowedVolumeAttachments(ctx context.Context, instance *linodego.Instance) (int, error) { @@ -677,33 +650,6 @@ func (cs *ControllerServer) getInstance(ctx context.Context, linodeID int) (*lin return instance, nil } -// checkAttachmentCapacity checks if the specified instance can accommodate -// additional volume attachments. It retrieves the maximum number of allowed -// attachments and compares it with the currently attached volumes. If the -// limit is exceeded, it returns an error indicating the maximum volume -// attachments allowed. -func (cs *ControllerServer) checkAttachmentCapacity(ctx context.Context, instance *linodego.Instance) error { - log := logger.GetLogger(ctx) - log.V(4).Info("Entering checkAttachmentCapacity()", "linodeID", instance.ID) - defer log.V(4).Info("Exiting checkAttachmentCapacity()") - - canAttach, err := cs.canAttach(ctx, instance) - if err != nil { - return err - } - if !canAttach { - // If the instance cannot accommodate more attachments, retrieve the maximum allowed attachments. - limit, err := cs.maxAllowedVolumeAttachments(ctx, instance) - if errors.Is(err, errNilInstance) { - return errInternal("cannot calculate max volume attachments for a nil instance") - } else if err != nil { - return errMaxAttachments // Return an error indicating the maximum attachments limit has been reached. - } - return errMaxVolumeAttachments(limit) // Return an error indicating the maximum volume attachments allowed. - } - return nil // Return nil if the instance can accommodate more attachments. -} - // attachVolume attaches the specified volume to the given Linode instance. // It logs the action and handles any errors that may occur during the // attachment process. If the volume is already attached, it allows for a @@ -719,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 index 39184a18..ac4821a3 100644 --- a/internal/driver/controllerserver_helper_test.go +++ b/internal/driver/controllerserver_helper_test.go @@ -933,64 +933,6 @@ func TestGetAndValidateVolume(t *testing.T) { } } -func TestCheckAttachmentCapacity(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockClient := mocks.NewMockLinodeClient(ctrl) - cs := &ControllerServer{ - client: mockClient, - } - - testCases := []struct { - name string - instance *linodego.Instance - setupMocks func() - expectedError error - }{ - { - name: "Can attach volume", - instance: &linodego.Instance{ - ID: 123, - Specs: &linodego.InstanceSpec{ - Memory: 4096, - }, - }, - setupMocks: func() { - mockClient.EXPECT().ListInstanceVolumes(gomock.Any(), 123, gomock.Any()).Return([]linodego.Volume{}, nil) - mockClient.EXPECT().ListInstanceDisks(gomock.Any(), 123, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) - }, - expectedError: nil, - }, - { - name: "Cannot attach volume - max attachments reached", - instance: &linodego.Instance{ - ID: 456, - Specs: &linodego.InstanceSpec{ - Memory: 1024, - }, - }, - setupMocks: func() { - mockClient.EXPECT().ListInstanceDisks(gomock.Any(), 456, gomock.Any()).Return([]linodego.InstanceDisk{{ID: 1}, {ID: 2}}, nil).AnyTimes() - mockClient.EXPECT().ListInstanceVolumes(gomock.Any(), 456, gomock.Any()).Return([]linodego.Volume{{ID: 1}, {ID: 2}, {ID: 3}, {ID: 4}, {ID: 5}, {ID: 6}}, nil) - }, - expectedError: errMaxVolumeAttachments(6), - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - tc.setupMocks() - - err := cs.checkAttachmentCapacity(context.Background(), tc.instance) - - if err != nil && !reflect.DeepEqual(tc.expectedError, err) { - t.Errorf("expected error %v, got %v", tc.expectedError, err) - } - }) - } -} - func TestGetContentSourceVolume(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -1169,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", @@ -1178,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/controllerserver_test.go b/internal/driver/controllerserver_test.go index b3741739..d0897430 100644 --- a/internal/driver/controllerserver_test.go +++ b/internal/driver/controllerserver_test.go @@ -215,8 +215,6 @@ func TestControllerPublishVolume(t *testing.T) { m.EXPECT().GetVolume(gomock.Any(), gomock.Any()).Return(&linodego.Volume{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil).AnyTimes() m.EXPECT().WaitForVolumeLinodeID(gomock.Any(), 630706045, gomock.Any(), gomock.Any()).Return(&linodego.Volume{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil) m.EXPECT().AttachVolume(gomock.Any(), 630706045, gomock.Any()).Return(&linodego.Volume{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}, nil) - m.EXPECT().ListInstanceVolumes(gomock.Any(), 1003, gomock.Any()).Return([]linodego.Volume{{ID: 1001, LinodeID: createLinodeID(1003), Size: 10, Status: linodego.VolumeActive}}, nil) - m.EXPECT().ListInstanceDisks(gomock.Any(), 1003, gomock.Any()).Return([]linodego.InstanceDisk{}, nil) }, expectedError: nil, }, @@ -679,82 +677,6 @@ func createLinodeID(i int) *int { return &i } -func TestControllerCanAttach(t *testing.T) { - t.Parallel() - - tests := []struct { - memory uint // memory in bytes - nvols int // number of volumes already attached - ndisks int // number of attached disks - want bool // can we attach another? - fail bool // should we expect a non-nil error - }{ - { - memory: 1 << 30, // 1GiB - nvols: 7, // maxed out - ndisks: 1, - }, - { - memory: 16 << 30, // 16GiB - nvols: 14, // should allow one more - ndisks: 1, - want: true, - }, - { - memory: 16 << 30, - nvols: 15, - ndisks: 1, - }, - { - memory: 256 << 30, // 256GiB - nvols: 64, // maxed out - }, - } - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - for _, tt := range tests { - tname := fmt.Sprintf("%dGB-%d", tt.memory>>30, tt.nvols) - t.Run(tname, func(t *testing.T) { - vols := make([]linodego.Volume, 0, tt.nvols) - for i := 0; i < tt.nvols; i++ { - vols = append(vols, linodego.Volume{ID: i}) - } - - disks := make([]linodego.InstanceDisk, 0, tt.ndisks) - for i := 0; i < tt.ndisks; i++ { - disks = append(disks, linodego.InstanceDisk{ID: i}) - } - - memMB := 8192 - if tt.memory != 0 { - memMB = int(tt.memory >> 20) // convert bytes -> MB - } - instance := &linodego.Instance{ - Specs: &linodego.InstanceSpec{Memory: memMB}, - } - - srv := ControllerServer{ - client: &fakeLinodeClient{ - volumes: vols, - disks: disks, - }, - } - - got, err := srv.canAttach(ctx, instance) - if err != nil && !tt.fail { - t.Fatal(err) - } else if err == nil && tt.fail { - t.Fatal("should have failed") - } - - if got != tt.want { - t.Errorf("got=%t want=%t", got, tt.want) - } - }) - } -} - func TestControllerMaxVolumeAttachments(t *testing.T) { tests := []struct { name string diff --git a/internal/driver/errors.go b/internal/driver/errors.go index c043df8e..e25b6d6f 100644 --- 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) }