Update enimetadata for interfaces with no ip info #3041
Conversation
| return ENIMetadata{}, err | ||
| } | ||
| if len(imdsIPv4s) > 0 { | ||
| isEFAOnlyInterface = false |
There was a problem hiding this comment.
My concern is if the ENI is truly missing local-ipv4
There was a problem hiding this comment.
field == "local-ipv4s"
I agree. If these kind of 'structural' definitions is supported by any documentation or spec, that will be best to rely upon.
| @@ -136,6 +136,24 @@ func (imds TypedIMDS) GetMACs(ctx context.Context) ([]string, error) { | |||
| return list, err | |||
| } | |||
|
|
|||
| // GetMACImdsFields returns the imds fields present for a MAC | |||
There was a problem hiding this comment.
Couldn't we use GetMACs function and check for it.
- Introducing yet another
GetMACImdsFields- only to check for thefieldsmight be introduced a too specialized a function for our purpose here.
There was a problem hiding this comment.
It will be preferable to use GetMACs to continue with coverage here.
There was a problem hiding this comment.
@orsenthil Can you add more details on using GetMACs.. So efa-only will not have any IP fields associated with them. So checking local-ipv4s field. Want to understand how we can use GetMACs here
There was a problem hiding this comment.
@Pavani-Panakanti - you are right. I thought, we were using the network/interfaces/macs/ and some property of that output. This is different. Sorry for the distraction.
|
The integration test is successful on this change - https://github.com/aws/amazon-vpc-cni-k8s/actions/runs/11053939700 But we have to make sure that we are doing this accurately - with the support of the spec which defines the multi-card instance characteristics. |
|
Following with this change in new PR #3047 |
What type of PR is this?
bug
Which issue does this PR fix?:
Network card field is not being present in imds in all instances. We can rely on it to filter out ENI's on non-0 network cards
What does this PR do / Why do we need it?:
Check if ipv4 and ipv6 fields are present in imds. Fetch these data only if these fields are present
Testing done on this change:
Verified aws-node is coming up as expected and pods are being scheduled for both multi card and single card instances. Verified with interface with no ips
Will this PR introduce any new dependencies?:
No
Will this break upgrades or downgrades? Has updating a running cluster been tested?:
No
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.