Skip to content

Bump pysnmp to v7 and brother to v5 #129761

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

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

nmaggioni
Copy link
Contributor

@nmaggioni nmaggioni commented Nov 3, 2024

Proposed change

Spurred by #129685, this PR updates the pysnmp dependency to its most recent next major version, v7.1.9.
I have tested these changes in my setup using the device_tracker platform only, but the synthetic tests for the other platforms (sensor, switch) look okay.

I can't seem to get the MIBs to cache properly, though: I tried to update the old mechanism by looking at pysnmp's own tests but I keep getting a couple of Detected blocking call warnings logged when the first SNMP scan starts.

Logged warnings
Recorder: homeassistant.util.loop
Source: util/loop.py:136

Detected blocking call to listdir with args ('/root/venv_hass/lib/python3.12/site-packages/pysnmp/smi/mibs',) inside the event loop by integration 'snmp' at homeassistant/components/snmp/device_tracker.py, line 185: async for errindication, errstatus, errindex, res in walker: (offender: /root/venv_hass/lib/python3.12/site-packages/pysnmp/smi/builder.py, line 237: if f in os.listdir(self._srcName): # make FS case-sensitive), please create a bug report at https://github.com/home-assistant/core/issues?q=is%3Aopen+is%3Aissue+label%3A%22integration%3A+snmp%22 For developers, please see https://developers.home-assistant.io/docs/asyncio_blocking_operations/#listdir Traceback (most recent call last): File "/root/venv_hass/bin/hass", line 8, in <module> sys.exit(main()) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/__main__.py", line 209, in main exit_code = runner.run(runtime_conf) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/runner.py", line 189, in run return loop.run_until_complete(setup_and_run_hass(runtime_config)) File "/usr/lib/python3.12/asyncio/base_events.py", line 651, in run_until_complete self.run_forever() File "/usr/lib/python3.12/asyncio/base_events.py", line 618, in run_forever self._run_once() File "/usr/lib/python3.12/asyncio/base_events.py", line 1951, in _run_once handle._run() File "/usr/lib/python3.12/asyncio/events.py", line 84, in _run self._context.run(self._callback, *self._args) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/device_tracker/legacy.py", line 331, in async_setup_legacy scanner = await self.platform.async_get_scanner( File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/snmp/device_tracker.py", line 63, in async_get_scanner await scanner.async_init(hass) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/snmp/device_tracker.py", line 141, in async_init data = await self.async_get_snmp_data() File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/snmp/device_tracker.py", line 185, in async_get_snmp_data async for errindication, errstatus, errindex, res in walker:
Detected blocking call to open with args ('/root/venv_hass/lib/python3.12/site-packages/pysnmp/smi/mibs/PYSNMP-SOURCE-MIB.py', 'r') inside the event loop by integration 'snmp' at homeassistant/components/snmp/device_tracker.py, line 185: async for errindication, errstatus, errindex, res in walker: (offender: /root/venv_hass/lib/python3.12/site-packages/pysnmp/smi/builder.py, line 239: fp = open(p, mode)), please create a bug report at https://github.com/home-assistant/core/issues?q=is%3Aopen+is%3Aissue+label%3A%22integration%3A+snmp%22 For developers, please see https://developers.home-assistant.io/docs/asyncio_blocking_operations/#open Traceback (most recent call last): File "/root/venv_hass/bin/hass", line 8, in <module> sys.exit(main()) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/__main__.py", line 209, in main exit_code = runner.run(runtime_conf) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/runner.py", line 189, in run return loop.run_until_complete(setup_and_run_hass(runtime_config)) File "/usr/lib/python3.12/asyncio/base_events.py", line 651, in run_until_complete self.run_forever() File "/usr/lib/python3.12/asyncio/base_events.py", line 618, in run_forever self._run_once() File "/usr/lib/python3.12/asyncio/base_events.py", line 1951, in _run_once handle._run() File "/usr/lib/python3.12/asyncio/events.py", line 84, in _run self._context.run(self._callback, *self._args) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/device_tracker/legacy.py", line 331, in async_setup_legacy scanner = await self.platform.async_get_scanner( File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/snmp/device_tracker.py", line 63, in async_get_scanner await scanner.async_init(hass) File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/snmp/device_tracker.py", line 141, in async_init data = await self.async_get_snmp_data() File "/root/venv_hass/lib/python3.12/site-packages/homeassistant/components/snmp/device_tracker.py", line 185, in async_get_snmp_data async for errindication, errstatus, errindex, res in walker:

Since the brother component also depends on pysnmp, @bieniu may want to chime in.
brother updated to 5.0.0.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

This is not-actionable, because brother==4.3.1 depends on pysnmp>=6.2.6,<7.0

brother dependency will need to be adjusted to support pysnmp v7 before this can be actionned

@home-assistant
Copy link

home-assistant bot commented Nov 4, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft November 4, 2024 13:29
@bieniu
Copy link
Member

bieniu commented Nov 4, 2024

I hope to find time this week to try updating pysnmp in brother.

@bieniu
Copy link
Member

bieniu commented Nov 5, 2024

@nmaggioni Please bump brother version to 5.0.0.

@nmaggioni nmaggioni changed the title Bump pysnmp to v7 Bump pysnmp to v7 and brother to v5 Nov 6, 2024
@nmaggioni
Copy link
Contributor Author

@bieniu Thanks. I see that you've used the same MIBs caching snippet I did, does it work for you or are the definitions synchronously loaded again on the first use of the engine?

@bieniu
Copy link
Member

bieniu commented Nov 6, 2024

Yeah I copied your solution 😄 I didn't notice any event loop blocking. I checked SNMP sensor and of course Brother.

I'll try to test this PR again tonight.

@bieniu
Copy link
Member

bieniu commented Nov 6, 2024

I tested the snmp sensor again and I don't see any event loop blocking.

@nmaggioni
Copy link
Contributor Author

It seems that only the device_tracker implementation is causing warnings to be logged: to me it looks like the async generator returned by bulk_walk_cmd somehow still triggers blocking I/O detection when it parses the data in the response, but I'm not proficient enough with asyncio to understand why (isn't it already literally async?).

