Skip to content
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

net: introduce find_candidate_nics() #1313

Merged
merged 4 commits into from
Mar 11, 2022

Conversation

cjp256
Copy link
Contributor

@cjp256 cjp256 commented Mar 1, 2022

find_fallback_nic_on_linux(), etc. provides valuable filtering of
network interfaces in an effort to determine the best candidate
for the fallback interface.

Expose this logic with a new set of methods for finding the candidate
network interfaces. These methods can be used by data sources which
cannot rely on the fallback interface being the correct choice.

Note that the MAC address filtering is now part of
find_candidate_nics_on_linux(). This should be consistent behavior
as find_fallback_nic_on_linux() never selected an interface without
a MAC. find_fallback_nic_on_linux() continues to prefer eth0,
but we make no such distinction in the candidate search.

Signed-off-by: Chris Patterson cpatterson@microsoft.com

continue
driver = device_driver(interface)
if driver in blacklist_drivers:
LOG.debug(
Copy link
Contributor Author

@cjp256 cjp256 Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if these debug logs are excessive, but I figured we may want to have some indication of why we are ignoring candidate NICs.

@cjp256 cjp256 force-pushed the net-find-candidate-nics branch 5 times, most recently from 5cbc43e to 90b7ce6 Compare March 1, 2022 19:46
find_fallback_nic_on_linux(), etc. provides valuable filtering of
network interfaces in an effort to determine the best candidate
for the fallback interface.

Expose this logic with a new set of methods for finding the candidate
network interfaces.  These methods can be used by data sources which
cannot rely on the fallback interface being the correct choice.

Note that the MAC address filtering is now part of
find_candidate_nics_on_linux().  This should be consistent behavior
as find_fallback_nic_on_linux() never selected an interface without
a MAC.  find_fallback_nic_on_linux() continues to prefer eth0,
but we make no such distinction in the candidate search.

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@cjp256 cjp256 force-pushed the net-find-candidate-nics branch from 90b7ce6 to dcf224e Compare March 1, 2022 19:47
@cjp256
Copy link
Contributor Author

cjp256 commented Mar 2, 2022

@eb3095 This may be of interest to you?

@eb3095
Copy link
Contributor

eb3095 commented Mar 2, 2022

@eb3095 This may be of interest to you?

Indeed, was hoping it was ready for the last PR, but left a note in a comment about switching out for it when it was available. This solves a few issues for us as well.

@@ -389,8 +389,23 @@ def is_disabled_cfg(cfg):
return cfg.get("config") == "disabled"


def find_fallback_nic(blacklist_drivers=None):
"""Return the name of the 'fallback' network device."""
def find_candidate_nics(blacklist_drivers: Optional[List] = None) -> List[str]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be Optional[List[str]]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point fixed everywhere!

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@cjp256 cjp256 force-pushed the net-find-candidate-nics branch from cae07aa to 1d84f8e Compare March 8, 2022 16:00
Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This is really convenient refactor and also simplifying of some of the logic. Thanks!

Wrt to the debug logs, I think it is fine. Better to have a trail than nothing at all.

@TheRealFalcon TheRealFalcon merged commit e5ce6b8 into canonical:main Mar 11, 2022
@cjp256 cjp256 deleted the net-find-candidate-nics branch April 25, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants