From 80fbe370a84e2f8a21f98605751f154c9479d79f Mon Sep 17 00:00:00 2001 From: Eddie Torres Date: Mon, 11 Dec 2023 19:20:58 +0000 Subject: [PATCH 1/2] Revert #1843 - Remove block-device-mapping from attach limit Signed-off-by: Eddie Torres --- pkg/cloud/metadata.go | 17 +++++++---- pkg/cloud/metadata_ec2.go | 20 +++++++++---- pkg/cloud/metadata_interface.go | 1 + pkg/cloud/metadata_k8s.go | 11 ++++---- pkg/cloud/metadata_test.go | 25 +++++++++++++++-- pkg/cloud/mock_metadata.go | 14 +++++++++ pkg/driver/node.go | 3 +- pkg/driver/node_test.go | 50 +++++++++++++++++++++++++++++++++ 8 files changed, 121 insertions(+), 20 deletions(-) diff --git a/pkg/cloud/metadata.go b/pkg/cloud/metadata.go index b3d911019..f289ade49 100644 --- a/pkg/cloud/metadata.go +++ b/pkg/cloud/metadata.go @@ -26,12 +26,13 @@ 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 - OutpostArn arn.ARN + InstanceID string + InstanceType string + Region string + AvailabilityZone string + NumAttachedENIs int + NumBlockDeviceMappings int + OutpostArn arn.ARN } const ( @@ -71,6 +72,10 @@ 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 diff --git a/pkg/cloud/metadata_ec2.go b/pkg/cloud/metadata_ec2.go index 509e65e39..d3841997d 100644 --- a/pkg/cloud/metadata_ec2.go +++ b/pkg/cloud/metadata_ec2.go @@ -61,13 +61,23 @@ 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, + InstanceID: doc.InstanceID, + InstanceType: doc.InstanceType, + Region: doc.Region, + AvailabilityZone: doc.AvailabilityZone, + NumAttachedENIs: attachedENIs, + NumBlockDeviceMappings: blockDevMappings, } outpostArn, err := svc.GetMetadata(outpostArnEndpoint) diff --git a/pkg/cloud/metadata_interface.go b/pkg/cloud/metadata_interface.go index 8e0d1d7b7..f98c22a83 100644 --- a/pkg/cloud/metadata_interface.go +++ b/pkg/cloud/metadata_interface.go @@ -12,6 +12,7 @@ type MetadataService interface { GetRegion() string GetAvailabilityZone() string GetNumAttachedENIs() int + GetNumBlockDeviceMappings() int GetOutpostArn() arn.ARN } diff --git a/pkg/cloud/metadata_k8s.go b/pkg/cloud/metadata_k8s.go index 4c610265e..1ba84a099 100644 --- a/pkg/cloud/metadata_k8s.go +++ b/pkg/cloud/metadata_k8s.go @@ -75,11 +75,12 @@ 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 + 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, } return &instanceInfo, nil diff --git a/pkg/cloud/metadata_test.go b/pkg/cloud/metadata_test.go index 770395fcf..16e4dccf3 100644 --- a/pkg/cloud/metadata_test.go +++ b/pkg/cloud/metadata_test.go @@ -60,6 +60,7 @@ func TestNewMetadataService(t *testing.T) { imdsBlockDeviceOutput string imdsENIOutput string expectedENIs int + expectedBlockDevices int expectedOutpostArn arn.ARN expectedErr error node v1.Node @@ -317,6 +318,20 @@ 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, @@ -326,9 +341,10 @@ func TestNewMetadataService(t *testing.T) { Region: "", AvailabilityZone: "", }, - imdsENIOutput: "00:00:00:00:00:00", - expectedENIs: 1, - regionFromSession: snowRegion, + imdsENIOutput: "00:00:00:00:00:00", + expectedENIs: 1, + regionFromSession: snowRegion, + expectedBlockDevices: 0, }, } @@ -409,6 +425,9 @@ 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() }) diff --git a/pkg/cloud/mock_metadata.go b/pkg/cloud/mock_metadata.go index fc61fbc6e..7eab9d099 100644 --- a/pkg/cloud/mock_metadata.go +++ b/pkg/cloud/mock_metadata.go @@ -91,6 +91,20 @@ func (mr *MockMetadataServiceMockRecorder) GetNumAttachedENIs() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNumAttachedENIs", reflect.TypeOf((*MockMetadataService)(nil).GetNumAttachedENIs)) } +// GetNumBlockDeviceMappings mocks base method. +func (m *MockMetadataService) GetNumBlockDeviceMappings() int { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetNumBlockDeviceMappings") + ret0, _ := ret[0].(int) + return ret0 +} + +// GetNumBlockDeviceMappings indicates an expected call of GetNumBlockDeviceMappings. +func (mr *MockMetadataServiceMockRecorder) GetNumBlockDeviceMappings() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNumBlockDeviceMappings", reflect.TypeOf((*MockMetadataService)(nil).GetNumBlockDeviceMappings)) +} + // GetOutpostArn mocks base method. func (m *MockMetadataService) GetOutpostArn() arn.ARN { m.ctrl.T.Helper() diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 12904f4fc..68d9b8452 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -777,6 +777,7 @@ 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 @@ -788,7 +789,7 @@ func (d *nodeService) getVolumesLimit() int64 { nvmeInstanceStoreVolumes := cloud.GetNVMeInstanceStoreVolumesForInstanceType(instanceType) availableAttachments = availableAttachments - enis - nvmeInstanceStoreVolumes } - availableAttachments = availableAttachments - 1 // -1 for root device + availableAttachments = availableAttachments - blockVolumes - 1 // -1 for root device if availableAttachments < 0 { availableAttachments = 0 } diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index 50bec5470..a8be6083c 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -2025,6 +2025,7 @@ func TestNodeGetInfo(t *testing.T) { availabilityZone string region string attachedENIs int + blockDevices int volumeAttachLimit int64 expMaxVolumes int64 outpostArn arn.ARN @@ -2136,6 +2137,54 @@ 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", @@ -2168,6 +2217,7 @@ 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) } From c01d2fe1921d6b153941df75c0784bcf38ef3435 Mon Sep 17 00:00:00 2001 From: Eddie Torres Date: Mon, 11 Dec 2023 19:22:44 +0000 Subject: [PATCH 2/2] If availableAttachments <= 0 clamp value to 1 Signed-off-by: Eddie Torres --- pkg/driver/node.go | 4 ++-- pkg/driver/node_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 68d9b8452..c2753e8b6 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -790,8 +790,8 @@ func (d *nodeService) getVolumesLimit() int64 { availableAttachments = availableAttachments - enis - nvmeInstanceStoreVolumes } availableAttachments = availableAttachments - blockVolumes - 1 // -1 for root device - if availableAttachments < 0 { - availableAttachments = 0 + if availableAttachments <= 0 { + availableAttachments = 1 } maxEBSAttachments, ok := cloud.GetEBSLimitForInstanceType(instanceType) diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index a8be6083c..397c931e4 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -2158,7 +2158,7 @@ func TestNodeGetInfo(t *testing.T) { volumeAttachLimit: -1, attachedENIs: 1, blockDevices: 27, - expMaxVolumes: 0, + expMaxVolumes: 1, outpostArn: emptyOutpostArn, }, { @@ -2170,7 +2170,7 @@ func TestNodeGetInfo(t *testing.T) { volumeAttachLimit: -1, attachedENIs: 1, blockDevices: 39, - expMaxVolumes: 0, + expMaxVolumes: 1, outpostArn: emptyOutpostArn, }, { @@ -2182,7 +2182,7 @@ func TestNodeGetInfo(t *testing.T) { volumeAttachLimit: -1, attachedENIs: 27, blockDevices: 1, - expMaxVolumes: 0, + expMaxVolumes: 1, outpostArn: emptyOutpostArn, }, {