Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-19
09:26:28 sean-k-mooney im also not alwasy setting it, im just setting it on patches that are either imporant or have not reablly been reviewed in a while to highlight them to others
09:27:30 sean-k-mooney for example i dont think this backport is critical https://review.opendev.org/c/openstack/nova/+/821349 but it would be nice to land sooner rather then later so RP +1
09:29:05 sean-k-mooney stephenfin: by the way im currently working on the final patch in the vdpa seriese would you have time to review the first 3. i just need to add a release note and update the docs in the last patch and its also done
09:43:31 stephenfin sean-k-mooney: sure (y)
09:44:21 sean-k-mooney stephenfin: just pushing the last patch now once i fix the commti message so it should be ready by the time you get to it
09:46:26 opendevreview sean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate https://review.opendev.org/c/openstack/nova/+/853704
09:47:21 sean-k-mooney gibi: ^ that should be ready for your review too. if ye have comments ill respin them collectinvly once ye have complete reviewing the whole sereise later today
09:49:52 gibi sean-k-mooney: ack, I will do a review round on it today
09:50:10 sean-k-mooney oh i have to fix 1 thing in the last patch for the compute service bump so ill respin that quickly
09:50:34 gibi (today I spent hours in a rabbit hole spreading provider mapping around the claim code, but now I'm backing out of it as it is a dead end)
09:54:11 opendevreview sean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate https://review.opendev.org/c/openstack/nova/+/853704
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 gibi yes
09:59:31 sean-k-mooney yep
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 gibi so eventually we can refactor the QoS path to use that too
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: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 sean-k-mooney or a pattern we shoudl really follow
12:00:49 stephenfin What's the difference?
12:00:59 sean-k-mooney that will cause the agent to restart potially
12:01:18 stephenfin so will an assert

Earlier   Later