Also, I had a dupe assignment in my caching snippet; it did no harm, but you may want to pull that revert too.

@bieniu
Copy link
Member

bieniu commented Nov 15, 2024

I tried to find where this event loop blocking occurs today but I failed. Some asyncio specialist is needed here.

@bieniu bieniu added the help-wanted Wanna help? Jump in! label Nov 15, 2024
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Jan 14, 2025
@nmaggioni
Copy link
Contributor Author

nmaggioni commented Jan 14, 2025

Still valid and needed, but unless logging a blocking I/O warning at boot is acceptable we need help with asyncio troubleshooting.

Also rebased on current dev branch.

@bieniu
Copy link
Member

bieniu commented Jan 15, 2025

Maybe try bumping pysnmp version to the latest 7.1.16?

@bieniu
Copy link
Member

bieniu commented Mar 13, 2025

Could you bump pysnmp to 7.1.16 and rebase?

MibViewControllerManager.get_mib_view_controller already assigns the
cached variable, no need to do that manually.
This reverts commit 767d855.
@nmaggioni
Copy link
Contributor Author

Sorry, this slipped through. Anyway, I've bumped the version but the issue is still there: MIBs are getting (re?)loaded when the walker is first called, so HA reports the relative fs calls.

I've retraced my steps briefly but I'm still stumped. Since the walker itself is an async iterator it can't be run inside an executor wrapper, but that point is more or less where the code flow moves away from HA and into pysnmp.

@bieniu
Copy link
Member

bieniu commented Mar 18, 2025

@lextm Sorry to tag you directly, maybe you can help here?

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

Successfully merging this pull request may close these issues.

SNMP Cypto Warning Log for TripleDES
3 participants