| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-01-15 | |||
| 15:55:40 | sean-k-mooney | im going to check the latest ci run and see if we see the error in the logs | |
| 15:55:56 | JayF | I am always happy to be a sounding board for eventlet stuff, but at some of this level I can't keep it all straight | |
| 15:56:24 | sean-k-mooney | ah dambit | |
| 15:56:39 | sean-k-mooney | it just triggred in my debug console | |
| 15:57:26 | JayF | is it possible this is whatever the opposite of a hesenbug is? the debug console itself causing the headache? | |
| 15:57:29 | sean-k-mooney | this is why its so annoying to debug. its been fine without the issue for the 5-10 mins we have been chatting | |
| 15:57:43 | sean-k-mooney | we are seeing it in the gate too | |
| 15:58:04 | sean-k-mooney | i did a partial fix patch that was enough to make the tempest jobs pass | |
| 15:58:19 | sean-k-mooney | then then i was inpsectign the logs and notice that we still had those runtime errors | |
| 15:58:52 | sean-k-mooney | so i think my orgianl "fix" just chagned the timing and made it less problematic | |
| 16:00:50 | JayF | I'm really having trouble wrapping my head around eventlet code | |
| 16:00:54 | JayF | that change you linked | |
| 16:01:02 | JayF | eventlet.sleep() is everywhere | |
| 16:01:16 | JayF | and making it so we only call that from coroutines (if I read the code right) seems absolutely brutal | |
| 16:01:28 | JayF | which is why I assumed at first that it must be somehow unpatched | |
| 16:01:44 | sean-k-mooney | right we document if you ever do something long running add eventlet.sleep(0) | |
| 16:01:48 | sean-k-mooney | to yeild | |
| 16:02:19 | sean-k-mooney | or time.sleep if you want to rely on the monkey patching | |
| 16:02:51 | JayF | maybe it's worth asking about that pattern in light of recent code changes in an eventlet issue? | |
| 16:03:26 | sean-k-mooney | oh i never looked at the history of this | |
| 16:03:28 | sean-k-mooney | https://github.com/eventlet/eventlet/commit/48c259c98edb1eb671afeda9800ca4db17b3ccbf | |
| 16:03:31 | JayF | if this is new in this eventlet release, it implies maybe there's a misalignment between how we use it and how they think we use it | |
| 16:03:36 | sean-k-mooney | it was maide a runtime error not an assert | |
| 16:04:00 | JayF | so it still would've failed, yes? | |
| 16:04:14 | sean-k-mooney | no before it was just an assert | |
| 16:04:19 | sean-k-mooney | so it woudl have done nothing at runtime | |
| 16:04:40 | sean-k-mooney | assert are off unless you run your interperter in debug mode | |
| 16:05:17 | sean-k-mooney | this was added in 0.36.0 | |
| 16:07:13 | sean-k-mooney | JayF: so oslo.db injects a time.sleep(0) on every "checkin" event emmited by the sqlalchemy engine | |
| 16:07:15 | sean-k-mooney | https://github.com/openstack/oslo.db/blob/21c484ffb26ed03cbb1ca9dd7fd27fb19a1d4b36/oslo_db/sqlalchemy/engines.py#L360-L361 | |
| 16:07:46 | JayF | aha | |
| 16:07:53 | JayF | sean-k-mooney: I'd file a bug against that change then in eventlet | |
| 16:08:14 | JayF | sean-k-mooney: it's going to break so much stuff in so many sneaky ways | |
| 16:09:26 | sean-k-mooney | JayF: nova's docs have this by the way https://docs.openstack.org/nova/latest/reference/threading.html#yielding-the-thread-in-long-running-tasks | |
| 16:10:05 | JayF | so my comment of "replace the time.sleep() with an if eventlet: eventlet.sleep()" was pretty close to a solution, seemingly? | |
| 16:10:14 | sean-k-mooney | we have documented the use of time.sleep(0) or greenthread.sleep(0) since before we supproted python3 | |
| 16:10:45 | JayF | yeah, so let the eventlet folks know so they can revert the problematic change :) | |
| 16:10:55 | JayF | or at least maybe except sleep(0) | |
| 16:11:08 | sean-k-mooney | JayF: i dont think it will actully change the behivor | |
| 16:11:33 | sean-k-mooney | what i can do is try locally reverting it in my venv | |
| 16:11:38 | sean-k-mooney | and see what happens | |
| 16:12:00 | sean-k-mooney | well hum i guess that wont tell me much | |
| 16:12:43 | sean-k-mooney | it will likely fix the issue but that because asssert are ignored | |
| 16:13:31 | sean-k-mooney | JayF: ill file an eventlet bug and push a revert patch, this is the watcher bug i had previously filed for this https://bugs.launchpad.net/watcher/+bug/2086710 | |
| 16:13:50 | JayF | I suspect the nice middle ground might be | |
| 16:13:59 | JayF | RuntimeError() if the sleep is nonzero | |
| 16:14:02 | JayF | but IMBW | |
| 16:14:11 | JayF | I also don't fully understand the case they're guarding against in eventlet | |
| 16:14:21 | JayF | or maybe I should say; why it's bad to do that | |
| 16:14:29 | sean-k-mooney | ya i think that could work. | |
| 16:14:48 | sean-k-mooney | what we are really using sleep(0) for is to yeild form teh current corotie back to the hub | |
| 16:15:07 | sean-k-mooney | so you could say well we shoudl have done hub.swtich() or the equivlent | |
| 16:15:57 | sean-k-mooney | but im pretty sure the eventlet docs used ot docuemtn time.sleep(0) too | |
| 16:16:59 | sean-k-mooney | JayF: ah the docsting says it all https://github.com/eventlet/eventlet/blob/3450e6adb017f4cd47d1c4e06331c9fa26a986d1/eventlet/greenthread.py#L25-L27 | |
| 16:17:33 | JayF | yeah | |
| 16:17:35 | sean-k-mooney | JayF: so ya i think they just forgot to make this condtional | |
| 16:17:38 | JayF | ++ | |
| 16:17:50 | JayF | If you can file that issue and/or put up a PR; I'll point itamar at it | |
| 16:18:00 | JayF | I can't tell him to work on it, but he's generally cooperative :D | |
| 16:18:34 | sean-k-mooney | thanks for the fresh persepctive i didnt think to see if this was a eventlet regression just because i expected this to kill nova and other things too | |
| 16:23:52 | JayF | I've moved beyond "who regressed" and into "the whole thing is so damn fragile any change can have massive ripple effects" | |
| 16:23:55 | JayF | lol | |
| 16:24:10 | JayF | is it eventlet's fault? is it openstack's fault? yes. all of us. lol | |
| 16:24:46 | sean-k-mooney | well i tought it was an issue with apschduelr because it does nto actully supprot uage with eventlet and is spwaning backgorund threads | |
| 16:25:06 | sean-k-mooney | os i tried to fix it by ensuring the backgound thread was monkey patched initally | |
| 16:25:57 | sean-k-mooney | JayF: watcher is in one optional part of the code also mixing asyncio with eventlet without usign the asyncio hub | |
| 16:26:38 | sean-k-mooney | so i was going down the line "ok watcher is mixing may concurance models so it must be a watcher issue becuase nova is ok" | |
| 16:29:27 | MengyangZhang[m] | Hello, I have a question about git review. I was following the Gerrit workflow tutorial https://docs.opendev.org/opendev/infra-manual/latest/sandbox.html#sandbox and ran into this error when running git review. I... (full message at <https://matrix.org/oftc/media/v1/media/download/AYfP68eKdfAp4GdhSOGMJNoxIcgRvFjHe-ES5V35PeyBtFAsEz-1GYjZH3yveUoKSXQ1CvR5Y8ZkyioBMNdqdLtCeUsuVqzQAG1hdHJpeC5vcmcvUHNabUJzV29SVVdnS2FwYUtYYm11d3RR>) | |
| 16:30:12 | JayF | MengyangZhang[m]: that's a new one on me, perhaps #opendev would have more insight? | |
| 16:30:52 | MengyangZhang[m] | okk thanks for quick response. I will raise it there. | |
| 16:31:08 | JayF | good luck! | |
| 16:31:21 | sean-k-mooney | MengyangZhang[m] so i think this is related to the verion of git you have installed | |
| 16:31:57 | MengyangZhang[m] | mine is 2.39.2 | |
| 16:32:00 | sean-k-mooney | i have not hit that but you coudl work around it by doing a local rebase agaisnt master before doing "git revew" | |
| 16:32:13 | sean-k-mooney | for what its worth we do nto allow merge commits to be part of a review | |
| 16:33:13 | sean-k-mooney | i have git version 2.46.0 | |
| 16:33:20 | sean-k-mooney | so your version is not that old | |
| 16:33:24 | JayF | I've absolutely used git review w/git 2.39.2 at some point, I think it's unlikely to be that | |
| 16:33:33 | JayF | sean-k-mooney may have id'd the issue -- if there are any merge commits in your push | |
| 16:34:15 | sean-k-mooney | you probaly dont want pull.rebase=merges in your git cofnig either | |
| 16:35:33 | sean-k-mooney | i woudl set that to pull.rebase=true | |
| 16:35:42 | sean-k-mooney | or just unset it to get the default behvior | |
| 16:38:53 | MengyangZhang[m] | ``` | |
| 16:38:53 | MengyangZhang[m] | Thanks for the help! So i did a local rebase agains master and tried git review again but still same error | |
| 16:39:10 | MengyangZhang[m] | Current branch mzhang-branch is up to date. | |
| 16:39:10 | MengyangZhang[m] | git rebase master | |
| 16:39:10 | MengyangZhang[m] | * Thanks for the help! So i did a local rebase agains master and tried git review again but still same error | |
| 16:42:05 | JayF | Look at the git config changes that Sean suggested, perhaps? | |
| 16:43:41 | sean-k-mooney | so you shoudl do it like this | |
| 16:43:47 | sean-k-mooney | git fetch --all | |
| 16:43:56 | sean-k-mooney | git rebase -i gerrit/master | |
| 16:44:00 | sean-k-mooney | git review | |
| 16:44:29 | sean-k-mooney | if your local master is behind origin or gerrit master there may be a remote merge conflict that you are not seeing | |
| 16:47:02 | sean-k-mooney | JayF: ill see if i can find time to push a ptch to add teh if check later but i need to step away for a bit, this is the bug https://github.com/eventlet/eventlet/issues/1014 | |
| 16:54:38 | JayF | sean-k-mooney: sounds like Itamar agrees with our suggested fix, will work on it soon :) | |
| 16:55:35 | sean-k-mooney | oh awsome | |
| 16:56:29 | sean-k-mooney | im going to try hackign it in locally to confirm that it does nto fail later but that woudl be good if they had time | |
| 16:59:38 | JayF | it's a good fix regardless of if it resolves the watcher issue | |
| 16:59:46 | JayF | but hopefully it cleans up both | |
| 18:16:12 | opendevreview | ribaudr proposed openstack/nova-specs master: Fix a line that was not removed before merging the specification https://review.opendev.org/c/openstack/nova-specs/+/939380 | |