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..89a59f31 100644 --- a/internal/driver/controllerserver_helper.go +++ b/internal/driver/controllerserver_helper.go @@ -95,31 +95,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 +652,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,11 +667,17 @@ func (cs *ControllerServer) attachVolume(ctx context.Context, volumeID, linodeID PersistAcrossBoots: &persist, }) if err != nil { + // https://github.com/container-storage-interface/spec/blob/master/spec.md#controllerpublishvolume-errors code := codes.Internal // Default error code is Internal. // Check if the error indicates that the volume is already attached. 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"): + code = codes.FailedPrecondition // Allow a retry if the volume is already attached: race condition can occur here + case strings.Contains(apiErr.Message, "Maximum number of block storage volumes are attached to this Linode"): + code = codes.ResourceExhausted + } } return status.Errorf(code, "attach volume: %v", err) } diff --git a/internal/driver/controllerserver_helper_test.go b/internal/driver/controllerserver_helper_test.go index 39184a18..539b9b21 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() diff --git a/internal/driver/controllerserver_test.go b/internal/driver/controllerserver_test.go index b3741739..48726582 100644 --- a/internal/driver/controllerserver_test.go +++ b/internal/driver/controllerserver_test.go @@ -679,82 +679,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