Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-19
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
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
12:05:26 sean-k-mooney those asserts didnt actully catch any issue in teh end but validated that that was not the issue
12:05:55 sean-k-mooney it wasnt the path that was wrong it was the key that was used for the path in the end
12:06:36 stephenfin Yeah, it's a band-aid for the lack of consistent type hinting in the code base, unfortunately
12:06:45 stephenfin but to answer your question, can we drop it so?
12:06:59 sean-k-mooney stephenfin: the fact that the dev_path is not a keyword arg is enough documentaiton for me that we require this arg
12:07:12 sean-k-mooney so ya ill drop them
12:07:30 sean-k-mooney can typing assert that something is not None by the way
12:07:51 sean-k-mooney or not None or '' in this case
12:07:59 sean-k-mooney i dont think so but it would be nice if it could
12:08:08 stephenfin def foo(bar: str) -> None:
12:08:22 stephenfin bar has to be a string. It can't be a bool, int, None or anything esle
12:08:24 stephenfin *else
12:08:36 sean-k-mooney it can be ''
12:08:36 stephenfin it won't help you with your tests though since we don't type check our tests
12:08:44 stephenfin yes, it could
12:09:03 sean-k-mooney ok ill drop them when i refactor
12:09:12 stephenfin ta
12:09:15 sean-k-mooney mind if i keep the ablity to disable optimiasation
12:09:21 sean-k-mooney ill add it to passargs
12:09:22 sean-k-mooney instead
12:09:27 opendevreview Arnaud Morin proposed openstack/nova master: Unbind port when offloading a shelved instance https://review.opendev.org/c/openstack/nova/+/853682
12:09:36 sean-k-mooney then i can add asset when debuging and just turn it off on the command line
12:10:08 sean-k-mooney *passenv
12:10:24 stephenfin wfm
12:10:35 sean-k-mooney cool thanks for looking
12:15:09 opendevreview Arnaud Morin proposed openstack/nova master: Unbind port when offloading a shelved instance https://review.opendev.org/c/openstack/nova/+/853682
12:15:10 stephenfin sean-k-mooney: regarding your earlier comments on +1 vs +2 review-priority, have you hovered over the +1 and +2 review-priority buttons?
12:15:41 sean-k-mooney ya
12:15:47 sean-k-mooney i know what the text says
12:16:28 sean-k-mooney the contibutor vs core promise thing is a bit weired to me

Earlier   Later