refactor: remove dependency on netifaces by catmsred · Pull Request #4634 · canonical/cloud-init

catmsred · 2023-11-28T15:15:12Z

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

refactor: remove dependency on netifaces

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.

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

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

catmsred · 2023-11-28T19:34:34Z

@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 cloudinit/sources/DataSourceVMware.py explaining VMWare's preference for netifaces. There were three main points listed as follows:

Netifaces is built in C and is more portable across multiple system and more deterministic than shell exec'ing local network commands and parsing their output. Is this still true with netifaces no longer being maintained?
Netifaces provides a stable way to determine the view of the host's network after DHCP has brought the network online. Can you provide more clarification on the specific testing you did that gave you inconsistent data with netinfo?
It is currently possible to execute this datasource on macOS (which many developers use today) to print the output of the get_host_info function. The commands executed by netdev_info are fully functional on my Mac environment; is this still a concern?

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!

akutz · 2023-11-29T00:27:43Z

Hi @catmsred,

I will take a look, thank you for working on this!

holmanb

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!

github-actions · 2024-01-04T00:21:14Z

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 · 2024-01-20T00:20:39Z

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.)

holmanb · 2024-01-22T17:28:27Z

@akutz Have you had a chance to look at this? I believe @catmsred is testing this currently and we'll be trying to land it soon.

catmsred · 2024-01-31T16:11:09Z

@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!

PengpengSun · 2024-02-06T05:49:07Z

Hi @catmsred , Thanks for your fix, we will review and verify it on VMware platform.

PengpengSun · 2024-02-07T07:22:17Z

+                LOG.debug(
+                    "multiple ipv4 gateways: %s, %s", ipv4, route["gateway"]
+                )
+            ipv4 = route["gateway"]


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?

PengpengSun · 2024-02-07T07:23:17Z

    ipv4 = None
    ipv6 = None
+    routes = netinfo.route_info()
+    for route in routes:


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?

PengpengSun · 2024-02-07T07:24:29Z

+                LOG.debug(
+                    "multiple ipv6 gateways: %s, %s", ipv6, route["gateway"]
+                )
+            ipv6 = route["gateway"]


ditto. Is this a gw6 or ipv6?

PengpengSun · 2024-02-07T07:26:44Z

+    # If there is an IPv4 gateway and an IPv6 gateway, return immediately to
+    # avoid extra work
+    if ipv4 and ipv6:
+        return ipv4, ipv4


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

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!

PengpengSun · 2024-02-08T07:14:10Z

+            ipv4_if = route["iface"]
+    for route in routes["ipv6"]:
+        if route["destination"] == "::/0":
+            ipv6 = route["iface"]


s/ipv6/ipv6_if/

PengpengSun · 2024-02-08T07:17:54Z

+        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"]


s/"ipv6"/"ipv4"/

akutz · 2024-02-13T15:34:23Z

I have put time on my schedule to review this today and/or tomorrow.

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 · 2024-02-26T14:59:06Z

@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!

PengpengSun

Thanks for fixing this.

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

holmanb

Thanks for this work @catmsred!

Looks good to me.

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.

catmsred force-pushed the feature/remove_netifaces branch from 3c605bc to 8955aae Compare November 28, 2023 15:48

holmanb mentioned this pull request Nov 30, 2023

Update build-on-openbsd dependencies #4644

Merged

holmanb reviewed Nov 30, 2023

View reviewed changes

holmanb self-assigned this Nov 30, 2023

catmsred force-pushed the feature/remove_netifaces branch from 8955aae to 5b3565e Compare November 30, 2023 19:37

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 4, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 5, 2024

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 20, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 22, 2024

catmsred force-pushed the feature/remove_netifaces branch from c55c07a to 70c72a1 Compare January 31, 2024 15:46

PengpengSun reviewed Feb 7, 2024

View reviewed changes

Comment thread cloudinit/sources/DataSourceVMware.py

PengpengSun reviewed Feb 7, 2024

View reviewed changes

catmsred requested a review from PengpengSun February 8, 2024 02:46

PengpengSun reviewed Feb 8, 2024

View reviewed changes

Comment thread cloudinit/sources/DataSourceVMware.py

