refactor: remove dependency on netifaces #4634
Conversation
catmsred
force-pushed
the
feature/remove_netifaces
branch
from
3c605bc to
8955aae
Compare
|
@akutz @PengpengSun This PR proposes replacing VMWare's dependence on netifaces (which is no longer being actively maintained) with cloud-init's netinfo. I know there was a previous thread that discussed this and there is an extensive comment at the top of
If you have some time could you validate that this PR works as you expect in a VMWare environment? I have updated the unit tests but obviously that uses mocks for networking set up and may not hit all the points that concerned you when you originally introduced netifaces into the code base. Thanks! |
|
Hi @catmsred, I will take a look, thank you for working on this! |
There was a problem hiding this comment.
Thanks for your effort on this @catmsred!
Dropping this dependency will help reduce cloud image size requirements due to cloud-init and standardize networking code paths across datasources. This gives us one less datasource variance to have to maintain and reason about.
I know that this is still WIP, but there are few more items that we will want to take care of as part of this change. I know that you're probably waiting on feedback from @akutz and probably testing feedback before doing more work on this.
You mentioned removing the comment at the top of cloudinit/sources/DataSourceVMware.py already when we chatted earlier today.
Here are some other pointers to required changes in case it helps:
main branch updates:
pyproject.toml
tox.ini
requirements.txt
tools/build-on-netbsd
tools/build-on-openbsd
ubuntu packaging: update debian/control in the following branches
- ubuntu/devel
- ubuntu/mantic
- ubuntu/lunar
- ubuntu/jammy
- ubuntu/focal
I haven't dug into the code yet to review, but a brief pass over it didn't yield any initial concerns besides the failing ci job.
Thanks again, and good work!
catmsred
force-pushed
the
feature/remove_netifaces
branch
from
8955aae to
5b3565e
Compare
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
github-actions
bot
added
the
stale-pr
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
github-actions
bot
added
the
stale-pr
catmsred
force-pushed
the
feature/remove_netifaces
branch
from
c55c07a to
70c72a1
Compare
|
@akutz @PengpengSun I have been struggling to set up a VMWare testing environment to validate this proposal. Would either of you have a chance to review and test it? Thanks! |
|
Hi @catmsred , Thanks for your fix, we will review and verify it on VMware platform. |
| LOG.debug( | ||
| "multiple ipv4 gateways: %s, %s", ipv4, route["gateway"] | ||
| ) | ||
| ipv4 = route["gateway"] |
There was a problem hiding this comment.
I think route["gateway"] should be a gateway address, so here ipv4 should be gw4. Furthermore, can we make sure this is a default gateway?
| ipv4 = None | ||
| ipv6 = None | ||
| routes = netinfo.route_info() | ||
| for route in routes: |
There was a problem hiding this comment.
I'm not familiar with netinfo.route_info(), but with my local check, it returns a dict but list, so is this loop working as expected?
| LOG.debug( | ||
| "multiple ipv6 gateways: %s, %s", ipv6, route["gateway"] | ||
| ) | ||
| ipv6 = route["gateway"] |
There was a problem hiding this comment.
ditto. Is this a gw6 or ipv6?
| # If there is an IPv4 gateway and an IPv6 gateway, return immediately to | ||
| # avoid extra work | ||
| if ipv4 and ipv6: | ||
| return ipv4, ipv4 |
There was a problem hiding this comment.
This function is to get ipv4 and ipv6 addresses of default interface, so return gateway is not expected. And it should be return ipv4, ipv6
There was a problem hiding this comment.
I think I have misunderstood the goal of this function. Based on your comments, the correct process for this functions is (one flow but actually IPv4/IPv6 for each step):
- Identify default gateway
- Extract interface of default gateway
- Get IP address of interface
- Return IP address
I will update the code to reflect this flow unless you have further corrections to add. Thanks again for your input here!
| ipv4_if = route["iface"] | ||
| for route in routes["ipv6"]: | ||
| if route["destination"] == "::/0": | ||
| ipv6 = route["iface"] |
| for dev_name in netdev: | ||
| for addr in netdev[dev_name]["ipv6"]: | ||
| if addr["ip"] == ipv6 and len(netdev[dev_name]["ipv4"]) == 1: | ||
| ipv4 = netdev[dev_name]["ipv6"][0]["ip"] |
catmsred
force-pushed
the
feature/remove_netifaces
branch
from
a1e43a8 to
e49d376
Compare
|
I have put time on my schedule to review this today and/or tomorrow. |
catmsred
force-pushed
the
feature/remove_netifaces
branch
2 times, most recently
from
7ff9cd6 to
f79e6c1
Compare
netifaces is no longer being maintained and is only used by the VMWare data source. As such this commit replaces the calls to netifaces with cloudinit's native netinfo.
This commit removes netifaces from the cloud-init dependency lists
Efforts to remove netifaces from cloud-init's dependency tree resulted in substantial changes to this function. This change adds unit testing to verify that the updated code behaves as intended.
catmsred
force-pushed
the
feature/remove_netifaces
branch
from
90543d8 to
b139d0a
Compare
catmsred
changed the title
|
@PengpengSun Thank you for the feedback -- I have moved this to a non-WIP status but please let me know if there's any additional testing you would like to do before approving! |
netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.
Upstream netifaces is no longer being maintained and is only used by the VMWare data source. As such this commit replaces the calls to netifaces with cloudinit's native netinfo.
netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in GH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in GH-4634. This change also removes it from the Ubuntu packaging.
netifaces is being removed from tip of main in GH-4634. This change also removes it from the Ubuntu packaging.
netifaces is no longer being maintained and is only used by the VMWare data source. As such this commit replaces the calls to netifaces with cloudinit's native netinfo.
Proposed Commit Message
Additional Context
This is related to #953. I have tagged is as WIP at this time because I would like to do more testing on VMWare directly to ensure the updated code works in an environment without mocks.
Checklist
Merge type