| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-03-14 | |||
| 13:57:09 | dansmith | I see it used in consume_requests() but the only other place is update_device() which isn't called from here at all | |
| 13:57:40 | dansmith | anyway, it doesn't matter that much because I see the problem now, it just seems confusing to me | |
| 13:58:13 | dansmith | however, at least it's consistent with all the other confusing terminology in here (devs, pools, dev_pools, stats, all of which are kinda the same :) | |
| 13:59:06 | gibi | _allocate_devs pops the device from the pool | |
| 13:59:17 | dansmith | we also call add_device() from _free_device() in compute manager while we're deallocating | |
| 13:59:18 | gibi | during consume_request | |
| 13:59:41 | dansmith | oh, it doesn't call remove_device() but it removes it some other way? | |
| 13:59:46 | gibi | yeah | |
| 13:59:48 | gibi | :/ | |
| 14:00:14 | sean-k-mooney | the way we test in teh schduler if a host passes is we call consume_requests on a copy of the pools and just dont save it from Supprots_requests() | |
| 14:00:21 | gibi | literarily pci_dev = pool['devices'].pop() | |
| 14:00:27 | dansmith | ack | |
| 14:00:48 | dansmith | okay so the add_device is adding it back in after it's no longer used I guess instead of just being marked as in-use... | |
| 14:00:49 | gibi | sean-k-mooney: nope, we call _apply, consume is for the pci tracker | |
| 14:01:04 | gibi | dansmith: yes that is my understanding | |
| 14:01:32 | sean-k-mooney | i may have mixed up the names but the point i wanted to make is, we test if we can allcoate the device by trying to do it on a copy of the data) | |
| 14:01:52 | gibi | sort of yes | |
| 14:03:03 | gibi | even in the scheduler (host manager) we do an allocation on the stats via apply to allow mutlicreate to see a proper resource availability | |
| 14:03:47 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/pci/stats.py#L779-L821 | |
| 14:04:01 | sean-k-mooney | yep | |
| 14:05:48 | gibi | bah we have 3 entry point not just two: | |
| 14:06:17 | gibi | 1. filters do supports_requests https://github.com/openstack/nova/blob/master/nova/scheduler/filters/pci_passthrough_filter.py#L62 | |
| 14:06:29 | gibi | 2. host manager do apply_request https://github.com/openstack/nova/blob/master/nova/scheduler/host_manager.py#L313 | |
| 14:06:47 | gibi | 3. pci_tracker do cosume_request https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L311 | |
| 14:07:27 | gibi | but you are right 1 is basically 2 on a copy of the pools | |
| 14:09:27 | sean-k-mooney | that not all of them | |
| 14:09:35 | sean-k-mooney | it kind of is but | |
| 14:09:42 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/scheduler/filters/numa_topology_filter.py#L105-L112 | |
| 14:10:08 | sean-k-mooney | numa_fit_instance_to_host also takes into acocunt the pci requests | |
| 14:10:38 | gibi | yeah that is 1 with extras, it calls support_request and look into the pools as well | |
| 14:11:40 | sean-k-mooney | ya so supprots_request is the entry point from the filters effectivly | |
| 14:11:44 | gibi | this is not for the faint-hearthed | |
| 14:12:40 | dansmith | yup | |
| 14:14:11 | sean-k-mooney | i could have sworn that we had factor the common logic out by the way | |
| 14:14:27 | sean-k-mooney | i.e. that supprot called consume or that support and consume both called apply | |
| 14:14:41 | sean-k-mooney | i dont recall why that does nto seam to be the case | |
| 14:15:18 | gibi | it needs some divergence as consume needs to return a specific device while apply only decrase pool count | |
| 14:15:30 | sean-k-mooney | oh right | |
| 14:15:37 | sean-k-mooney | consume is what we run on the compute | |
| 14:15:58 | gibi | yepp | |
| 14:16:01 | sean-k-mooney | its actully operating not just on the ppoos but the pci_device records and selecting the device | |
| 14:16:52 | dansmith | gibi: another thing that seems messy is that we take a thing, which is a dict'ing of an object, and pop out things that we ignore, and then see if "everything else is the same" right? | |
| 14:17:32 | dansmith | seems like we should opt-in to the things that distinguish them so that new things that get added in later don't cause fracturing of the pools if you don't also update the exclusion list | |
| 14:17:36 | dansmith | (or just do this differently) | |
| 14:18:26 | sean-k-mooney | that would be more maintianable wehn this was first writtin jay was pretty again adding new tag in the future | |
| 14:18:33 | sean-k-mooney | since then we have added several | |
| 14:18:47 | sean-k-mooney | btu never really revisited that orgianl approch | |
| 14:19:48 | gibi | dansmith: I think the concept is that a pool is a set of devs with a set of common field values. So it is the defintion of the interesting fields is the question here | |
| 14:20:10 | sean-k-mooney | gibi: yep | |
| 14:20:26 | gibi | and yes I share you feeling that ignoring flags is less maintainable than listing interesting flags | |
| 14:20:54 | dansmith | right | |
| 14:21:02 | gibi | I'm not motivated to change this logic in a bugfix though. I can queue up a refactor after it. | |
| 14:21:04 | sean-k-mooney | and this is what i mean when i say tis placement like a pool is effectivly an rp with and inventory fo common "fungable" dvices | |
| 14:22:06 | gibi | I would even wrap these dev and pool dicts in proper objects in memory (not ovos) for more readability (and mypy) | |
| 14:23:20 | gibi | I'm bit affraid of what can of worms this opens up | |
| 14:24:12 | gibi | and I have the ovo version bump preemptive unit test and refactor series to get back to as well | |
| 14:24:24 | gibi | I need more time for actually writing code | |
| 14:24:27 | dansmith | gibi: no I'm not suggesting you fix it in this bugfix or even later, I'm just lamenting the design | |
| 14:24:35 | gibi | dansmith: OK | |
| 14:25:10 | dansmith | _especially_ if it means more mypy in this code :) | |
| 14:26:03 | gibi | sh*t I have this from the PCI in Placement work remaining as well https://review.opendev.org/q/topic:%22bp/pci-device-spec-cleanup%22 (danger, it contain mypy changes as well) | |
| 14:26:39 | dansmith | going on three years old there... | |
| 14:26:52 | gibi | yeah it is sad | |
| 14:27:21 | sean-k-mooney | ya... | |
| 14:27:46 | sean-k-mooney | this https://review.opendev.org/c/openstack/nova/+/844491 i think was one fo the more valuable changes | |
| 14:28:28 | sean-k-mooney | although the seise of a whole impoved alot of things | |
| 14:30:48 | gibi | maybe in Flamingo :) | |
| 14:31:02 | gibi | less golang more python :) | |
| 14:33:39 | sean-k-mooney | :) | |
| 14:33:46 | gibi | I don't know what is the relationship of flamingos a pythons, who would eat who | |
| 14:34:24 | dansmith | pythons would eat flamingos no doubt | |
| 14:35:31 | gibi | cool then. (we have neither this side of the globe) | |
| 14:36:01 | gibi | and gopher (the golang mascot) would definitely loose againts python | |
| 16:03:02 | opendevreview | Balazs Gibizer proposed openstack/nova master: Reproduce bug/2098496 https://review.opendev.org/c/openstack/nova/+/941673 | |
| 16:03:03 | opendevreview | Balazs Gibizer proposed openstack/nova master: Ignore metadata tags in pci/stats _find_pool logic https://review.opendev.org/c/openstack/nova/+/944277 | |
| 16:09:56 | opendevreview | Balazs Gibizer proposed openstack/nova master: Ignore metadata tags in pci/stats _find_pool logic https://review.opendev.org/c/openstack/nova/+/944277 | |
| 17:12:54 | sean-k-mooney | werid i just got a destop notificaiton form a comment gibi made a few hours ago | |
| 17:13:28 | sean-k-mooney | dansmith: not that im disagreeign but dont we test migration with postgres in the tox jobs | |
| 17:13:41 | sean-k-mooney | i would not try and use it in production | |
| 17:14:01 | sean-k-mooney | but we do install it with bindep in the tox env | |
| 17:14:25 | sean-k-mooney | and the opertunistic database stuff should kick in but maybe that broke at some point | |
| 17:14:51 | sean-k-mooney | we defintlly dont have stong testin gthere as we only really apply the migration ot an empty db | |
| 17:18:37 | sean-k-mooney | ya nova.tests.unit.db.main.test_migrations.TestMigrationsWalkPostgreSQL.test_walk_versions is still running in tox | |
| 17:20:28 | dansmith | (back in a sec) | |
| 17:21:05 | sean-k-mooney | dansmith: its not urgent dont worry. teh prostgress test are still runing but obviously those migration tests dont test that much | |
| 17:21:35 | sean-k-mooney | pointing to https://governance.openstack.org/tc/resolutions/20170613-postgresql-status.html is still the best responce | |
| 17:23:10 | sean-k-mooney | i was just concerned that those opertuneistic tests broke at some point and we missed it but the pg and mysqlone are running in addtion to sqlite | |
| 17:26:37 | dansmith | sean-k-mooney: we do, but not full of real world data | |
| 17:27:05 | dansmith | so maybe I was less than accurate about 'we don't test it' but testing with an empty DB is not really testing IMHO | |
| 17:27:14 | dansmith | it makes sure all the schema SQL is right, but not much else | |
| 17:27:40 | sean-k-mooney | ya so i know we incorrectly set the coration type on at least one of the tables when movign to alembic | |
| 17:27:56 | sean-k-mooney | so that might cuase some issues only when there is data | |
| 17:28:07 | sean-k-mooney | or on some locales but not our default | |
| 17:28:40 | sean-k-mooney | i think neutron had a postgress job up until about a year ago | |
| 17:30:53 | dansmith | IMHO people that want to run postgres are more than welcome to test and fix the things | |
| 17:31:27 | sean-k-mooney | ya i dont think its unreasonable | |
| 17:32:05 | sean-k-mooney | i just know that it broke for them at some poitn and no one stepped up to fix it so they droped testing | |
| 17:32:47 | sean-k-mooney | oh wow | |
| 17:32:58 | sean-k-mooney | they actully dropped postgress code https://github.com/openstack/neutron/commit/16e4f0af89cc67b7a8901bfd357fd49b86de73c8 | |
| 17:33:28 | sean-k-mooney | i pointed out to stephen that we techinially still supprot it | |