lextudio / pysnmp

Python SNMP library
https://www.pysnmp.com/pysnmp/
BSD 2-Clause "Simplified" License
78 stars 21 forks source link

[Bug]: 6.0.0 switch to asyncio appears breaking #49

Closed juliakreger closed 2 months ago

juliakreger commented 7 months ago

Expected behavior

When upgrading OpenStack's upper constraint to 5.0.36, we discovered the changes in 5.0.35 which were breaking our unit tests and our pysnmp library usage.

Actual behavior

Specifically, it appears before we would end up with an artifact like this on 5.0.34 when we execute getCmd

<list_iterator object at 0x7f6a1650c610>

But going to 5.0.35:

TypeError: 'coroutine' object is not an iterator

Detailed steps

We have been able to reproduce this with our CI system for OpenStack:

Change: https://review.opendev.org/c/openstack/ironic/+/909315 Job Logs: https://zuul.opendev.org/t/openstack/build/bf13bc60e12d404080c9bea91dd4d9fd

I am able to reproduce this locally. As a result I came up with https://review.opendev.org/c/openstack/ironic/+/909483/1/ironic/drivers/modules/snmp.py for ironic's snmp driver usage, however we're unsure we can merge it without breaking our support version window. In reality, the base change likely necessitates a 6.0.0 version of pysnmp be released to enable operator and software migration.

Python package information

5.0.35

Operating system information

Linux - Debian

Python information

3.11.2

(Optional) Contents of your test script

git clone https://opendev.org/openstack/ironic
cd ironic && tox -epy3 -- test_snmp
. .tox/py3/bin/activate
pip install lextudio-pysnmp==3.0.35
tox -epy3 -- test_snmp

Relevant log output

No response

lextm commented 7 months ago

Thanks for the report.

It seems that we should yank 5.0.35 and above and republish them as 6.0.x. They contain breaking changes, so people do need efforts to upgrade to.

juliakreger commented 7 months ago

That was sort of what I was thinking as well. Unfortunately :(

lextm commented 7 months ago

We have yanked 5.0.35 and above, and republished them as 6.0.x.

@juliakreger May we know how to open a ticket so that ironic developers might be notified about this breaking change? They might determine when to upgrade to 6.0 releases.

grant-allan-ctct commented 7 months ago

Wondering if there could/should maybe be an announcement / discussion board item that anyone with requirements.txt line similar to pysnmp-lextudio~=5.0.35 should move instead to pysnmp-lextudio~=6.0.0

juliakreger commented 7 months ago

@lextm We're aware at this point, but if you'd like to file a bug use https://bugs.launchpad.net/ironic. I doubt, at this point, we would take on 6.0.0 until sometime in our next development cycle, and most likely we're going to have to figure out a way to handle both old and new.

juliakreger commented 7 months ago

I just realized I somehow was typing in 3.0.35 when I meant 5.0.35. Sorry!

lextm commented 7 months ago

@juliakreger Some update on this,

  1. I think your changes in https://review.opendev.org/c/openstack/ironic/+/909483 are no longer needed.
  2. I submitted some changes in https://review.opendev.org/c/openstack/ironic/+/912328 so that ironic can work with pysnmp-lextudio 6.0.9.
  3. Not quite sure if you still need https://review.opendev.org/c/openstack/ironic/+/910342. Once pysnmp-lextudio 6.0.9 is taken in, the proper version of pyasn1 should be pulled.
elfosardo commented 6 months ago

@lextm thanks for https://review.opendev.org/c/openstack/ironic/+/912328 I left a comment there but in short we need to wait for caracal to be released before moving forward

elfosardo commented 5 months ago

@lextm hi! Now that Openstack Caracal has been released we can move forward with https://review.opendev.org/c/openstack/ironic/+/912328 please rebase it on top of the latest ironic master when you get the chance, thanks!

lextm commented 5 months ago

@elfosardo Just rebased. Also bumped pysnmp-lextudio to 6.1.2 to include all recent bugfixes.

elfosardo commented 5 months ago

@lextm thanks! I've submitted https://review.opendev.org/c/openstack/requirements/+/915830 to update the upper constraint, once that merged we can retest and merge yours

elfosardo commented 3 months ago

@lextm the upper constraints patch has been merged in openstack upstream, but unit tests in your ironic patch https://review.opendev.org/c/openstack/ironic/+/912328 are failing, can you please have a look?

elfosardo commented 3 months ago

@lextm unfortunately pysnmp 6.x doesn't seem to play very well with Python threads due to the migration to asyncio, we found issues also in virtualpdu https://review.opendev.org/c/openstack/virtualpdu/+/922158 where the thread can't be stopped and all tests just go in timeout

lextm commented 2 months ago

@elfosardo We are not able to help much due to resource constraints, as Python/asyncio doesn't seem to make sync wrappers over asyncio based async API a feasible task (while other languages natively support that).

Since we just took over the PySNMP packages like pysnmp, pysmi on PyPI, the recommended approach is changed as below,

  1. Migrate back to pysnmp, and cap on its release 5.1.0. That release keeps the legacy asyncore based sync API, so downstream projects like OpenStack can just compile and run.
  2. Upgrade to release 6.2 when your project has enough resources to clean up all PySNMP usage, and fully converted to asyncio based async API. Projects like Home Assistant have done that already.

We stop maintaining packages with -lextudio postfix from now on.