| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-10-24 | |||
| 17:45:22 | opendevreview | Mengyang Zhang proposed openstack/nova-specs master: Add Burst Length Support to Cinder QoS https://review.opendev.org/c/openstack/nova-specs/+/932653 | |
| 17:58:24 | dansmith | tkajinam: sean-k-mooney should probably look at that | |
| 18:07:40 | sean-k-mooney | i dont recall if that is one of the things i was trying to clean up. | |
| 18:07:57 | sean-k-mooney | glancing at it if the bug in eventlet is already fixed then it soudn like an easy win | |
| 18:08:07 | sean-k-mooney | ill add myslef as a reviewer | |
| 18:11:35 | frickler | fyi, mentioned this in the oslo channel earlier: bumping to latest eventlet seems to break a couple of projects, including nova https://review.opendev.org/c/openstack/requirements/+/933257 | |
| 18:12:42 | frickler | hberaud mentioned a change that makes read/write calls actually non-blocking, which might trigger stuff | |
| 18:13:07 | frickler | https://github.com/eventlet/eventlet/issues/973 | |
| 18:13:52 | sean-k-mooney | os.read is ment to block but yeild execution fo the current greenthread | |
| 18:14:44 | JayF | that's going to be sneaky breaking for Ironic/IPA as well, I'll note | |
| 18:14:56 | JayF | likely would cause issues with image streaming error handling (timeouts/disk failures) | |
| 18:15:20 | JayF | stuff I wouldn't 100% expect to show up in happy path testing; but def. not unit tests | |
| 18:15:30 | JayF | oh, thought this was -requirements, either way, info is good :D | |
| 18:15:32 | sean-k-mooney | i have nto actully looked at the change in detail but reads and write need to be blocking and yeild | |
| 18:17:21 | frickler | I completely do not rule out the possibility that this is actually a bug in eventlet | |
| 18:17:25 | dansmith | um, is this a regression in eventlet? I was thinking it only monkeypatched the network IO bits | |
| 18:17:43 | sean-k-mooney | it monkeypatches file io too | |
| 18:17:46 | dansmith | like, is this change actually not part of the original intent of eventlet? | |
| 18:18:27 | sean-k-mooney | eventlet shoudl not make any standard lib fucntion that is normally blocking non blocking form the point of view of the caller fo the function | |
| 18:18:37 | frickler | best discuss in the oslo channel then where herve is actually around | |
| 18:19:01 | dansmith | right, and I think the change doesn't actually change the behavior, it just adds trampoline calls in between non-blocking reads | |
| 18:19:28 | sean-k-mooney | it may be doing somethign valid yes | |
| 18:19:39 | sean-k-mooney | frickler: do you have a like to the nova failure? | |
| 18:20:06 | sean-k-mooney | oh i see them | |
| 18:20:33 | frickler | sean-k-mooney: in the reqs patch I linked, both py311 and functional failing | |
| 18:21:18 | dansmith | sean-k-mooney: I don't see that it patches os.open, read, write: https://eventlet.readthedocs.io/en/latest/patching.html | |
| 18:22:01 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/monkey_patch.py#L44-L52 | |
| 18:22:20 | JayF | https://github.com/eventlet/eventlet/blob/master/eventlet/green/os.py#L22 | |
| 18:23:07 | dansmith | okay I guess I'm not sure that necessarily means it patches open() | |
| 18:23:16 | sean-k-mooney | os is one of the one it default patches | |
| 18:23:19 | JayF | it patches fdopen(), read(), write() and others outta tha library https://github.com/eventlet/eventlet/blob/master/eventlet/patcher.py#L533 | |
| 18:23:32 | dansmith | I say that because mocking open is different than other things and I thought it has (for quite a while) been a C function | |
| 18:24:00 | JayF | Yeah, eventlet overrides more than one C function and replaces it with a python-native version | |
| 18:24:10 | sean-k-mooney | by the way the unit and functional test patches dont obviously look like eventlet falirues | |
| 18:24:11 | JayF | that's how the major 3.12 blocker was resolved | |
| 18:24:19 | dansmith | I know it does, I just didn't think open() itself | |
| 18:25:35 | sean-k-mooney | https://github.com/eventlet/eventlet/blob/master/eventlet/green/builtin.py | |
| 18:25:46 | JayF | docstring says: Wrap os.open; This behaves identically, but collaborates with the hub's notify_opened protocol. ... which implies we're both right-ish? | |
| 18:26:16 | JayF | patched but no significant behavior change | |
| 18:26:34 | sean-k-mooney | ya it looks more or less the same | |
| 18:26:44 | sean-k-mooney | its just setting up a notifyier on the file descriptor | |
| 18:26:51 | sean-k-mooney | to know when the file is ready i guess | |
| 18:27:20 | JayF | sean-k-mooney: my knowledge of eventlet mostly stops at the patcher module :D hehe | |
| 18:27:46 | dansmith | idk, ages ago (like icehouse timeframe) when we were transitioning from the mysql C client to the python-native one, I thought that we encountered a situation where local file i/o was not patched and that surprised me, and some of the elders explained that it wasn't in scope | |
| 18:28:30 | sean-k-mooney | i mean we can disabel patchign of this | |
| 18:28:36 | dansmith | but that was a long time ago and we've shuffled our own monkey patching a lot since then, so maybe we weren't patching os at the time, or maybe I'm just misremembering | |
| 18:28:40 | sean-k-mooney | we coudl set os=false adn builtins=false | |
| 18:29:04 | sean-k-mooney | we disabel os on windows for posix reasons | |
| 18:29:30 | sean-k-mooney | so i can push a patch and see what that does but i need to look at the tempest job | |
| 18:29:51 | sean-k-mooney | the unit/fucniton test failrues dont really tell us a lot about what the actual effect of the chagne is | |
| 18:30:56 | sean-k-mooney | ok devstack only deploy nova-api neutron failed to start so we didnt finish stacking | |
| 18:31:19 | sean-k-mooney | nova api seams to be up | |
| 18:31:54 | sean-k-mooney | neutron.plugins.ml2.ovo_rpc RuntimeError: cannot notify on un-acquired lock | |
| 18:32:02 | sean-k-mooney | that is the neutron server error | |
| 18:32:38 | sean-k-mooney | oh thats coming form oslo messaging | |
| 18:32:42 | sean-k-mooney | https://5ce7fa2fbddee5b97128-6f49bdea8e90fe6675f1ae011c4f0f22.ssl.cf5.rackcdn.com/933257/1/check/tempest-full-py3/018987a/controller/logs/screen-q-svc.txt | |
| 18:36:36 | sean-k-mooney | frickler: i think its better to phase this as the new eventlet release is breaking oslo adn that is breakign everything else | |
| 18:36:58 | sean-k-mooney | the nova unit/funtional test failure might eb related or flaky tests so i woudl try at least one recheck to confirm that | |
| 18:39:31 | sean-k-mooney | frickler: the fact the devstack functional job worked is also odd | |
| 18:40:03 | dansmith | JayF: sean-k-mooney: https://github.com/openstack/nova/blob/icehouse-eol/nova/cmd/__init__.py | |
| 18:40:05 | dansmith | os=False | |
| 18:40:39 | sean-k-mooney | rigth so we still do that for winwos | |
| 18:40:42 | sean-k-mooney | *windows | |
| 18:40:58 | dansmith | but that was for every binary on every platform, AFAIK | |
| 18:41:00 | JayF | I think that was required to be added some point around t? u? v? era-ish for greendns to behave properly? My memory is very lossy but I remember having to back os=False outta IPA at some point | |
| 18:41:03 | dansmith | ...at the time | |
| 18:41:20 | sean-k-mooney | it was yes | |
| 18:41:28 | sean-k-mooney | im trying to see when we changed it | |
| 18:41:43 | sean-k-mooney | we sould be able to go back but i am not sure its breaking nova | |
| 18:42:22 | dansmith | it was that way through rocky at least | |
| 18:42:42 | dansmith | I lose the thread when the refactor to monkey_patch.py happened | |
| 18:42:48 | dansmith | anyway, | |
| 18:43:05 | dansmith | I'm not saying we should, I'm defending why I thought the os.open thing wouldn't affect us :) | |
| 18:43:08 | dansmith | and proving that I'm old | |
| 18:43:22 | sean-k-mooney | dansmith: the sdk devstack tests passed https://5355e5989bc73779bf10-cc682349d9014dd293929a9a1be5247d.ssl.cf5.rackcdn.com/933257/1/check/openstacksdk-functional-devstack/fad2996/testr_results.html | |
| 18:46:20 | sean-k-mooney | dansmith: https://github.com/openstack/nova/commit/4f2d4d03f3a1a0a277dd04ec2abcbee2d4018fa2 | |
| 18:47:27 | sean-k-mooney | so there was no reason given why it orginally was false and also no reason given why we shoudl remov it | |
| 18:47:38 | dansmith | yeah :) | |
| 18:47:55 | sean-k-mooney | dansmith: by the way i also remember os=false for what its worth | |
| 18:48:24 | sean-k-mooney | as in that what i expected to see on master | |
| 18:48:52 | sean-k-mooney | for now i just recheck the requiremnt patch to see if things are failing the same way consitently | |
| 18:49:51 | sean-k-mooney | hum we changed in stein | |
| 18:58:26 | opendevreview | Ghanshyam proposed openstack/osc-placement master: Update python classifier in setup.cfg https://review.opendev.org/c/openstack/osc-placement/+/931113 | |
| 19:12:03 | sean-k-mooney | frickler: i was able to repoduce the nova.tests.unit.test_service.TestWSGIServer.test_server_pool_waitall failure locally | |
| 19:13:30 | sean-k-mooney | nova.tests.unit.test_service.TestWSGIService has two failing unit tests for me too because we are not stubing calsl to bind but that happened with our without the new eventlet | |
| 19:24:15 | frickler | sean-k-mooney: maybe the results also vary between python versions/distros. the cross jobs run with py311 on bookworm | |
| 19:28:33 | sean-k-mooney | im using debian testing on 3.12 | |
| 19:28:46 | sean-k-mooney | frickler: the unit test fialure is because its too specific | |
| 19:29:03 | sean-k-mooney | the unit test is ment to assert that waitall is called when we shutdown the wsgi server | |
| 19:29:17 | sean-k-mooney | but its asserting its called once and now its called twice | |
| 19:29:25 | sean-k-mooney | so the fix for that is just change the assert | |
| 19:29:39 | sean-k-mooney | im repoducing the functional test failure now | |
| 19:40:38 | sean-k-mooney | the functionatl test that is failing is restarting the scheduler and that reulst in self.service_ref being None | |
| 19:40:55 | sean-k-mooney | i think this is an artifact of how we are doing that | |
| 19:41:45 | sean-k-mooney | restarting services in our functional tests is kind of a hack | |
| 19:42:19 | sean-k-mooney | so again i think this will be a case of a slight test tweak but not an actual regression | |
| 19:47:37 | opendevreview | sean mooney proposed openstack/nova master: [eventlet] update nova tests for eventlet 0.37.0 https://review.opendev.org/c/openstack/nova/+/933365 | |
| 19:47:45 | sean-k-mooney | frickler: ^ i think that will do it | |
| 19:49:13 | sean-k-mooney | i dont think that matrially changes the intent of any of those tests so if that passes ci we can merge it and at least get those jobs passign with eventlet 0.37.0 | |
| 19:49:25 | sean-k-mooney | i can try installing that in a devstack tomorrow or next week | |