Skip to content

Commit

Permalink
Remove block-device-mapping from attach limit
Browse files Browse the repository at this point in the history
meta-data/block-device-mapping reports devices that were attached to the
VM at the time the VM was started. It does not reflect devices
attached/detached later.

In addition, even when it showed the attachments accurately, the CSI driver
uses this information only at the driver startup. At this time, there
may be some CSI volumes attached from the previous node boot. Those
volumes are then counted as permanently attached to the node, while
Kubernetes may detach them later.

Therefore do not count block-device-mapping at all and always reserve just
1 attachment for the root disk. Assume all other attachments are CSI
volumes, and let Kubernetes do the calculations of attached volumes
instead.
  • Loading branch information
jsafrane committed Nov 21, 2023
1 parent 97688f8 commit 885c935
Show file tree
Hide file tree
Showing 8 changed files with 20 additions and 121 deletions.
17 changes: 6 additions & 11 deletions pkg/cloud/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,12 @@ import (

// Metadata is info about the ec2 instance on which the driver is running
type Metadata struct {
InstanceID string
InstanceType string
Region string
AvailabilityZone string
NumAttachedENIs int
NumBlockDeviceMappings int
OutpostArn arn.ARN
InstanceID string
InstanceType string
Region string
AvailabilityZone string
NumAttachedENIs int
OutpostArn arn.ARN
}

const (
Expand Down Expand Up @@ -72,10 +71,6 @@ func (m *Metadata) GetNumAttachedENIs() int {
return m.NumAttachedENIs
}

func (m *Metadata) GetNumBlockDeviceMappings() int {
return m.NumBlockDeviceMappings
}

// GetOutpostArn returns outpost arn if instance is running on an outpost. empty otherwise.
func (m *Metadata) GetOutpostArn() arn.ARN {
return m.OutpostArn
Expand Down
20 changes: 5 additions & 15 deletions pkg/cloud/metadata_ec2.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,13 @@ func EC2MetadataInstanceInfo(svc EC2Metadata, regionFromSession string) (*Metada
}

attachedENIs := strings.Count(enis, "\n") + 1
blockDevMappings := 0

if !util.IsSBE(doc.Region) {
mappings, mapErr := svc.GetMetadata(blockDevicesEndpoint)
if mapErr != nil {
return nil, fmt.Errorf("could not get number of block device mappings: %w", err)
}
blockDevMappings = strings.Count(mappings, "ebs")
}

instanceInfo := Metadata{
InstanceID: doc.InstanceID,
InstanceType: doc.InstanceType,
Region: doc.Region,
AvailabilityZone: doc.AvailabilityZone,
NumAttachedENIs: attachedENIs,
NumBlockDeviceMappings: blockDevMappings,
InstanceID: doc.InstanceID,
InstanceType: doc.InstanceType,
Region: doc.Region,
AvailabilityZone: doc.AvailabilityZone,
NumAttachedENIs: attachedENIs,
}

outpostArn, err := svc.GetMetadata(outpostArnEndpoint)
Expand Down
1 change: 0 additions & 1 deletion pkg/cloud/metadata_interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ type MetadataService interface {
GetRegion() string
GetAvailabilityZone() string
GetNumAttachedENIs() int
GetNumBlockDeviceMappings() int
GetOutpostArn() arn.ARN
}

Expand Down
11 changes: 5 additions & 6 deletions pkg/cloud/metadata_k8s.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,11 @@ func KubernetesAPIInstanceInfo(clientset kubernetes.Interface) (*Metadata, error
}

instanceInfo := Metadata{
InstanceID: instanceID,
InstanceType: instanceType,
Region: region,
AvailabilityZone: availabilityZone,
NumAttachedENIs: 1, // All nodes have at least 1 attached ENI, so we'll use that
NumBlockDeviceMappings: 0,
InstanceID: instanceID,
InstanceType: instanceType,
Region: region,
AvailabilityZone: availabilityZone,
NumAttachedENIs: 1, // All nodes have at least 1 attached ENI, so we'll use that
}

return &instanceInfo, nil
Expand Down
25 changes: 3 additions & 22 deletions pkg/cloud/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ func TestNewMetadataService(t *testing.T) {
imdsBlockDeviceOutput string
imdsENIOutput string
expectedENIs int
expectedBlockDevices int
expectedOutpostArn arn.ARN
expectedErr error
node v1.Node
Expand Down Expand Up @@ -318,20 +317,6 @@ func TestNewMetadataService(t *testing.T) {
imdsENIOutput: "00:00:00:00:00:00\n00:00:00:00:00:01",
expectedENIs: 2,
},
{
name: "success: GetMetadata() returns correct number of block device mappings",
ec2metadataAvailable: true,
getInstanceIdentityDocumentValue: ec2metadata.EC2InstanceIdentityDocument{
InstanceID: stdInstanceID,
InstanceType: stdInstanceType,
Region: stdRegion,
AvailabilityZone: stdAvailabilityZone,
},
imdsENIOutput: "00:00:00:00:00:00",
expectedENIs: 1,
imdsBlockDeviceOutput: "ami\nroot\nebs1\nebs2",
expectedBlockDevices: 2,
},
{
name: "success: region from session is snow",
ec2metadataAvailable: true,
Expand All @@ -341,10 +326,9 @@ func TestNewMetadataService(t *testing.T) {
Region: "",
AvailabilityZone: "",
},
imdsENIOutput: "00:00:00:00:00:00",
expectedENIs: 1,
regionFromSession: snowRegion,
expectedBlockDevices: 0,
imdsENIOutput: "00:00:00:00:00:00",
expectedENIs: 1,
regionFromSession: snowRegion,
},
}

Expand Down Expand Up @@ -425,9 +409,6 @@ func TestNewMetadataService(t *testing.T) {
if m.GetNumAttachedENIs() != tc.expectedENIs {
t.Errorf("GetMetadata() failed for %s: got %v, expected %v", enisEndpoint, m.GetNumAttachedENIs(), tc.expectedENIs)
}
if m.GetNumBlockDeviceMappings() != tc.expectedBlockDevices {
t.Errorf("GetMetadata() failed for %s: got %v, expected %v", blockDevicesEndpoint, m.GetNumBlockDeviceMappings(), tc.expectedBlockDevices)
}
}
mockCtrl.Finish()
})
Expand Down
14 changes: 0 additions & 14 deletions pkg/cloud/mock_metadata.go

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

3 changes: 1 addition & 2 deletions pkg/driver/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,6 @@ func (d *nodeService) getVolumesLimit() int64 {

isNitro := cloud.IsNitroInstanceType(instanceType)
availableAttachments := cloud.GetMaxAttachments(isNitro)
blockVolumes := d.metadata.GetNumBlockDeviceMappings()
dedicatedLimit := cloud.GetDedicatedLimitForInstanceType(instanceType)

// For special dedicated limit instance types, the limit is only for EBS volumes
Expand All @@ -789,7 +788,7 @@ func (d *nodeService) getVolumesLimit() int64 {
nvmeInstanceStoreVolumes := cloud.GetNVMeInstanceStoreVolumesForInstanceType(instanceType)
availableAttachments = availableAttachments - enis - nvmeInstanceStoreVolumes
}
availableAttachments = availableAttachments - blockVolumes - 1 // -1 for root device
availableAttachments = availableAttachments - 1 // -1 for root device
if availableAttachments < 0 {
availableAttachments = 0
}
Expand Down
50 changes: 0 additions & 50 deletions pkg/driver/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2025,7 +2025,6 @@ func TestNodeGetInfo(t *testing.T) {
availabilityZone string
region string
attachedENIs int
blockDevices int
volumeAttachLimit int64
expMaxVolumes int64
outpostArn arn.ARN
Expand Down Expand Up @@ -2137,54 +2136,6 @@ func TestNodeGetInfo(t *testing.T) {
expMaxVolumes: 11,
outpostArn: emptyOutpostArn,
},
{
name: "nitro instances already attached EBS volumes",
instanceID: "i-123456789abcdef01",
instanceType: "t3.xlarge",
availabilityZone: "us-west-2b",
region: "us-west-2",
volumeAttachLimit: -1,
attachedENIs: 1,
blockDevices: 2,
expMaxVolumes: 24,
outpostArn: emptyOutpostArn,
},
{
name: "nitro instance already attached max EBS volumes",
instanceID: "i-123456789abcdef01",
instanceType: "t3.xlarge",
availabilityZone: "us-west-2b",
region: "us-west-2",
volumeAttachLimit: -1,
attachedENIs: 1,
blockDevices: 27,
expMaxVolumes: 0,
outpostArn: emptyOutpostArn,
},
{
name: "non-nitro instance already attached max EBS volumes",
instanceID: "i-123456789abcdef01",
instanceType: "m5.xlarge",
availabilityZone: "us-west-2b",
region: "us-west-2",
volumeAttachLimit: -1,
attachedENIs: 1,
blockDevices: 39,
expMaxVolumes: 0,
outpostArn: emptyOutpostArn,
},
{
name: "nitro instance already attached max ENIs",
instanceID: "i-123456789abcdef01",
instanceType: "t3.xlarge",
availabilityZone: "us-west-2b",
region: "us-west-2",
volumeAttachLimit: -1,
attachedENIs: 27,
blockDevices: 1,
expMaxVolumes: 0,
outpostArn: emptyOutpostArn,
},
{
name: "nitro instance with dedicated limit",
instanceID: "i-123456789abcdef01",
Expand Down Expand Up @@ -2217,7 +2168,6 @@ func TestNodeGetInfo(t *testing.T) {

if tc.volumeAttachLimit < 0 {
mockMetadata.EXPECT().GetInstanceType().Return(tc.instanceType)
mockMetadata.EXPECT().GetNumBlockDeviceMappings().Return(tc.blockDevices)
if cloud.IsNitroInstanceType(tc.instanceType) && cloud.GetDedicatedLimitForInstanceType(tc.instanceType) == 0 {
mockMetadata.EXPECT().GetNumAttachedENIs().Return(tc.attachedENIs)
}
Expand Down

0 comments on commit 885c935

Please sign in to comment.