-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(ec2): check elastic NICs for metadata server first #6651
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.
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
Conversation
The EC2 datasource checks all NICs one by one to see which can reach the metadata server. This can lead to slow boot if the primary NIC is not checked first. We use the fact that typically the primary NIC on an EC2 instance will be the lowest numbered Elastic interface (either ENA or EFA) and order these first. Only then checking other network devices that may be present. Fixes canonicalGH-6618 Signed-off-by: Zachary Raines <zachary.raines@canonical.com>
18e5f16 to
594c912
Compare
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few nitpicks comments - this is really good. Thanks, @raineszm.
cloudinit/sources/DataSourceEc2.py
Outdated
|
|
||
| # Drivers that indicate a NIC is being provided by EC2 | ||
| # as an Elastic Network Adaptor or Elastic Fabric Adapter | ||
| # https://github.com/amzn/amzn-drivers/tree/46e50d6265ef6669877610549205973955748039/kernel/linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this link relates to the comment, outside of the fact that it is the repo for amazon's network drivers. Is there something significant about this commitish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just where I sourced the list of amazon driver types from. I'm not sure if there's a more authoritative listing of what kernel drivers are used by AWS elastic interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In that case the main repo page (https://github.com/amzn/amzn-drivers/) seems more authoritative than a specific committish that could be removed at any point. And no need for the extra whitespace before the link.
Signed-off-by: Zachary Raines <zachary.raines@canonical.com>
014a07e to
19d5340
Compare
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this, @raineszm! We're getting there, this looks closer to ready.
Can you please include some details in the PR description about verification? It would be helpful in the future to know what kind of instance will be affected by this change, and cloud-init.log from a system that was tested with this change.
Also the commit message is long - it should be wrapped to a shorter line length.
Once those and my other comment are addressed, I think this should be ready to merge.
cloudinit/sources/DataSourceEc2.py
Outdated
|
|
||
| # Drivers that indicate a NIC is being provided by EC2 | ||
| # as an Elastic Network Adaptor or Elastic Fabric Adapter | ||
| # https://github.com/amzn/amzn-drivers/tree/46e50d6265ef6669877610549205973955748039/kernel/linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In that case the main repo page (https://github.com/amzn/amzn-drivers/) seems more authoritative than a specific committish that could be removed at any point. And no need for the extra whitespace before the link.
Signed-off-by: Zachary Raines <zachary.raines@canonical.com>
holmanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @raineszm!
Proposed Commit Message
Test Steps
The fix can be tested by starting an AWS EC2 instance with an image that contains the patch. The metadata should be fetched from the primary interface of the instance.
See PALS-1874 for more details.
Merge type