| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-05-06 | |||
| 12:55:25 | sean-k-mooney | and only the split endpoints | |
| 12:55:27 | stephenfin | yeah, hence my concern | |
| 12:55:37 | stephenfin | but it sounds like it's just worded weirdly | |
| 12:55:39 | opendevreview | Merged openstack/osc-placement stable/2025.1: Add bindep.txt for ubunutu 24.04 support https://review.opendev.org/c/openstack/osc-placement/+/948867 | |
| 12:55:48 | sean-k-mooney | we used to have seperate console script for nova-api and nvoa-api-metadata | |
| 12:55:51 | stephenfin | and I can rephrase like so | |
| 12:56:07 | sean-k-mooney | i think gibi dropped the refences to the console scripts when they were remvoed | |
| 12:56:28 | stephenfin | > The ``api.local_metadata_per_cell`` option must be set to ``False`` | |
| 12:56:31 | sean-k-mooney | but keepd the verbage related to the pbr wsgi scripts | |
| 12:56:35 | stephenfin | Or drop the sentence entirely | |
| 12:56:40 | gibi | yeah I tried to cleanup the doc when the eventlet server is removed but probably missed things | |
| 12:57:02 | sean-k-mooney | it need to be set based on teh toplogy | |
| 12:57:55 | gibi | yeah, if the metadata is deployed globally (top level) the local_metadata_per_cell needs to be false, but when deployed metadata to each cell then it need to be true | |
| 12:58:15 | sean-k-mooney | it tecnically optiona to set this to true for the per cell deployment by the way. it default to false | |
| 12:58:37 | sean-k-mooney | meanign if you deploy per cell metadata they can lookup metdata for other cells if they have access to the api db to do so | |
| 12:58:47 | gibi | sean-k-mooney: it depends, the the local metadata has cell db access then sure it can be set to false | |
| 12:58:57 | gibi | yeah that | |
| 12:59:28 | gibi | sean-k-mooney: cotyledon has the worker respawn implemented here https://github.com/sileht/cotyledon/blob/be444189de32a8c29c7107a9b02da44248a7e64a/cotyledon/_service_manager.py#L254-L256 | |
| 12:59:39 | sean-k-mooney | https://docs.openstack.org/nova/latest/configuration/config.html#api.local_metadata_per_cell is basically a configuration to prevnet that cross cell lookup | |
| 13:00:08 | gibi | yepp | |
| 13:14:09 | gibi | option b) aka re-initing the executor after fork works, based on the process name stored on the executor compared to the current process name. But this is ugly and nova only solution. Fortunately we will move all our spawns to executors so at least it applicable easily for now | |
| 13:38:46 | opendevreview | Merged openstack/osc-placement stable/2025.1: Update .gitreview for stable/2025.1 https://review.opendev.org/c/openstack/osc-placement/+/943757 | |
| 14:02:24 | dansmith | gibi: that move to spawn soon is surprising to me, as I suspect it will be hella slow, especially for openstack code | |
| 14:02:53 | dansmith | gibi: I'm not sure where we care about process workers, other than the api services in standalone mode.. in wsgi mode we shouldn't be forking at all, right? | |
| 14:02:54 | gibi | we don't spawn much, just at the start to get the worker processes (or when they die) | |
| 14:03:31 | gibi | dansmith: this is scheduler. The default behavior of oslo.service is to fork the workers | |
| 14:03:31 | dansmith | oh I guess conductor needs process workers too | |
| 14:03:46 | gibi | conductor too | |
| 14:04:00 | dansmith | do we need to do that for scheduler though? we did under eventlet for parallelism, but I'm not sure either do going forward | |
| 14:05:10 | gibi | threading in python 3.12 still limited by the GIL, so having a way to spawn processes as scaling make sense as that way we can saturate more CPU cores if needed | |
| 14:06:02 | dansmith | sure, but there are lots of multithreaded python programs providing reasonable performance with the GIL :) | |
| 14:06:04 | gibi | Im not 100% sure but I feel that oslo.service with worker=1 still forks a worker proc from the master proc | |
| 14:06:14 | dansmith | I suspect neither are cpu-intensive enough to really need full parallelism | |
| 14:06:46 | dansmith | okay | |
| 14:09:13 | gibi | but I think I understand you, we might not need to fork just use a single proc with big enough thread pools. I'm not sure this is something that supported by oslo.service out of the box. | |
| 14:09:32 | gibi | could be an improvement request | |
| 14:09:49 | gibi | to provide alternative way to avoid the fork | |
| 14:10:22 | dansmith | yeah, just might be worth consideration | |
| 14:11:06 | dansmith | conductor especially I suspect is mostly waiting on mysql and rabbit connections anyway.. scheduler *might* do enough work traversing lots of host objects or something, but I suspect not anymore with placement | |
| 14:14:31 | gibi | I think we had issues with the Numa filter where I needed to add cacheing to help with the execution time | |
| 14:14:34 | gibi | so meh | |
| 14:17:23 | opendevreview | Merged openstack/osc-placement stable/2025.1: Update TOX_CONSTRAINTS_FILE for stable/2025.1 https://review.opendev.org/c/openstack/osc-placement/+/943758 | |
| 14:17:59 | opendevreview | Balazs Gibizer proposed openstack/nova master: Translate scatter-gather to futurist https://review.opendev.org/c/openstack/nova/+/947966 | |
| 14:18:00 | opendevreview | Balazs Gibizer proposed openstack/nova master: Use futurist for _get_default_green_pool() https://review.opendev.org/c/openstack/nova/+/948072 | |
| 14:18:00 | opendevreview | Balazs Gibizer proposed openstack/nova master: Replace utils.spawn_n with spawn https://review.opendev.org/c/openstack/nova/+/948076 | |
| 14:18:01 | opendevreview | Balazs Gibizer proposed openstack/nova master: Add spawn_on https://review.opendev.org/c/openstack/nova/+/948079 | |
| 14:18:02 | opendevreview | Balazs Gibizer proposed openstack/nova master: Move ComputeManager to use spawn_on https://review.opendev.org/c/openstack/nova/+/948186 | |
| 14:18:02 | opendevreview | Balazs Gibizer proposed openstack/nova master: Move ConductorManager to use spawn_on https://review.opendev.org/c/openstack/nova/+/948187 | |
| 14:18:03 | opendevreview | Balazs Gibizer proposed openstack/nova master: Make nova.utils.pass_context private https://review.opendev.org/c/openstack/nova/+/948188 | |
| 14:18:03 | opendevreview | Balazs Gibizer proposed openstack/nova master: Rename DEFAULT_GREEN_POOL to DEFAULT_EXECUTOR https://review.opendev.org/c/openstack/nova/+/948086 | |
| 14:18:04 | opendevreview | Balazs Gibizer proposed openstack/nova master: Make the default executor configurable https://review.opendev.org/c/openstack/nova/+/948087 | |
| 14:18:05 | opendevreview | Balazs Gibizer proposed openstack/nova master: Print ThreadPool statistics https://review.opendev.org/c/openstack/nova/+/948340 | |
| 14:18:05 | opendevreview | Balazs Gibizer proposed openstack/nova master: WIP: allow service to start with threading https://review.opendev.org/c/openstack/nova/+/948311 | |
| 14:18:07 | opendevreview | Balazs Gibizer proposed openstack/nova master: DNM:Run nova-next with n-sch in threading mode https://review.opendev.org/c/openstack/nova/+/948450 | |
| 14:18:28 | opendevreview | Stephen Finucane proposed openstack/nova master: setup: Remove pbr's wsgi_scripts https://review.opendev.org/c/openstack/nova/+/902688 | |
| 14:18:46 | stephenfin | sean-k-mooney: gibi: Context for my question earlier ^ | |
| 14:19:56 | gibi | dansmith: sean-k-mooney: so I think I fixed the fork issue with re-initing the executor in the worker processes https://review.opendev.org/c/openstack/nova/+/947966/8/nova/utils.py#1276 not nice but seem to work. (I noticed the issue when tested slow / never finishing scatter gatter scenarios locally) | |
| 14:21:09 | dansmith | yeah that's not great | |
| 14:21:09 | gibi | stephenfin: thank I will take a loook | |
| 14:21:50 | dansmith | gibi: is there any way I could get you to stop putting many lines of comments in the middle of a condition like that? seems like a pattern that has been emerging and it completely breaks my brain when trying to reason about the logic like that :( | |
| 14:22:14 | gibi | feel free to leave a comment and I will move the comment | |
| 14:22:37 | gibi | I tried to be as close to the condition as possible but I can move it a bit further | |
| 14:28:26 | dansmith | gibi: I may have missed context from the above conversation about where this gets initialized so early, but is moving that an option? | |
| 14:28:49 | gibi | it is the service version check that happens before the fork | |
| 14:28:54 | gibi | and that uses scatter-gather | |
| 14:29:05 | gibi | (and also any init that happens at Service.create() today) | |
| 14:29:27 | gibi | we can move it but we have no way to signal to the master process that a worker want to kill the master if the version check fails | |
| 14:29:38 | gibi | I need to drop for a bit, I will be back for the nova meeting | |
| 14:29:46 | dansmith | ah, I see | |
| 14:30:16 | dansmith | seems like we could maybe just disable the pooling there, or just destroy the threadpool at the end of service.create before we return to allow the fork to be clean? | |
| 14:31:21 | gibi | disable pooling needs infra as the version check uses the generic scatter-gather. I would not duplicate that | |
| 14:31:42 | gibi | destroying the pool before the fork could work | |
| 14:31:51 | gibi | probably a bit cleaner than the re-init | |
| 14:31:54 | gibi | but | |
| 14:32:03 | gibi | I need to test it | |
| 14:32:46 | dansmith | for the disable pooling suggestion, I meant something like "use a temporary pool instead of the global one" - I know we need *a* pool there | |
| 14:33:34 | dansmith | you could also have a "sequential thread pool" that just runs each cell query synchronously in order instead of the threads maybe too | |
| 14:33:46 | dansmith | but yeah, just destroying after we're done with that check would be simple and clean, I suspect | |
| 14:34:11 | dansmith | I will miss the nova meeting today, btw. see you later | |
| 14:36:14 | sean-k-mooney | gibi: if notthing else it confirm that the issue is with how it forking and not reinitalisting it | |
| 14:36:48 | sean-k-mooney | gibi: dansmith i was on a metting but would markign the pool as thread local help | |
| 14:37:25 | sean-k-mooney | they would not be shared with the process correct. the only problem with that is if work on a thread pool wanted to add somethign to the same thread pool | |
| 14:37:56 | sean-k-mooney | my thinking is we should not have that patter in general | |
| 14:38:17 | sean-k-mooney | so if we mark the module level thread pool at threadlocal then on fork it will be initalsed to empty | |
| 14:38:39 | sean-k-mooney | and the first attempt to use it will init it in the child process without any shareing of state | |
| 14:39:24 | sean-k-mooney | having a way to disable scatter gahter for that first call on startup would also be an option | |
| 14:40:40 | sean-k-mooney | dansmith: by "sequential thread pool" you mean a sequtial executor right. like the one we used in tests | |
| 14:41:06 | dansmith | yes | |
| 14:41:55 | sean-k-mooney | ok futureist already provides that https://github.com/openstack/futurist/blob/master/futurist/_futures.py#L227 | |
| 14:42:14 | sean-k-mooney | so that would be an option for the early init | |
| 14:50:35 | sean-k-mooney | gibi: i comment with links to the thread local apporch but i have not tested that change it mich be worth trying with a DNM patch or locally | |
| 14:50:51 | sean-k-mooney | im just not sure if its the correct patthern to apply to this type of problem | |
| 14:51:26 | sean-k-mooney | i.e. can we convert all module global state to thread local storage if we have this problem or would that only work in this case | |
| 14:51:54 | sean-k-mooney | we dotn really want to have to have thread local db engine facades for example | |
| 14:54:31 | opendevreview | Stephen Finucane proposed openstack/placement master: setup: Remove pbr's wsgi_scripts https://review.opendev.org/c/openstack/placement/+/919582 | |
| 14:54:48 | stephenfin | sean-k-mooney: Think you can remove your -1 on that now? ^ | |
| 14:55:22 | sean-k-mooney | probaly we had that supprot in epoxy right | |
| 14:55:35 | sean-k-mooney | so we should be able to remove it now if we wanted too | |
| 14:58:08 | stephenfin | Yep | |
| 14:58:33 | sean-k-mooney | stephenfin: im still hesitent to remvoe this entrily for the simple reason that apache mod_wsgi does not suport using the module approch supproted by uwsgi/gunicorn | |