Skip to content

d/control: remove netifaces [noble]#4998

Closed
catmsred wants to merge 26 commits into
canonical:ubuntu/develfrom
catmsred:remove_netifaces_packaging_devel
Closed

d/control: remove netifaces [noble]#4998
catmsred wants to merge 26 commits into
canonical:ubuntu/develfrom
catmsred:remove_netifaces_packaging_devel

Conversation

@catmsred
Copy link
Copy Markdown
Contributor

@catmsred catmsred commented Mar 4, 2024

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

Test Steps

Untested

Checklist

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 catmsred added the wip Work in progress, do not land label Mar 4, 2024
@catmsred
Copy link
Copy Markdown
Contributor Author

catmsred commented Mar 4, 2024

Setting as WIP until GH-4634 lands

@catmsred catmsred requested a review from holmanb March 4, 2024 18:08
@catmsred catmsred added dependencies Pull requests that update a dependency file and removed wip Work in progress, do not land labels Mar 5, 2024
@holmanb
Copy link
Copy Markdown
Member

holmanb commented Mar 5, 2024

@catmsred this needs to be refreshed against the recent updates to ubuntu/devel

@catmsred catmsred force-pushed the remove_netifaces_packaging_devel branch from b2cbe63 to b60e4c1 Compare March 5, 2024 18:34
@catmsred catmsred changed the title d/control: remove netifaces d/control: remove netifaces [noble] Mar 5, 2024
Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

This looks really close @catmsred, however since this builds directly against 24.1, which still requires netifaces, build fails with the following error:

==================================== ERRORS ====================================
___________ ERROR collecting tests/unittests/sources/test_common.py ____________
ImportError while importing test module '/<<PKGBUILDDIR>>/tests/unittests/sources/test_common.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/unittests/sources/test_common.py:34: in <module>
    from cloudinit.sources import DataSourceVMware as VMware
cloudinit/sources/DataSourceVMware.py:75: in <module>
    import netifaces
E   ModuleNotFoundError: No module named 'netifaces'
___________ ERROR collecting tests/unittests/sources/test_vmware.py ____________
ImportError while importing test module '/<<PKGBUILDDIR>>/tests/unittests/sources/test_vmware.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/unittests/sources/test_vmware.py:17: in <module>
    from cloudinit.sources import DataSourceVMware
cloudinit/sources/DataSourceVMware.py:75: in <module>
    import netifaces
