Fix hang in SNMP device_tracker implementation

[nmaggioni]

Proposed change

Port SNMP device_tracker to the new asyncio implementation

Stemming from #112548 and #113605, this PR updates the SNMP device_tracker implementation to use the same new async facilities that its sensor and switch counterpart are already using and thus resolves the current breakage of the component.

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

• This PR fixes or closes issue: The device tracker feature works for one round but hangs ever after #113605
• This PR is related to issue: SNMP device tracker broken in 2024.3.0 and Python 3.12 #112548, The device tracker feature works for one round but hangs ever after #113605
• Link to documentation pull request: N/A

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:

• Documentation added/updated for www.home-assistant.io

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.
• Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[khenderick]

I wonder whether syncing snmp device_tracker with e.g. snmp switch would be sufficient, since snmp switch (have this on my setup) and snmp sensor (as mentioned in #112548) were also broken.

[nmaggioni]

@khenderick I see that you confirmed the solution in #112795. My testing on device_tracker against real devices worked fine with 6.0.8 and these changes, but if 6.0.9 fixes things for switch and sensor too that's all the better.

If this PR goes through I may look into updating those other two components too to future-proof them, but from a cursory look I had the impression that they were already using the newest implementation.

[lextm]

@nmaggioni Thanks for making the changes, and they turn out to be very important now. More details in #113605.

In short, we must apply both an upgrade to pysnmp-lextudio 6.0.11 (already done in #113463) and your commit fbcba7f to make device tracker feature back to normal.

The sensor/switch features should have been fixed by upgrading pysnmp-lextudio to 6.0.11 alone.

[lextm]

Ping @bdraco / @bieniu / @khenderick

[nmaggioni]

@lextm Thanks for checking in, I've merged your latest version bump into this PR to avoid having to cherry pick it again.

[bieniu]

PR has 3 types of change marked. We should avoid such situations. In most cases, the PR should contain such a change that only one type can be selected. I understand that in this case it should be a bugfix. Please split the change into smaller parts.

Moreover, it seems to me that currently it is not allowed to add new configuration variables for integrations that are configured only using YAML, config flow should be implemented first.

[home-assistant]

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.

[bdraco]

For reference

Moreover, it seems to me that currently it is not allowed to add new configuration variables for integrations that are configured only using YAML

https://github.com/home-assistant/architecture/blob/master/adr/0007-integration-config-yaml-structure.md

config flow should be implemented first.

https://github.com/home-assistant/architecture/blob/master/adr/0010-integration-configuration.md

[bdraco]

The legacy syntax also has the problem that all the python code for all the integrations listed under the base platform has to be loaded before the base platform can complete setup. In the example below sensor can't finish loading until all the code for awesome_integration is loaded so it can validate the yaml:

# Not allowed
sensor:
  - platform: awesome_integration
    username: user

[nmaggioni]

@bieniu The changetype list in the PR description can now be considered outdated: the dependency upgrade has already been handled in #113463 and is only repeated here for ease of merging (the alternative would have been deleting 217ff1b through a force push, losing its revision history).

The combined bugfix + new feat upgrade is really just a bugfix with a (IMO) necessary config coherence bump: since I have ported the async logic from the other two SNMP integrations (sensor and switch), I thought it would make sense if all three config trees that rely on SNMP had the same config syntax and options. I erroneously marked it as a breaking change with an abundance of caution, since what it really means is that a couple of edge cases that the other two integrations already handled can now be tolerated by device_tracker too (namely, specifying the target port and specific SNMPv3 auth protocols). Existing configs will still work without modifications.

As @lextm said the device_tracker integration will still be broken until these changes are merged, and while I fully understand the reasoning behind the ADRs that @bdraco linked I currently do not have the time needed to port all three SNMP integrations to the UI config flow. From my point of view, a working integration with a legacy config is still better than a broken and error-spitting one. If the YAML changes are the real problem here I can revert those modifications, but would it really make sense? This backwards-compatible config options sync up between integrations under the same platform could help the migration to the new flow in the future since the same approach would be valid for all of them.

[bdraco]

If the YAML changes are the real problem here I can revert those modifications

Please do, as we can't merge it with changes that violate architecture decisions. ADR0007 only has one exception and it does not apply here.

but would it really make sense?

We would have to change the ADR if we wanted an exception here, which would require a new discussion at https://github.com/home-assistant/architecture/discussions and agreement by core members.

[nmaggioni]

@bdraco Understood, 63e1c06 keeps the updated internal structure but relies on defaults instead of exposing the new config keys. This commit could eventually be reverted before upgrading to the new flow.

[bdraco]

It looks like this isn't broken anymore AFAICT (or the opening text is out of date) so it doesn't need to be tagged for backport at this time

changes addressed, needs testing

[nmaggioni]

As per #113605 yes, SNMP device tracking is still broken. That issue has partially superseded the initial description of this PR since this one has been stale for longer.

Local testing on a fresh instance with the latest version of these changes works fine for me (that is to say: devices are correctly tracked and refreshed, and the HA startup phase doesn't hang).

[bdraco]

As per #113605 yes, SNMP device tracking is still broken. That issue has partially superseded the initial description of this PR since this one has been stale for longer.

Would you please update the opening text to reflect the current state. Thanks

[nmaggioni]

I have amended the initial description and truncated most of the historical info as requested.