Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-03-14
13:47:26 gibi dansmith: yes, find_pool tries to find a matching pool, the callers of _find_pool creating a logic that if a pool is not found for a dev then a new pool is created for the dev
13:48:02 gibi but as matching logic was broken in _find_pool the upper layer created new pool for the devs
13:48:05 gibi during deallocation
13:48:21 dansmith ohhh, I see okay, let me follow that thread
13:49:00 dansmith since you're changing it and the allocation part I was mostly linking those in my head (and because _find_pool is almost like _filter_pools)
13:50:13 dansmith so if I comment out the _find_pool() part, I fail this part of the test:
13:50:22 gibi yeah the two changed code path are only linked from afar. the _find_pool runs during new device discovery and device deallocatin (putting back to pool). The _apply runs during scheduling of a VM, the consume codepath runs during the pci claim from the pci tracker in the compute
13:50:24 dansmith # assert that the single pool is not broken into two during the de-allocation
13:50:33 dansmith which makes sense based on what you said
13:50:34 gibi yeas
13:51:28 gibi and the _find_pool fix is the only thing that is necessary, the other code change is just safetynet
13:54:43 dansmith gibi: okay and the place where we do that (add a pool if we don't find it) is in add_device()..
13:54:55 dansmith it doesn't look like we run remove_device() when we consume it from the pool or anything,
13:55:19 dansmith so I'm not sure why we're re-running add_device() there, except maybe just as idempotentcy/healing or something?
13:56:46 gibi I think add_device is used both during new device discover at startup and during deallocation when we place the device back to the pool
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

Earlier   Later