Kombu 4.6.4 breaks Openstack with python 3.7

[2uasimojo]

I'll start by saying I don't really know what kombu is/does. I'm basically proxying for [1].

When we upgraded to kombu 4.6.4, we started seeing failures like this in python3.7:

2019-08-16 22:56:25.446 | File "/usr/local/lib/python3.7/dist-packages/eventlet/green/os.py", line 107, in open
2019-08-16 22:56:25.446 | fd = original_open(file, flags, mode, dir_fd=dir_fd)
2019-08-16 22:56:25.446 | TypeError: open: path should be string, bytes or os.PathLike, not _NormalAccessor

Here's some reports from other places [2][3][4].

I'm here because I was asked [5] to link to a kombu bug.

Thanks,
efried

[1] http://lists.openstack.org/pipermail/openstack-discuss/2019-August/008553.html
[2] eventlet/eventlet#534
[3] nameko/nameko#655
[4] https://bugs.launchpad.net/nova/+bug/1840551
[5] http://eavesdrop.openstack.org/irclogs/%23openstack-release/%23openstack-release.2019-08-17.log.html#t2019-08-17T17:45:16

[auvipy]

are you using kombu 4.6.4 with celery 4.4.0rc3?

[2uasimojo]

In my local repro of the problem, I don't have any celery at all.

[Later] ...but when I install that rev of celery into the venv, the problem still occurs. Ditto with the rev of celery that installs by default, 4.3.0.

[matteius]

@2uasimojo Any idea what your file variable is then thats being passed in fits not a string, bytes or os.PathLike. I'll start by saying I have no idea what a_NormalAccessor object is, but when I searched Google for that error the first thing that come up is this eventlet bug and you have eventlet in your stack trace, so I would start there: eventlet/eventlet#534

Kombu is a component of celery that "The author of this package has not provided a project description" on pypi so in some ways its entirely open to interpretation what kombu is/does which kind of makes my job hard. Some believe kombu is an algae or edible kelp from mostly the family Laminariaceae and is widely eaten in East Asia. However I can say in this context its got underlying classes and utilities for managing connections and pools of workers for Celery.

I guess though any joking aside, the docs say kombu is a messaging library and I would have to agree, and you can read more about it here: https://kombu.readthedocs.io/en/stable/introduction.html#about

[matteius]

@2uasimojo Do you have a full stack trace? I just looked at your openstack PRs and see you pinned kombu to 4.6.3 instead of latest 4.6.4 -- so I just diffed the two source tarballs from pypi and looked for any code changes that could explain what you are seeing. With a full stack trace that could make me more confident in what I am saying, but I am not seeing any code changes in that release of kombu that would cause what you are saying but I do see in that release of kombu amqp went from being >= 2.5.0 to being >= 2.5.1. What version of amqp gets installed when it works on kombu 4.6.3? I was looking for where kombu or amqp or one of the downstream requirements might call out to install eventlet, because I do think the os patch thing is specifically a bug in a newer version of eventlet based on reading that report.

[matteius]

Ok I found in that commit for open stack (https://opendev.org/openstack/requirements/commit/b236f0af43259959cb2a0f82880cebbdd0da7f27) that amqp was moved from being pinned on 2.5.0 to 2.5.1 -- I would ask also that you try it this way to help me troubleshoot, try using for now in the CI the newer version of kombu with the prior version of amqp and see if tests pass so:
kombu==4.6.4
amqp==2.5.0

[2uasimojo]

Thanks Matt. I tried that out, with the same result. In case it helps, here's a stack trace (for one test): http://paste.openstack.org/show/760022/

[matteius]

@2uasimojo Thanks for providing that stack trace, very useful, and has helped me realize that your error happens here:
b' File "/opt/stack/nova/.tox/py37/lib/python3.7/site-packages/kombu/utils/compat.py", line 10, in '
b' import importlib_metadata'
Which actually was a change in Kombu 4.6.3 -> 4.6.4 in the file compat.py in the method definition def entrypoints. It used to use from pkg_resources import iter_entry_points but was changed to use importlib_metadata and the import was moved to the top of the file (the old one existed inside a try/except: block).

Here is where the change was introduced to save 150 ms:
#1071

Feel free to pin back to the latest amqp, I think this change is kombu is the issue you are reporting.

[2uasimojo]

Great \o/
Thanks for the analysis.
I don't know how this works, but I assume 4.6.4 will remain dead to us, and this issue will be fixed (somehow) in a subsequent version, which will allow us to uncap and keep tracking the latest.

[matteius]

Generally yes @2uasimojo but we gotta wait to hear from @davidszotten and @auvipy since they were the ones to curate that original change. If you ask me 150 ms isn't worth it for something that gets run once on setup or startup, and that we should just revert to the prior method. However the prior method was wrapped in a try/except so its possible that would also have not worked if it was imported at top, so maybe we just move the new importlib_metadata import down to inside the entrypoint method where the usage is inside a try/except like before.

[davidszotten]

Hi,

My 2 cents:

While 150ms may be considered small "once at startup" for you in production, that's quite a lot when e.g. repeatedly running a small test suite.

From the links above it looks like the underlying issue is that eventlet monkey patching breaks pathlib. pathlib is getting more common, and while this may be the trigger at the moment, i suspect that sooner or later some other dependency will start using so we should really be looking at how to fix this in eventlet

[2uasimojo]

That would be lovely.

[davidszotten]

ref: eventlet/eventlet#584

[2uasimojo]

We already had eventlet/eventlet#534 (see the opening comment of this issue).

[Later] Okay, I see you've already cross-referenced... or something. (I'm very unfamiliar with how things are done in github-land.)

[davidszotten]

Yes, that's the issue (=bug report), i was linking to a pr with a fix (that also references the original issue)

[matteius]

Well from my perspective it really can only be argued that 150 ms is a large time for something that encounters so many iterations in a production environment that it is noticeable. My preference is for the test suite to take 15 seconds longer (arbitrary number) than to ever introduce a production bug. That being said -- I am not sure if its importlib_metadata thats the problem or how its used in the file. The old usage there only imported inside the call to the entrypoints method and inside of a try:/except: block to keep anything from blowing up if there were an error.

It is possible that importlib_metadata could work if moved to the same spot of the code there, and its also possible that doing so will incur the import every single time the method entrypoints is called, which in my mind may equate to about 150 ms for an import (maybe?) and that could cause the test test "slowness" that you originally described. Still though from a code correctness stand point, the change introduced a regression for production uses that also use eventles (still supported) and so we must correct the regression inside of kombu--one way that will work for certain is to revert the prior change. The other way if we really think importlib_metadata is better -- move it to a try/except: and see if that fixes it, and not worry about the extra 2 seconds running the test suite.

[davidszotten]

eventlet 0.25.1 has been released with a fix for the pathlib issue

[2uasimojo]

Thanks David! I've tested locally with kombu 4.6.4 and eventlet 0.25.1 and the problem seems to be resolved. We'll blacklist eventlet 0.25.0 and move on.

[2uasimojo]

FYI https://review.opendev.org/678078