| *** mhen_ is now known as mhen | 01:57 | |
| dansmith | croelandt: you're working on functional test de-eventletification right? | 13:42 |
|---|---|---|
| croelandt | tkajinam: yes, I meant to do that. Fixed it :) | 13:56 |
| croelandt | dansmith: so I did a bunch of the inital work, abhishek took over for some more, and I'm not sure how much we have left, let me check | 13:56 |
| dansmith | oh okay | 13:57 |
| croelandt | oh we still have a dozen files that reference FunctionalTest | 13:58 |
| dansmith | okay | 13:59 |
| dansmith | maybe we should try to get that done in G? | 13:59 |
| croelandt | definitely | 13:59 |
| dansmith | has anyone done any checking on trying to remove eventlet from the venv to see if all the runtime code is really clean? | 13:59 |
| dansmith | I assume we still have some references to it that could be commented out (imports and things) but.. I hope we should fully run without those | 14:00 |
| croelandt | and running like unit tests? | 14:00 |
| dansmith | no, I mean running like real life.. like a devstack | 14:00 |
| croelandt | So I'm pretty sure devstack runs Glance without eventlet these days | 14:05 |
| croelandt | stephenfin: can you confirm this? | 14:05 |
| dansmith | croelandt: in config yes, I mean.. we might should make sure we can actually remove it | 14:05 |
| dansmith | nova-api runs without it as well, but we still use eventlet utilities incidentally (until recently) | 14:05 |
| stephenfin | croelandt: It can still technically deploy via it but it doesn't do in any gate job that I'm aware of. See https://review.opendev.org/c/openstack/devstack/+/932201 | 14:07 |
| stephenfin | Though as dansmith notes, there's more to removing eventlet than simply not using eventlet server | 14:07 |
| stephenfin | Probably would be no harm submit a DNM (for now) patch that deletes any imports of eventlet and oslo.service and see what explodes? | 14:09 |
| stephenfin | s/eventlet and oslo.service/eventlet/ seems there's no oslo.service in glance #TIL | 14:10 |
| JayF | You'll also need to explicitly set the backend to threading in oslo_service | 14:10 |
| JayF | oh, neat | 14:10 |
| croelandt | hm interesting | 14:10 |
| stephenfin | JayF: Maybe. Depends on what, if anything, it's using instead | 14:10 |
| croelandt | I could try to build a patch that removes all eventlet references from the runtime code | 14:10 |
| croelandt | and see how bad things become | 14:10 |
| croelandt | dansmith: how does that sound? | 14:10 |
| dansmith | yep, only removing the eventlet import should be required for glance | 14:11 |
| croelandt | (this reminds me of the evenlet.posthooks weird stuff we have :p) | 14:11 |
| dansmith | croelandt: sure, I wasn't saying you had to do this, I was just wondering if you had tried | 14:11 |
| stephenfin | It would certainly be a fun experiment. Deleting code is one of my favourite pastimes | 14:11 |
| croelandt | nah but I'll see if that's doable, should be fun | 14:11 |
| dansmith | like, if we kept eventlet longer just for the way our tests run, it would be nice to say that at least at _runtime_ we're clean | 14:11 |
| croelandt | stephenfin: so amazing when you read the stats of a patch and it's +134 - 890 :D | 14:11 |
| stephenfin | The original no code movement | 14:12 |
| croelandt | dansmith: yeah. There are some weird cases as well. Like in the glanceclient, we check whether we are running in a patched environment, and according to hberaud I cannot just remove that, because it might cause issues if we are running in a patched environment but do not do anything specific. So that has to go last. What a freaking headache | 14:13 |
| dansmith | croelandt: right that sort of thing is what I'm talking about.. there are also things we might use like eventlet.Event that still work in threading and thus we might not realize that just running in threaded mode, we're still using eventlet bits | 14:13 |
| dansmith | which is worse when you find out that their Event is a superset of threading.Event, and we might be using a thing that doesn't exist in the regular implementation | 14:14 |
| croelandt | and I also wanted to try 3.14rc1 soon, which I'm sure will bring other eventlet bugs | 14:14 |
| croelandt | F-U-N | 14:14 |
| croelandt | ok just getting the unit tests to properly run is fun :D | 15:15 |
| tkajinam | IIUC as long as you don't use standalone glance-api eventlet is not used | 15:24 |
| tkajinam | it may be imported but no patching may be done | 15:24 |
| tkajinam | because glance/cmd/api.py is the only place calling monkey_patch, but in uwsgi/mod_wsgi everything starts from different places like glance.wsgi.api or glance.common.wsgi_app | 15:25 |
| tkajinam | ( I see you are saying the same thing. I had to scroll up the log before I dumped these :-P | 15:27 |
| dansmith | tkajinam: yep :) | 15:37 |
| tkajinam | maybe the remaining question is whether you want to keep glance-api by replacing eventlet by something else like cheroot | 15:38 |
| tkajinam | as was done in a few projects like ironic | 15:38 |
| tkajinam | or even aim to drop glance-api script suggesting users to use external mechanism such as uwsgi or gunicorn to run api | 15:38 |
| dansmith | personally, I'd rather just have one way, which is a wsgi app and let them use gunicorn or uwsgi or whatever | 15:40 |
| tkajinam | (for example nova decided to drop it, because nova-api was deprecated long ago due to recommendation to use wsgi app | 15:40 |
| tkajinam | yeah that's aligned with my own preference | 15:40 |
| dansmith | cool | 15:41 |
| tkajinam | less code, better life :-) | 15:42 |
| opendevreview | Cyril Roelandt proposed openstack/glance master: DNM Eventlet removal https://review.opendev.org/c/openstack/glance/+/960401 | 15:52 |
| dansmith | croelandt: there's no easy replacement for eventlet.Timeout :) | 15:53 |
| dansmith | but yeah, hopefully that exercise was worthwhile to see all the places we reference it if we had to rip it out quickly | 15:54 |
| croelandt | you already read it, damn | 15:55 |
| croelandt | there is weird stuff here, like cmd/api.py is not really used if you run Glance as a wsgi app | 15:55 |
| croelandt | but yeah it's interesting | 15:55 |
| dansmith | yeah | 15:55 |
| croelandt | In 2 hours we'll see what actually fails :D | 15:55 |
| tkajinam | looks like sqlite cache driver uses eventlet ? | 15:57 |
| tkajinam | I guess it has been broken in wsgi environment | 15:57 |
| tkajinam | maybe we can just remove it at F then everything would be sorted :-) | 15:57 |
| dansmith | that's deprecated anyway AFAIK | 15:59 |
| tkajinam | yeah at C | 15:59 |
| tkajinam | so can be removed at F for sure | 15:59 |
| tkajinam | (I meant G = 2026.1 | 15:59 |
| tkajinam | I'm always confused by recent release names due to yyyy.n mainly used :-P | 16:00 |
| dansmith | better to use the release number, IMHO | 16:00 |
| croelandt | https://review.opendev.org/c/openstack/glance/+/930027 see Abhishek's comment | 16:00 |
| dansmith | once it wrapped around to re-use letters I've lost my context | 16:00 |
| dansmith | croelandt: yeah just too late for F now | 16:01 |
| opendevreview | Merged openstack/glance master: Use Ubuntu Noble instead of Jammy https://review.opendev.org/c/openstack/glance/+/958988 | 16:02 |
| croelandt | yeah and we apparently need to not use the driver in the functional tests | 16:04 |
| croelandt | but it's going away at some point | 16:04 |
| dansmith | yeah | 16:04 |
| croelandt | nova-ceph-multistore failed | 16:10 |
| croelandt | die 686 'g-api did not start' | 16:10 |
| croelandt | hm interesting | 16:10 |
| croelandt | somehow that job must not be using WSGI | 16:10 |
| croelandt | hm, maybe I should not have remove glance-api from setup.cfg though | 16:14 |
| croelandt | tkajinam: do you understand how we start glance with WSGI in devstack? | 16:14 |
| dansmith | croelandt: we start uwsgi to load it right? | 16:18 |
| croelandt | I think so | 16:18 |
| croelandt | we end up at https://github.com/openstack/devstack/blob/master/lib/glance#L686 | 16:19 |
| croelandt | and the only place where glance-wsgi-api is mentioned is in cleanup_glance() | 16:20 |
| croelandt | so I'm a bit confused as to how this works | 16:20 |
| tkajinam | croelandt, https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_dda/openstack/ddab9b6eb4b54e4b9b86e8a5629d923f/controller/logs/etc/glance/glance-uwsgi.ini | 16:23 |
| tkajinam | module = glance.wsgi.api:application | 16:24 |
| tkajinam | this is how uwsgi loads the wsgi app | 16:24 |
| dansmith | yeah, | 16:24 |
| dansmith | so setup.cfg changes could break the installability/loadability of the module | 16:24 |
| dansmith | although I don't see that the changes you made would be problematic | 16:25 |
| dansmith | maybe more likely that we failed to import because you removed an eventlet import that we use at import time maybe? | 16:25 |
| dansmith | but clearly it did start | 16:26 |
| tkajinam | assuming you are looking at https://zuul.opendev.org/t/openstack/build/051e7fab42664b46a962b0e6b51df378 | 16:26 |
| dansmith | er wait, maybe those are not the failed logs | 16:26 |
| dansmith | yeah I see, I was following your wsgi.ini link to other logs | 16:27 |
| tkajinam | yeah I picked it from a different devstack job | 16:27 |
| tkajinam | https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_051/openstack/051e7fab42664b46a962b0e6b51df378/controller/logs/services.txt | 16:27 |
| dansmith | nothing helpful in the g-api log.. maybe need to test locally | 16:28 |
| tkajinam | × devstack@g-api.service - Devstack devstack@g-api.service | 16:28 |
| tkajinam | Loaded: loaded (/etc/systemd/system/devstack@g-api.service; enabled; preset: enabled) | 16:28 |
| tkajinam | Active: failed (Result: exit-code) since Wed 2025-09-10 16:05:49 UTC; 1min 48s ago | 16:28 |
| tkajinam | Duration: 1ms | 16:28 |
| tkajinam | Process: 102121 ExecStart=/opt/stack/data/venv/bin/glance-api --config-dir=/etc/glance (code=exited, status=203/EXEC) | 16:28 |
| tkajinam | Main PID: 102121 (code=exited, status=203/EXEC) | 16:28 |
| dansmith | yeah | 16:28 |
| tkajinam | so it seems that multi store job is using standalone g-api | 16:28 |
| tkajinam | which we should switch | 16:28 |
| dansmith | but how could it be? | 16:28 |
| dansmith | we removed that flag from devstack no? | 16:28 |
| dansmith | oh sorry that's not merged I see | 16:29 |
| tkajinam | https://opendev.org/openstack/nova/src/branch/master/.zuul.yaml#L699 | 16:29 |
| dansmith | https://review.opendev.org/c/openstack/devstack/+/932201 | 16:29 |
| tkajinam | https://codesearch.opendev.org/?q=GLANCE_STANDALONE%3A%20%5BtT%5Drue&i=nope&literal=nope&files=&excludeFiles=&repos= | 16:30 |
| tkajinam | https://codesearch.opendev.org/?q=WSGI_MODE%3A&i=nope&literal=nope&files=&excludeFiles=&repos= | 16:31 |
| tkajinam | so standalone glance-api is used only when GLANCE_STANDALONE=True or WSGI_MODE!=uwsgi | 16:31 |
| tkajinam | and that multistore job in nova only meets that condition (due to GLANCE_STANDALONE: True in that zuul.yaml) | 16:31 |
| dansmith | yeah, that may have been leftover from just wanting some of the jobs to be standalone early on, or before import worked in wsgi mode, idk | 16:32 |
| tkajinam | croelandt, https://review.opendev.org/c/openstack/nova/+/960404 | 16:33 |
| dansmith | croelandt: maybe just flip that flag off in a patch before your DNM, and if everything is cool, we can merge that? | 16:33 |
| tkajinam | add Depend-on to that for testing | 16:33 |
| dansmith | yeah | 16:33 |
| tkajinam | Depends-on | 16:33 |
| tkajinam | If that DNM patch for nova can pass then we can make it formal proposal | 16:33 |
| tkajinam | but for now we can probably keep it to see if the other jobs (which are supposed to use uwsgi) can pass | 16:34 |
| croelandt | dansmith: ok trying the Depends-On thing | 16:50 |
| opendevreview | Cyril Roelandt proposed openstack/glance master: DNM Eventlet removal https://review.opendev.org/c/openstack/glance/+/960401 | 16:51 |
| croelandt | dansmith: ok so 9 failures for the nova job, other than that s3-multistore also fails | 18:48 |
| croelandt | could be worse, I think | 18:48 |
| dansmith | looks like failed imports, which would be the part where we need to thread | 18:53 |
| dansmith | so I mean.. maybe it "could be worse" but it also might mean that we're not as clean as we think | 18:54 |
| dansmith | ah, I bet this is missing the python interpreter config maybe | 18:54 |
| dansmith | https://zuul.opendev.org/t/openstack/build/92b1b1137ffb4b66963391d9553d0f0e/log/controller/logs/screen-g-api.txt#17771 | 18:55 |
| dansmith | there's a [wsgi]python_interpreter= you need to set when we're running in wsgi mode in order to be able to do things that require exec'ing sys.argv[0] like privsep | 18:55 |
| dansmith | since that job was previously standalone, might just need to do that here | 18:56 |
| dansmith | ideally that's done in the devstack de-standalone-ing patch always | 18:56 |
| croelandt | stephenfin: does that sound doable? ^ | 19:21 |
| dansmith | croelandt: um, what? | 19:53 |
| dansmith | croelandt: just set this on your job: https://github.com/openstack/glance/blob/master/.zuul.yaml#L235 | 19:54 |
| dansmith | the patch to remove standalone mode needs to set that ^ for us always instead of us encoding it in all our jobs, which we only do for the ones that are both not standalone and have import enabled | 19:55 |
| croelandt | dansmith: yeah so this should be part of devstack, right | 20:54 |
| dansmith | today, devstack should probably set it "if wsgi_mode and import_enabled" yeah | 20:54 |
| dansmith | but, there's no reason not to just always enable it | 20:54 |
| dansmith | er, s/enable/set/ | 20:55 |
| croelandt | do real world installations haev to set up python_interpreter as well? Or can it somehow default to "which python"? | 20:55 |
| dansmith | ...yeah it's real :) | 20:55 |
| dansmith | https://github.com/openstack/glance/blob/master/glance/common/config.py#L651 | 20:58 |
| croelandt | oh right | 21:03 |
| croelandt | Gotta make sure Red Hat does that properly :) | 21:03 |
| dansmith | well, again, it's only actually important for import | 21:04 |
| dansmith | and tbh I suspect not many people use it :D | 21:04 |
| croelandt | oh but I cannot set python_interpreter for nova-ceph-multistore in Glance's zuul.yaml, I'd have to redefine a job that inherits from nova-ceph-multistore, so I guess I should rather change it in Nova's zuul.yaml | 21:06 |
| dansmith | you can change it in tkajinam's patch | 21:07 |
| croelandt | ok let's do that | 21:08 |
| dansmith | AFAIK, the devstack patch to remove standalone mode would pass devstack's set but would have broken nova and glance's gate because of this | 21:17 |
| croelandt | ok maybe we revisit that patch in the future | 21:17 |
| croelandt | damn I probably should have edited s3-multistore config as well | 21:18 |
| croelandt | but it's late, let's see if that helps with Nova and then we'll fix s3 | 21:18 |
| dansmith | ack | 21:18 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!