Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-19
09:54:42 sean-k-mooney i think you should just need to pass them to filter_pools
09:55:07 sean-k-mooney i.e. if you have the provider uuid in the pool filter pools can jsut filter to the ones it the alloctiaons
09:55:50 sean-k-mooney although that is proably over simplfying
09:56:36 sean-k-mooney i woudl be tempted to extend the pci request object to carry the RP infor that it shoudl be fullfiled form
09:56:50 sean-k-mooney and then use that latter when we are doing the filtering /caliming
09:57:07 sean-k-mooney gibi: would something like ^ work better
09:57:29 sean-k-mooney that woudl avoid chaning the sigurtures of any of the fuctions
09:57:38 gibi yeah I'm on this track
09:57:49 gibi the filter_pools needs it
09:58:01 gibi and we call that from 3 different places
09:58:08 sean-k-mooney but you would have to update teh request objecject before calling support/consume/apply
09:58:20 gibi 1) during scheduling (there we hace an allocation candidate to work with)
09:58:35 gibi 2) during claim (there we have the request spec to work with probably)
09:58:46 gibi 3) during consume (that is a big rabbit hole :D)
09:58:51 gibi but you are right
09:59:02 gibi the InstancePCIRequest could carry the PR uuid
09:59:15 gibi _after_ the scheduler made the allocation in placement
09:59:20 gibi as that is then fixed
09:59:24 sean-k-mooney well if save the updated request object after 1 when we claim the allcoation candiate then 2 and 3 can just read it from there
09:59:31 sean-k-mooney yep
09:59:31 gibi yes
09:59:59 sean-k-mooney so i think that will work out cleanly in the end
10:00:02 gibi we did something similar for QoS with the parent_ifname tag in the InstancePCIRequest
10:00:24 sean-k-mooney ah yes that is indeed similar
10:00:44 sean-k-mooney as that narrowed the set of pools to consider
10:00:45 gibi the RP uuid is actually cleaner
10:01:01 opendevreview Merged openstack/nova stable/xena: add regression test case for bug 1978983 https://review.opendev.org/c/openstack/nova/+/853218
10:01:01 gibi so eventually we can refactor the QoS path to use that too
10:01:17 sean-k-mooney yep
10:01:40 sean-k-mooney once we start populating the rp uuid in the pci device tabel and pci_stat pools
10:02:10 sean-k-mooney for the pci devices are you going to add a new column for the rp_uuid
10:02:18 sean-k-mooney or just put it in the extra info column
10:02:26 sean-k-mooney i woudl be tempted to do the former
10:02:50 sean-k-mooney for the pools i would just put it in the json blob
10:03:00 gibi at the moment I don't see where I need the rp_uuid from the pci device, when I will see that I can consider this
10:03:12 gibi for the pools it is probably the blob
10:03:30 sean-k-mooney ya so i dont think we will evern need to do an sql query on the pools
10:03:38 sean-k-mooney that will alway be processed in python
10:03:53 gibi I thinks so too
10:03:56 sean-k-mooney but the pci devices it would be nice to have them correalated with the placment rp
10:04:34 sean-k-mooney so it would be nice to put it at least in extra_info blob
10:04:47 sean-k-mooney but you possibely coudl move the fitlering to sql
10:05:14 sean-k-mooney i.e. have filter pools intially get the candiate devcie with an sql query
10:05:34 sean-k-mooney alhtogh we have the pools in memory so i dont think that actully bys you much
10:05:47 sean-k-mooney so really i would jsut like it in the pci device tabel fro debugging
10:05:59 sean-k-mooney so extra info would be fine for that
10:10:18 gibi yeah without the rp_uuid there you have to look up the pci address from the dev and query placement by RP name
10:10:57 gibi I will see how this fits together but it probably make sense to add rp_uuid to the dev
10:12:09 sean-k-mooney ya this is a minor convince feature so we can see where it fits when you get to it
10:12:58 sean-k-mooney we lived without trackign the neutron port uuid in the requster_id for years
10:13:37 sean-k-mooney but due to recent evnets the more info we have for debuging things like this the happier i will be
10:14:06 opendevreview Arnaud Morin proposed openstack/nova master: Unbind port when offloading a shelved instance https://review.opendev.org/c/openstack/nova/+/853682
10:16:51 amorin sean-k-mooney ^ I wrote both unit + functional tests
10:17:01 sean-k-mooney thanks ill review it shortly
10:17:04 amorin I also patch one of your tests to make it work
10:17:16 amorin no hurries, thanks!
10:18:16 sean-k-mooney ah yes test_shelve for remote managed ports
10:52:02 opendevreview Merged openstack/nova stable/xena: For evacuation, ignore if task_state is not None https://review.opendev.org/c/openstack/nova/+/853219
10:56:55 gibi sean-k-mooney: I did a review round on the vdpa series, the last patch needs some func test coverage for live migration but other than that I'm happy
11:02:38 tobias-urdin from what I understand nova's policy code restricts POST /os-external-server-events requests to only be allowed from the admin endpoint, how is that enforced? referring to issue mentioned in https://bugzilla.redhat.com/show_bug.cgi?id=1640443
11:02:56 tobias-urdin i'm trying to understand if that change is still required and valid or if that fix can be reverted
11:03:03 tobias-urdin to using, for example, the internal endpoint instead
11:04:39 sean-k-mooney gibi: or right good point ill add that
11:05:31 sean-k-mooney tobias-urdin: the external events api is not only admin only but intended to be called only by other services
11:05:39 sean-k-mooney tobias-urdin: as in admin shoudl never call it directly
11:06:17 sean-k-mooney using the internal endpoint is fine you the calling service jsut need to use the admin clint
11:06:24 sean-k-mooney but old cinder did not do that properly
11:07:43 sean-k-mooney so if cinder have actully fixed there code to use the admin client to call the external events api instead of trying to use the user token then it can be revierted but the change in ooo was jsut a workaround for a bug in cinder
11:08:24 sean-k-mooney nova has never required /os-external-server-events to use the admin endpoint
11:08:31 tobias-urdin ah, the logic was enforced by cinder I see! that's why I couldn't figure it out, my bad
11:08:35 tobias-urdin thx
11:08:49 sean-k-mooney well not enforced they just had a bug
11:09:06 tobias-urdin ack
11:09:09 sean-k-mooney they tried to use teh users token that requsted the volume extend to send the external event
11:09:21 sean-k-mooney instead of using an admin token
11:09:44 sean-k-mooney by using the admin endpoint they bypassed the admin check
11:23:31 sean-k-mooney gibi: based on your questions regarding suspend and the compute service bump i realise i need a compute service bump for attach/detach. suspend can use the same one as migrate but attach/detach needs one too. so ill adress all your nit as part of that too
11:40:21 stephenfin sean-k-mooney: done
11:48:37 sean-k-mooney stephenfin: thanks ill respine them shortly
11:55:54 gibi sean-k-mooney: ack, good point
11:59:37 sean-k-mooney stephenfin: i really would prefer to use the asserts by the way
12:00:02 stephenfin You shouldn't though. You've already highlighted the risks of doing so
12:00:04 sean-k-mooney i dont want to check this at runtime but do want to prevent you breaking the tests with bad mocks/fixtures
12:00:26 stephenfin Then just raise a NovaException and don't handle it
12:00:42 sean-k-mooney but that is not the behaivor i want
12:00:49 stephenfin What's the difference?
12:00:49 sean-k-mooney or a pattern we shoudl really follow
12:00:59 sean-k-mooney that will cause the agent to restart potially
12:01:18 stephenfin so will an assert
12:01:41 sean-k-mooney well it wont because i know this is always set in real code
12:01:53 stephenfin then the exception won't do anything either
12:02:14 sean-k-mooney but what im trying to protect agaisnt is you create a unit test where you populate the object manually and fail to set the requried value
12:02:29 sean-k-mooney we already have assert in real code in nova
12:02:36 sean-k-mooney we dotn have many but they exists
12:03:17 stephenfin we do, but they really shouldn't be there and we shouldn't be adding to them
12:03:29 stephenfin I don't get what the issue with 'raise Exception(...)' is
12:03:43 stephenfin if we'll never see the AssertionError in real code then we'll never see the Exception
12:04:01 sean-k-mooney right but we pay the cost of checking
12:04:16 sean-k-mooney i can drop the check if you prefer but that is why assert exist
12:04:36 sean-k-mooney to help you debug and not pay any runtime cost in production code

Earlier   Later