From 885c935529d07fabe598bf09a6f3d44c99c1c89c Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 21 Nov 2023 17:58:10 +0100 Subject: [PATCH] Remove block-device-mapping from attach limit 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. --- 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, 20 insertions(+), 121 deletions(-) diff --git a/pkg/cloud/metadata.go b/pkg/cloud/metadata.go index f289ade496..b3d9110193 100644 --- a/pkg/cloud/metadata.go +++ b/pkg/cloud/metadata.go @@ -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 ( @@ -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 diff --git a/pkg/cloud/metadata_ec2.go b/pkg/cloud/metadata_ec2.go index d3841997d8..509e65e394 100644 --- a/pkg/cloud/metadata_ec2.go +++ b/pkg/cloud/metadata_ec2.go @@ -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) diff --git a/pkg/cloud/metadata_interface.go b/pkg/cloud/metadata_interface.go index f98c22a832..8e0d1d7b79 100644 --- a/pkg/cloud/metadata_interface.go +++ b/pkg/cloud/metadata_interface.go @@ -12,7 +12,6 @@ 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 1ba84a0990..4c610265ee 100644 --- a/pkg/cloud/metadata_k8s.go +++ b/pkg/cloud/metadata_k8s.go @@ -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 diff --git a/pkg/cloud/metadata_test.go b/pkg/cloud/metadata_test.go index 16e4dccf37..770395fcfa 100644 --- a/pkg/cloud/metadata_test.go +++ b/pkg/cloud/metadata_test.go @@ -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 @@ -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, @@ -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, }, } @@ -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() }) diff --git a/pkg/cloud/mock_metadata.go b/pkg/cloud/mock_metadata.go index 7eab9d0998..fc61fbc6e5 100644 --- a/pkg/cloud/mock_metadata.go +++ b/pkg/cloud/mock_metadata.go @@ -91,20 +91,6 @@ 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 68d9b84523..12904f4fc3 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -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 @@ -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 } diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index a8be6083c2..50bec5470c 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -2025,7 +2025,6 @@ func TestNodeGetInfo(t *testing.T) { availabilityZone string region string attachedENIs int - blockDevices int volumeAttachLimit int64 expMaxVolumes int64 outpostArn arn.ARN @@ -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", @@ -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) }