E   ModuleNotFoundError: No module named 'netifaces'
=============================== warnings summary ===============================
../../../usr/lib/python3/dist-packages/_pytest/config/__init__.py:1316
  /usr/lib/python3/dist-packages/_pytest/config/__init__.py:1316: PytestRemovedIn8Warning: The --strict option is deprecated, use --strict-markers instead.
    self.issue_config_time_warning(

tests/unittests/helpers.py:554
  /<<PKGBUILDDIR>>/tests/unittests/helpers.py:554: DeprecationWarning: Accessing jsonschema.__version__ is deprecated and will be removed in a future release. Use importlib.metadata directly to query for jsonschema's version.
    int(part) for part in jsonschema.__version__.split(".")  # type: ignore

cloudinit/sources/DataSourceAzure.py:52
  /<<PKGBUILDDIR>>/cloudinit/sources/DataSourceAzure.py:52: DeprecationWarning: 'crypt' is deprecated and slated for removal in Python 3.13
    import crypt

tests/unittests/config/test_cc_ubuntu_pro.py:451
  /<<PKGBUILDDIR>>/tests/unittests/config/test_cc_ubuntu_pro.py:451: DeprecationWarning: Accessing jsonschema.__version__ is deprecated and will be removed in a future release. Use importlib.metadata directly to query for jsonschema's version.
    if Version.from_str(getattr(jsonschema, "__version__", "999"))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
ERROR tests/unittests/sources/test_common.py
ERROR tests/unittests/sources/test_vmware.py
!!!!!!!!!!!!!!!!!!! Interrupted: 2 errors during collection !!!!!!!!!!!!!!!!!!!!
======================== 4 warnings, 2 errors in 2.11s =========================
make[2]: *** [Makefile:30: unittest] Error 2
make[2]: Leaving directory '/<<PKGBUILDDIR>>'
make[1]: *** [debian/rules:13: override_dh_auto_test] Error 2
make[1]: Leaving directory '/<<PKGBUILDDIR>>'
make: *** [debian/rules:9: binary] Error 2
dpkg-buildpackage: error: debian/rules binary subprocess returned exit status 2
--------------------------------------------------------------------------------
Build finished at 2024-03-08T01:54:56Z

Finished
--------


+------------------------------------------------------------------------------+
| Cleanup                                                                      |
+------------------------------------------------------------------------------+

Purging /<<BUILDDIR>>
Not cleaning session: cloned chroot in use
E: Build failure (dpkg-buildpackage died)

+------------------------------------------------------------------------------+
| Summary                                                                      |
+------------------------------------------------------------------------------+

Build Architecture: amd64
Build Type: binary
Build-Space: 22640
Build-Time: 4
Distribution: noble
Fail-Stage: build
Host Architecture: amd64
Install-Time: 17
Job: /home/holmanb/ci-a/../out/cloud-init_24.1-0ubuntu2.dsc
Machine Architecture: amd64
Package: cloud-init
Package-Time: 88
Source-Version: 24.1-0ubuntu2
Space: 22640
Status: attempted
Version: 24.1-0ubuntu2
--------------------------------------------------------------------------------
Finished at 2024-03-08T01:54:56Z
Build needed 00:01:28, 22640k disk space
E: Build failure (dpkg-buildpackage died)

You can reproduce this by running:

build-package -- -S -d
sbuild --dist=noble --arch=amd64 ../out/cloud-init_24.1-0ubuntu2.dsc

To rectify this, we'll need a new upstream snapshot first. Hopefully in the near future we won't need new upstream snapshots, but for now we do.

@catmsred catmsred requested a review from holmanb March 8, 2024 15:50
@holmanb
Copy link
Copy Markdown
Member

holmanb commented Mar 9, 2024

Status: successful
Version: 24.2~1g621c522c-0ubuntu1
--------------------------------------------------------------------------------
Finished at 2024-03-09T19:57:27Z
Build needed 00:02:15, 27684k disk space

@catmsred Nice, that looks much better!

Question: How did you end up with b392d72 and b60e4c1 in the middle of the merge commits? Maybe a manual rebase after the snapshot? This technically works, but normally I don't think we insert commits into the middle of a block of commits introduced by a merge. Would you mind rebasing those commits to the end of the this PR? Once you do, I think this is ready to merge!

@holmanb holmanb self-assigned this Mar 9, 2024
Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

see comment

@catmsred
Copy link
Copy Markdown
Contributor Author

Question: How did you end up with b392d72 and b60e4c1 in the middle of the merge commits? Maybe a manual rebase after the snapshot? This technically works, but normally I don't think we insert commits into the middle of a block of commits introduced by a merge. Would you mind rebasing those commits to the end of the this PR? Once you do, I think this is ready to merge!

I have no idea how that happened, to be honest. I ran new_upstream_snapshot.py uss-tableflip and this was the result. I will rebase to fix that.

holmanb and others added 16 commits March 11, 2024 09:16
When someone receives a 404, the splash page should no longer
point to https://canonical-cloud-init.readthedocs-hosted.com
as Canonical now has a CNAME docs.cloud-init.io available.

Add a documentation bug link for broken references which
allow users to quickly report a bug with a failed link if
necessary.
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.
test: cleanup test_net.py

* Move ~3500 lines of network config into its own file
* Replace unittest idioms with pytest idioms
* Remove CiTestCase and FilesystemMockingTestCase
* Parametrize the obvious tests
* Remove commented tests and debug print statements

Much of the heavy lifting was done using `pytestify`.
Moving configuration into another file was mainly because the file
is way too large.

We have commented tests that are years old. Git will keep the
history if we ever need to bring them back. I left the TODO
in place as that can still be a reminder that they are needed.
…anonical#5010)

A debug message is logged whenever an instance is able to connect 
to the IMDS but when it can't, and an ephemeral network needs 
brought up, nothing is logged. Adding a log message here will help 
improve readability of logs and help troubleshoot failure paths 
when an instance is having issues with networking.
…al#4633)

Running systemd-detect-virt is expensive and on systemd >=251
it is unnecessary.

The environment variable SYSTEMD_VIRTUALIZATION contains the
virtualization type that we currently manually get from
systemd-detect-virt[1].

Also fix a minor FreeBSD virtualization bug.

[1] https://www.freedesktop.org/software/systemd/man/latest/systemd.generator.html#Environment
Force ds-identify to use the correct file mocks
Set defaults for optional os-release fields [1]

VERSION_ID and ID might not be set; set defaults.

[1] man 5 os-release

Fixes canonicalGH-5002
When processing multiple urls in wait_for_url and a timeout error
occurs, the local variable url is None yet url_exc.url is set.

Fallback to url_exc.url with url is unet to provide sensible
warning messages:
 Calling '<full_failed_url>' failed [3/120s]: request error

instead of:
 Calling 'None' failed [3/120s]

LP: #2055077
Avoid leaking reads to the underlying host's /sys/class/net files.
Some test environments contain virtual network hardware and
configuration such as Calico network devices which provide
duplicated MAC addresses for each device in /sys/class/net. This
results in errors from unittests calling cloudinit.net.get_interfaces.

Provide a disable_sysfs_net` fixture and use it in net-related
modules or functions to avoid unrelated test failures due to
environmental differences.

Provide the ability to turn off this fixture for tests which
need to write to the mocked sysfs tmp directory so test artifacts
do not pollute other tests.

This fixture can be disabled by passing False to the disable_sysfs_net
via request.param. We want to avoid polluting tox.ini with pytest.marks
for infrequently used cases like this.

Also fix whitespace in tox.ini
Prevent disable_sysfs_net fixture from impacting TestSysDevPath
and TestReadSysNet in order to avoid shared mocks of /sys/class/net.
This avoid test artifact pollution for TestReadSysNet.

Adapt the following tests, dropping CiTestCase to use pytest:
 TestDHCPDiscoveryClean, TestSysDevPath, TestReadSysNet,
 TestGenerateFallbackConfig, TestNetFailOver, TestConvertNetJson

Fixes canonicalGH-4973
@catmsred catmsred force-pushed the remove_netifaces_packaging_devel branch from 7c1e8a0 to b863d75 Compare March 11, 2024 13:17
@catmsred catmsred requested a review from holmanb March 11, 2024 17:14
@catmsred
Copy link
Copy Markdown
Contributor Author

Linkcheck failure is not related to this code

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@catmsred , sorry for the churn on this one. Since we've already independently merged an upstream snapshot for this branch, I'm closing this PR in favor of #5081 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants