diff --git a/pkg/linode-bs/controllerserver.go b/pkg/linode-bs/controllerserver.go index 7b4ed7c6..4e59f521 100644 --- a/pkg/linode-bs/controllerserver.go +++ b/pkg/linode-bs/controllerserver.go @@ -258,19 +258,34 @@ func (linodeCS *LinodeControllerServer) ControllerPublishVolume(ctx context.Cont return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("Volume with id %d already attached to node %d", volumeID, *volume.LinodeID)) } - if _, err := linodeCS.CloudProvider.GetInstance(ctx, linodeID); err != nil { - if apiErr, ok := err.(*linodego.Error); ok && apiErr.Code == 404 { - return nil, status.Error(codes.NotFound, fmt.Sprintf("Linode with id %d not found", linodeID)) - } + instance, err := linodeCS.CloudProvider.GetInstance(ctx, linodeID) + if err, ok := err.(*linodego.Error); ok && err.Code == 404 { + return nil, status.Errorf(codes.NotFound, "Linode with id %d not found", linodeID) + } else if err != nil { return nil, status.Error(codes.Internal, err.Error()) } - opts := &linodego.VolumeAttachOptions{ - LinodeID: linodeID, - ConfigID: 0, + // Check to see if there is room to attach this volume to the instance. + if canAttach, err := linodeCS.canAttach(ctx, instance); err != nil { + return &csi.ControllerPublishVolumeResponse{}, status.Error(codes.Internal, err.Error()) + } else if !canAttach { + limit := linodeCS.maxVolumeAttachments(instance) + return &csi.ControllerPublishVolumeResponse{}, status.Errorf(codes.ResourceExhausted, "max number of volumes (%d) already attached to instance", limit) } - if _, err := linodeCS.CloudProvider.AttachVolume(ctx, volumeID, opts); err != nil { + // Whether or not the volume attachment should be persisted across + // boots. + // + // Setting this to true will limit the maximum number of attached + // volumes to 8 (eight), minus any instance disks, since volume + // attachments get persisted by adding them to the instance's boot + // config. + persist := false + + if _, err := linodeCS.CloudProvider.AttachVolume(ctx, volumeID, &linodego.VolumeAttachOptions{ + LinodeID: linodeID, + PersistAcrossBoots: &persist, + }); err != nil { retCode := codes.Internal if apiErr, ok := err.(*linodego.Error); ok && strings.Contains(apiErr.Message, "is already attached") { retCode = codes.Unavailable // Allow a retry if the volume is already attached: race condition can occur here @@ -289,6 +304,47 @@ func (linodeCS *LinodeControllerServer) ControllerPublishVolume(ctx context.Cont return &csi.ControllerPublishVolumeResponse{PublishContext: pvInfo}, nil } +// 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 Linode +// instance. +func (s *LinodeControllerServer) canAttach(ctx context.Context, instance *linodego.Instance) (canAttach bool, err error) { + // Total number of attached drives. + var attached int + + disks, err := s.CloudProvider.ListInstanceDisks(ctx, instance.ID, nil) + if err != nil { + return false, fmt.Errorf("list instance disks: %w", err) + } + attached += len(disks) + + volumes, err := s.CloudProvider.ListInstanceVolumes(ctx, instance.ID, nil) + if err != nil { + return false, fmt.Errorf("list instance volumes: %w", err) + } + attached += len(volumes) + + limit := s.maxVolumeAttachments(instance) + return attached < limit, nil +} + +// maxVolumeAttachments returns the maximum number of volumes that can be +// attached to a single Linode instance. If instance == nil, or instance.Specs +// == nil, [maxPersistentAttachments] will be returned. +func (s *LinodeControllerServer) maxVolumeAttachments(instance *linodego.Instance) int { + if instance == nil || instance.Specs == nil { + return maxPersistentAttachments + } + + // The reported amount of memory for an instance is in MB. + // Convert it to bytes. + memBytes := uint(instance.Specs.Memory) << 20 + + return maxVolumeAttachments(memBytes) +} + // ControllerUnpublishVolume deattaches the given volume from the node func (linodeCS *LinodeControllerServer) ControllerUnpublishVolume(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) { volumeID, statusErr := common.VolumeIdAsInt("ControllerUnpublishVolume", req) @@ -509,7 +565,6 @@ func (linodeCS *LinodeControllerServer) ControllerExpandVolume(ctx context.Conte } vol, err = linodeCS.CloudProvider.WaitForVolumeStatus(ctx, vol.ID, linodego.VolumeActive, waitTimeout) - if err != nil { return nil, status.Error(codes.Internal, err.Error()) } @@ -522,12 +577,12 @@ func (linodeCS *LinodeControllerServer) ControllerExpandVolume(ctx context.Conte } klog.V(4).Info("volume is resized") return resp, nil - } // attemptGetContentSourceVolume attempts to get information about the Linode volume to clone from. func (linodeCS *LinodeControllerServer) attemptGetContentSourceVolume( - ctx context.Context, contentSource *csi.VolumeContentSource) (*common.LinodeVolumeKey, error) { + ctx context.Context, contentSource *csi.VolumeContentSource, +) (*common.LinodeVolumeKey, error) { // No content source was defined; no clone operation if contentSource == nil { return nil, nil @@ -598,8 +653,8 @@ func (linodeCS *LinodeControllerServer) attemptCreateLinodeVolume( // createLinodeVolume creates a Linode volume and returns the result func (linodeCS *LinodeControllerServer) createLinodeVolume( - ctx context.Context, label string, sizeGB int, tags string) (*linodego.Volume, error) { - + ctx context.Context, label string, sizeGB int, tags string, +) (*linodego.Volume, error) { volumeReq := linodego.VolumeCreateOptions{ Region: linodeCS.MetadataService.GetZone(), Label: label, @@ -625,7 +680,8 @@ func (linodeCS *LinodeControllerServer) createLinodeVolume( // cloneLinodeVolume clones a Linode volume and returns the result func (linodeCS *LinodeControllerServer) cloneLinodeVolume( - ctx context.Context, label string, sizeGB, sourceID int) (*linodego.Volume, error) { + ctx context.Context, label string, sizeGB, sourceID int, +) (*linodego.Volume, error) { klog.V(4).Infoln("cloning volume", map[string]interface{}{ "source_vol_id": sourceID, }) diff --git a/pkg/linode-bs/controllerserver_test.go b/pkg/linode-bs/controllerserver_test.go index cb3a0f4d..092a54e7 100644 --- a/pkg/linode-bs/controllerserver_test.go +++ b/pkg/linode-bs/controllerserver_test.go @@ -8,6 +8,7 @@ import ( "github.com/container-storage-interface/spec/lib/go/csi" "github.com/linode/linode-blockstorage-csi-driver/pkg/common" + linodeclient "github.com/linode/linode-blockstorage-csi-driver/pkg/linode-client" "github.com/linode/linodego" ) @@ -179,37 +180,52 @@ func TestListVolumes(t *testing.T) { } } }) - } - } +var _ linodeclient.LinodeClient = &fakeLinodeClient{} + type fakeLinodeClient struct { volumes []linodego.Volume + disks []linodego.InstanceDisk throwErr bool } func (flc *fakeLinodeClient) ListInstances(context.Context, *linodego.ListOptions) ([]linodego.Instance, error) { return nil, nil } + func (flc *fakeLinodeClient) ListVolumes(context.Context, *linodego.ListOptions) ([]linodego.Volume, error) { if flc.throwErr { return nil, errors.New("sad times mate") } return flc.volumes, nil } + +func (c *fakeLinodeClient) ListInstanceVolumes(_ context.Context, _ int, _ *linodego.ListOptions) ([]linodego.Volume, error) { + return c.volumes, nil +} + +func (c *fakeLinodeClient) ListInstanceDisks(_ context.Context, _ int, _ *linodego.ListOptions) ([]linodego.InstanceDisk, error) { + return c.disks, nil +} + func (flc *fakeLinodeClient) GetInstance(context.Context, int) (*linodego.Instance, error) { return nil, nil } + func (flc *fakeLinodeClient) GetVolume(context.Context, int) (*linodego.Volume, error) { return nil, nil } + func (flc *fakeLinodeClient) CreateVolume(context.Context, linodego.VolumeCreateOptions) (*linodego.Volume, error) { return nil, nil } + func (flc *fakeLinodeClient) CloneVolume(context.Context, int, string) (*linodego.Volume, error) { return nil, nil } + func (flc *fakeLinodeClient) AttachVolume(context.Context, int, *linodego.VolumeAttachOptions) (*linodego.Volume, error) { return nil, nil } @@ -217,6 +233,7 @@ func (flc *fakeLinodeClient) DetachVolume(context.Context, int) error { return n func (flc *fakeLinodeClient) WaitForVolumeLinodeID(context.Context, int, *int, int) (*linodego.Volume, error) { return nil, nil } + func (flc *fakeLinodeClient) WaitForVolumeStatus(context.Context, int, linodego.VolumeStatus, int) (*linodego.Volume, error) { return nil, nil } @@ -229,3 +246,170 @@ func (flc *fakeLinodeClient) NewEventPoller(context.Context, any, linodego.Entit 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 := LinodeControllerServer{ + CloudProvider: &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) + } + }) + } +} + +// testMetadata is an implementation of pkg/metadata.MetadataService that can +// be used by the [LinodeControllerServer] to retrieve metadata about the +// current Linode instance. +// +//nolint:unused +type testMetadata struct { + region string + nodeID int + label string + project string + memory uint // Amount of memory, in bytes +} + +//nolint:unused +func (m *testMetadata) GetZone() string { return m.region } + +//nolint:unused +func (m *testMetadata) GetProject() string { return m.project } + +//nolint:unused +func (m *testMetadata) GetName() string { return m.label } + +//nolint:unused +func (m *testMetadata) GetNodeID() int { return m.nodeID } + +//nolint:unused +func (m *testMetadata) Memory() uint { return m.memory } + +func TestControllerMaxVolumeAttachments(t *testing.T) { + tests := []struct { + name string + instance *linodego.Instance + want int + }{ + { + name: "NilInstance", + want: maxPersistentAttachments, + }, + { + name: "NilInstanceSpecs", + instance: &linodego.Instance{}, + want: maxPersistentAttachments, + }, + { + name: "1GB", + instance: &linodego.Instance{ + Specs: &linodego.InstanceSpec{Memory: 1 << 10}, + }, + want: maxPersistentAttachments, + }, + { + name: "16GB", + instance: &linodego.Instance{ + Specs: &linodego.InstanceSpec{Memory: 16 << 10}, + }, + want: 16, + }, + { + name: "32GB", + instance: &linodego.Instance{ + Specs: &linodego.InstanceSpec{Memory: 32 << 10}, + }, + want: 32, + }, + { + name: "64GB", + instance: &linodego.Instance{ + Specs: &linodego.InstanceSpec{Memory: 64 << 10}, + }, + want: maxAttachments, + }, + { + name: "96GB", + instance: &linodego.Instance{ + Specs: &linodego.InstanceSpec{Memory: 96 << 10}, + }, + want: maxAttachments, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &LinodeControllerServer{} + got := s.maxVolumeAttachments(tt.instance) + if got != tt.want { + t.Errorf("got=%d want=%d", got, tt.want) + } + }) + } +} diff --git a/pkg/linode-bs/limits.go b/pkg/linode-bs/limits.go new file mode 100644 index 00000000..faad94ac --- /dev/null +++ b/pkg/linode-bs/limits.go @@ -0,0 +1,31 @@ +package linodebs + +// maxVolumeAttachments returns the maximum number of block storage volumes +// that can be attached to a Linode instance, given the amount of memory the +// instance has. +// +// TODO: This code should be cleaned up to use the built-in max and min +// functions once the project is updated to Go 1.21. See +// https://go.dev/ref/spec#Min_and_max. +func maxVolumeAttachments(memoryBytes uint) int { + attachments := memoryBytes >> 30 + if attachments > maxAttachments { + return maxAttachments + } + if attachments < maxPersistentAttachments { + return maxPersistentAttachments + } + return int(attachments) +} + +const ( + // maxPersistentAttachments is the default number of volume attachments + // allowed when they are persisted to an instance/boot config. This is + // also the maximum number of allowed volume attachments when the + // instance type has < 16GiB of RAM. + maxPersistentAttachments = 8 + + // maxAttachments it the hard limit of volumes that can be attached to + // a single Linode instance. + maxAttachments = 64 +) diff --git a/pkg/linode-bs/limits_test.go b/pkg/linode-bs/limits_test.go new file mode 100644 index 00000000..2a698532 --- /dev/null +++ b/pkg/linode-bs/limits_test.go @@ -0,0 +1,39 @@ +package linodebs + +import ( + "fmt" + "testing" +) + +func TestMaxVolumeAttachments(t *testing.T) { + t.Parallel() + + tests := []struct { + memory uint + want int + }{ + {memory: 1 << 30, want: maxPersistentAttachments}, + {memory: 2 << 30, want: maxPersistentAttachments}, + {memory: 4 << 30, want: maxPersistentAttachments}, + {memory: 8 << 30, want: maxPersistentAttachments}, + {memory: 16 << 30, want: 16}, + {memory: 32 << 30, want: 32}, + {memory: 64 << 30, want: maxAttachments}, + {memory: 96 << 30, want: maxAttachments}, + {memory: 128 << 30, want: maxAttachments}, + {memory: 150 << 30, want: maxAttachments}, + {memory: 256 << 30, want: maxAttachments}, + {memory: 300 << 30, want: maxAttachments}, + {memory: 512 << 30, want: maxAttachments}, + } + + for _, tt := range tests { + tname := fmt.Sprintf("%dGB", tt.memory>>30) + t.Run(tname, func(t *testing.T) { + got := maxVolumeAttachments(tt.memory) + if got != tt.want { + t.Errorf("want=%d got=%d", tt.want, got) + } + }) + } +} diff --git a/pkg/linode-bs/nodeserver.go b/pkg/linode-bs/nodeserver.go index 44627ff2..d86e3703 100644 --- a/pkg/linode-bs/nodeserver.go +++ b/pkg/linode-bs/nodeserver.go @@ -35,10 +35,6 @@ import ( "k8s.io/utils/mount" ) -const ( - maxVolumesPerNode = 7 -) - type LinodeNodeServer struct { Driver *LinodeDriver Mounter *mount.SafeFormatAndMount @@ -246,7 +242,6 @@ func (ns *LinodeNodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeSt deviceName := key.GetNormalizedLabel() devicePaths := ns.DeviceUtils.GetDiskByIdPaths(deviceName, partition) devicePath, err := ns.DeviceUtils.VerifyDevicePath(devicePaths) - if err != nil { return nil, status.Error(codes.Internal, fmt.Sprintf("Error verifying Linode Volume (%q) is attached: %v", key.GetVolumeLabel(), err)) } @@ -409,11 +404,10 @@ func (ns *LinodeNodeServer) NodeGetInfo(ctx context.Context, req *csi.NodeGetInf resp := &csi.NodeGetInfoResponse{ NodeId: strconv.Itoa(nodeID), - MaxVolumesPerNode: maxVolumesPerNode, + MaxVolumesPerNode: int64(maxVolumeAttachments(ns.MetadataService.Memory())), AccessibleTopology: top, } return resp, nil - } func (ns *LinodeNodeServer) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) { diff --git a/pkg/linode-client/linode-client.go b/pkg/linode-client/linode-client.go index 9f3f2d4c..f6f2b0c5 100644 --- a/pkg/linode-client/linode-client.go +++ b/pkg/linode-client/linode-client.go @@ -12,6 +12,8 @@ import ( type LinodeClient interface { ListInstances(context.Context, *linodego.ListOptions) ([]linodego.Instance, error) // Needed for metadata ListVolumes(context.Context, *linodego.ListOptions) ([]linodego.Volume, error) + ListInstanceVolumes(ctx context.Context, instanceID int, options *linodego.ListOptions) ([]linodego.Volume, error) + ListInstanceDisks(ctx context.Context, instanceID int, options *linodego.ListOptions) ([]linodego.InstanceDisk, error) GetInstance(context.Context, int) (*linodego.Instance, error) GetVolume(context.Context, int) (*linodego.Volume, error) diff --git a/pkg/metadata/metadata.go b/pkg/metadata/metadata.go index 3eac593f..85fb00a2 100644 --- a/pkg/metadata/metadata.go +++ b/pkg/metadata/metadata.go @@ -17,6 +17,7 @@ type MetadataService interface { GetProject() string GetName() string GetNodeID() int + Memory() uint } type metadataServiceManager struct { @@ -25,6 +26,7 @@ type metadataServiceManager struct { nodeID int label string project string + memory uint // Amount of memory, in bytes } var _ MetadataService = &metadataServiceManager{} @@ -61,13 +63,26 @@ func NewMetadataService(linodeClient linodeclient.LinodeClient, nodeName string) } } + // Figure out how much memory this instance has. The API returns the + // amount of memory in MB (megabytes, not mebibytes) and even then, it + // is a strange number. + // + // If we cannot determine how much memory the instance has, we are + // simply going to assume 1GiB, which is the smallest amount of memory + // available across all instance types as of 2024-05-16. + var memory uint = 1 << 30 + if linode.Specs != nil { + // Store the amount of memory as number of bytes. + memory = uint(linode.Specs.Memory) << 20 + } + return &metadataServiceManager{ region: linode.Region, nodeID: linode.ID, label: linode.Label, project: linode.Group, + memory: memory, }, nil - } func getLinodeByID(client linodeclient.LinodeClient, id string) (*linodego.Instance, error) { @@ -96,18 +111,8 @@ func getLinodeByLabel(client linodeclient.LinodeClient, label string) (*linodego return nil, errors.New("User has no Linode instances with the given label") } -func (manager *metadataServiceManager) GetZone() string { - return manager.region -} - -func (manager *metadataServiceManager) GetProject() string { - return manager.project -} - -func (manager *metadataServiceManager) GetName() string { - return manager.label -} - -func (manager *metadataServiceManager) GetNodeID() int { - return manager.nodeID -} +func (manager *metadataServiceManager) GetZone() string { return manager.region } +func (manager *metadataServiceManager) GetProject() string { return manager.project } +func (manager *metadataServiceManager) GetName() string { return manager.label } +func (manager *metadataServiceManager) GetNodeID() int { return manager.nodeID } +func (m *metadataServiceManager) Memory() uint { return m.memory }