catmsred force-pushed the feature/remove_netifaces branch from a1e43a8 to e49d376 Compare February 13, 2024 15:32

catmsred force-pushed the feature/remove_netifaces branch 2 times, most recently from 7ff9cd6 to f79e6c1 Compare February 13, 2024 15:45

Catherine Redfield added 6 commits February 26, 2024 09:31

refactor: remove dependency on netifaces

6c3d810

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.

feature: remove netifaces dependency

ff6d713

This commit removes netifaces from the cloud-init dependency lists

fix: update VMWareDataSource per PengpengSun's comments

c787836

test: add unit tests for get_default_ip_addrs

3750742

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.

chore: remove comment explaining rational for netifaces

22fc541

fix: clean up test functions

b139d0a

catmsred force-pushed the feature/remove_netifaces branch from 90543d8 to b139d0a Compare February 26, 2024 14:34

catmsred changed the title ~~[WIP] refactor: remove dependency on netifaces~~ refactor: remove dependency on netifaces Feb 26, 2024

catmsred requested review from PengpengSun and holmanb February 26, 2024 14:36

fix(openbsd): remove netifaces dependency

9db7ec1

PengpengSun approved these changes Feb 27, 2024

View reviewed changes

catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 4, 2024

d/control: remove netifaces

f035882

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

catmsred mentioned this pull request Mar 4, 2024

d/control: remove netifaces [noble] #4998

Closed

5 tasks

holmanb approved these changes Mar 5, 2024

View reviewed changes

holmanb merged commit 2ba7fdf into canonical:main Mar 5, 2024

catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 5, 2024

d/control: remove netifaces

b392d72

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 5, 2024

d/control: remove netifaces

8bc6c47

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

This was referenced Mar 5, 2024

d/control: remove netifaces [mantic] #5003

Closed

d/control: remove netifaces [mantic] #5004

Merged

catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 5, 2024

d/control: remove netifaces

ce89306

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

catmsred mentioned this pull request Mar 5, 2024

d/control: remove netifaces [jammy] #5006

Merged

5 tasks

catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 5, 2024

d/control: remove netifaces

89037d9

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

catmsred mentioned this pull request Mar 5, 2024

d/control: remove netifaces [focal] #5007

Merged

5 tasks

catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 11, 2024

d/control: remove netifaces

b840c63

netifaces is being removed from tip of main in canonicalGH-4634. This change also removes it from the Ubuntu packaging.

holmanb pushed a commit that referenced this pull request Mar 13, 2024

d/control: remove netifaces

48947ef

netifaces is being removed from tip of main in GH-4634. This change also removes it from the Ubuntu packaging.

holmanb pushed a commit that referenced this pull request Mar 13, 2024

d/control: remove netifaces

a890f4b

netifaces is being removed from tip of main in GH-4634. This change also removes it from the Ubuntu packaging.

holmanb pushed a commit that referenced this pull request Mar 15, 2024

d/control: remove netifaces

0f9aa8b

netifaces is being removed from tip of main in GH-4634. This change also removes it from the Ubuntu packaging.

Conversation

catmsred commented Nov 28, 2023

Proposed Commit Message

Additional Context

Checklist

Merge type

Uh oh!

catmsred commented Nov 28, 2023

Uh oh!

akutz commented Nov 29, 2023

Uh oh!

holmanb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

main branch updates:

ubuntu packaging: update debian/control in the following branches

Uh oh!

github-actions Bot commented Jan 4, 2024

Uh oh!

github-actions Bot commented Jan 20, 2024

Uh oh!

holmanb commented Jan 22, 2024

Uh oh!

catmsred commented Jan 31, 2024

Uh oh!

PengpengSun commented Feb 6, 2024

Uh oh!

Uh oh!

PengpengSun Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PengpengSun Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

PengpengSun Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PengpengSun Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

catmsred Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

PengpengSun Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

PengpengSun Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akutz commented Feb 13, 2024

Uh oh!

catmsred commented Feb 26, 2024

Uh oh!

PengpengSun left a comment

Choose a reason for hiding this comment

Uh oh!

holmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holmanb left a comment •

edited

Loading

PengpengSun Feb 7, 2024 •

edited

Loading

PengpengSun Feb 7, 2024 •

edited

Loading