Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add max number of volumes that can be attached to an instance #289

Merged
merged 2 commits into from
May 11, 2019
Merged

Add max number of volumes that can be attached to an instance #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5eafd95
Add instancey type to metadata
bertinatto May 5, 2019
6201e3b
Add max number of volumes that can be attached to an instance
bertinatto May 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/cloud/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ type EC2Metadata interface {
// MetadataService represents AWS metadata service.
type MetadataService interface {
GetInstanceID() string
GetInstanceType() string
GetRegion() string
GetAvailabilityZone() string
}

type Metadata struct {
InstanceID string
InstanceType string
Region string
AvailabilityZone string
}
Expand All @@ -47,6 +49,11 @@ func (m *Metadata) GetInstanceID() string {
return m.InstanceID
}

// GetInstanceID returns the instance type.
func (m *Metadata) GetInstanceType() string {
return m.InstanceType
}

// GetRegion returns the region which the instance is in.
func (m *Metadata) GetRegion() string {
return m.Region
Expand All @@ -72,6 +79,10 @@ func NewMetadataService(svc EC2Metadata) (MetadataService, error) {
return nil, fmt.Errorf("could not get valid EC2 instance ID")
}

if len(doc.InstanceType) == 0 {
return nil, fmt.Errorf("could not get valid EC2 instance type")
}

if len(doc.Region) == 0 {
return nil, fmt.Errorf("could not get valid EC2 region")
}
Expand All @@ -82,6 +93,7 @@ func NewMetadataService(svc EC2Metadata) (MetadataService, error) {

return &Metadata{
InstanceID: doc.InstanceID,
InstanceType: doc.InstanceType,
Region: doc.Region,
AvailabilityZone: doc.AvailabilityZone,
}, nil
Expand Down
11 changes: 11 additions & 0 deletions pkg/cloud/metadata_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

var (
stdInstanceID = "instance-1"
stdInstanceType = "t2.medium"
stdRegion = "instance-1"
stdAvailabilityZone = "az-1"
)
Expand All @@ -44,6 +45,7 @@ func TestNewMetadataService(t *testing.T) {
isAvailable: true,
identityDocument: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: stdInstanceID,
InstanceType: stdInstanceType,
Region: stdRegion,
AvailabilityZone: stdAvailabilityZone,
},
Expand All @@ -54,6 +56,7 @@ func TestNewMetadataService(t *testing.T) {
isAvailable: false,
identityDocument: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: stdInstanceID,
InstanceType: stdInstanceType,
Region: stdRegion,
AvailabilityZone: stdAvailabilityZone,
},
Expand All @@ -64,6 +67,7 @@ func TestNewMetadataService(t *testing.T) {
isAvailable: true,
identityDocument: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: stdInstanceID,
InstanceType: stdInstanceType,
Region: stdRegion,
AvailabilityZone: stdAvailabilityZone,
},
Expand All @@ -75,6 +79,7 @@ func TestNewMetadataService(t *testing.T) {
isPartial: true,
identityDocument: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: "",
InstanceType: stdInstanceType,
Region: stdRegion,
AvailabilityZone: stdAvailabilityZone,
},
Expand All @@ -86,6 +91,7 @@ func TestNewMetadataService(t *testing.T) {
isPartial: true,
identityDocument: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: stdInstanceID,
InstanceType: stdInstanceType,
Region: "",
AvailabilityZone: stdAvailabilityZone,
},
Expand All @@ -97,6 +103,7 @@ func TestNewMetadataService(t *testing.T) {
isPartial: true,
identityDocument: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: stdInstanceID,
InstanceType: stdInstanceType,
Region: stdRegion,
AvailabilityZone: "",
},
Expand Down Expand Up @@ -124,6 +131,10 @@ func TestNewMetadataService(t *testing.T) {
t.Fatalf("GetInstanceID() failed: expected %v, got %v", tc.identityDocument.InstanceID, m.GetInstanceID())
}

if m.GetInstanceType() != tc.identityDocument.InstanceType {
t.Fatalf("GetInstanceType() failed: expected %v, got %v", tc.identityDocument.InstanceType, m.GetInstanceType())
}

if m.GetRegion() != tc.identityDocument.Region {
t.Fatalf("GetRegion() failed: expected %v, got %v", tc.identityDocument.Region, m.GetRegion())
}
Expand Down
14 changes: 14 additions & 0 deletions pkg/driver/mocks/mock_metadata_service.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions pkg/driver/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"
"strings"

csi "github.com/container-storage-interface/spec/lib/go/csi"
Expand All @@ -44,6 +45,13 @@ const (

// default file system type to be used when it is not provided
defaultFsType = FSTypeExt4

// defaultMaxEBSVolumes is the maximum number of volumes that an AWS instance can have attached.
// More info at https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/volume_limits.html
defaultMaxEBSVolumes = 39

// defaultMaxEBSNitroVolumes is the limit of volumes for some smaller instances, like c5 and m5.
defaultMaxEBSNitroVolumes = 25
)

var (
Expand Down Expand Up @@ -316,6 +324,7 @@ func (d *nodeService) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoReque

return &csi.NodeGetInfoResponse{
NodeId: m.GetInstanceID(),
MaxVolumesPerNode: d.getVolumesLimit(),
AccessibleTopology: topology,
}, nil
}
Expand Down Expand Up @@ -464,6 +473,16 @@ func findNvmeVolume(findName string) (device string, err error) {
return resolved, nil
}

// getVolumesLimit returns the limit of volumes that the node supports
func (d *nodeService) getVolumesLimit() int64 {
ebsNitroInstanceTypeRegex := "^[cmr]5.*|t3|z1d"
instanceType := d.metadata.GetInstanceType()
if ok, _ := regexp.MatchString(ebsNitroInstanceTypeRegex, instanceType); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels its less error prune if we just maintain a hard coded list of nitro instance instead of regex. And define a function that checks if it is nitro based:

func isNitroInstance(instanceType string) boolean

How do you think?

See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-types.html#ec2-nitro-instances for a full list of Nitro instances.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with that, but I'm not sure if I understand that page well enough to come up with a reliable list. If nothing, the regex above comes from the in-tree implementation, so it seems to be a reasonable good start.

return defaultMaxEBSNitroVolumes
}
return defaultMaxEBSVolumes
}

func newSafeMounter() *mount.SafeFormatAndMount {
return &mount.SafeFormatAndMount{
Interface: mount.New(""),
Expand Down
74 changes: 51 additions & 23 deletions pkg/driver/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,34 +755,62 @@ func TestNodeGetCapabilities(t *testing.T) {
}

func TestNodeGetInfo(t *testing.T) {
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()
testCases := []struct {
name string
instanceID string
instanceType string
availabilityZone string
expMaxVolumes int64
}{
{
name: "success normal",
instanceID: "i-123456789abcdef01",
instanceType: "t2.medium",
availabilityZone: "us-west-2b",
expMaxVolumes: 39,
},
{
name: "success normal with NVMe",
instanceID: "i-123456789abcdef01",
instanceType: "m5d.large",
availabilityZone: "us-west-2b",
expMaxVolumes: 25,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()

mockMetadata := mocks.NewMockMetadataService(mockCtl)
mockMetadata.EXPECT().GetInstanceID().Return(expInstanceId)
mockMetadata.EXPECT().GetAvailabilityZone().Return(expZone).Times(2)
mockMetadata := mocks.NewMockMetadataService(mockCtl)
mockMetadata.EXPECT().GetInstanceID().Return(tc.instanceID)
mockMetadata.EXPECT().GetInstanceType().Return(tc.instanceType)
mockMetadata.EXPECT().GetAvailabilityZone().Return(tc.availabilityZone)

req := &csi.NodeGetInfoRequest{}
awsDriver := newTestNodeService(mockMetadata, NewFakeMounter())

awsDriver := newTestNodeService(mockMetadata, NewFakeMounter())
resp, err := awsDriver.NodeGetInfo(context.TODO(), &csi.NodeGetInfoRequest{})
if err != nil {
srvErr, ok := status.FromError(err)
if !ok {
t.Fatalf("Could not get error status code from error: %v", srvErr)
}
t.Fatalf("Expected nil error, got %d message %s", srvErr.Code(), srvErr.Message())
}

expResp := &csi.NodeGetInfoResponse{
NodeId: expInstanceId,
AccessibleTopology: &csi.Topology{
Segments: map[string]string{TopologyKey: mockMetadata.GetAvailabilityZone()},
},
}
if resp.GetNodeId() != tc.instanceID {
t.Fatalf("Expected node ID %q, got %q", tc.instanceID, resp.GetNodeId())
}

resp, err := awsDriver.NodeGetInfo(context.TODO(), req)
if err != nil {
srvErr, ok := status.FromError(err)
if !ok {
t.Fatalf("Could not get error status code from error: %v", srvErr)
}
t.Fatalf("Expected nil error, got %d message %s", srvErr.Code(), srvErr.Message())
}
if !reflect.DeepEqual(expResp, resp) {
t.Fatalf("Expected response {%+v}, got {%+v}", expResp, resp)
at := resp.GetAccessibleTopology()
if at.Segments[TopologyKey] != tc.availabilityZone {
t.Fatalf("Expected topology %q, got %q", tc.availabilityZone, at.Segments[TopologyKey])
}

if resp.GetMaxVolumesPerNode() != tc.expMaxVolumes {
t.Fatalf("Expected %d max volumes per node, got %d", tc.expMaxVolumes, resp.GetMaxVolumesPerNode())
}
})
}
}

Expand Down
13 changes: 9 additions & 4 deletions tests/e2e/pre_provsioning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,17 @@ package e2e
import (
"context"
"fmt"
"math/rand"
"os"
"strings"

awscloud "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud"
"github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/driver"
"github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/testsuites"
. "github.com/onsi/ginkgo"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
clientset "k8s.io/client-go/kubernetes"
"k8s.io/kubernetes/test/e2e/framework"
"math/rand"
"os"
"strings"

ebscsidriver "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver"
)
Expand All @@ -53,6 +54,10 @@ func (s e2eMetdataService) GetInstanceID() string {
return ""
}

func (s e2eMetdataService) GetInstanceType() string {
return ""
}

func (s e2eMetdataService) GetAvailabilityZone() string {
return s.availabilityZone
}
Expand Down