| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-19 | |||
| 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 | |
| 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 | stephenfin | it won't help you with your tests though since we don't type check our tests | |
| 12:08:36 | sean-k-mooney | it can be '' | |
| 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 | |
| 12:16:31 | stephenfin | gibi: This PCI in placement series is really well structured and pretty easy to review (especially for PCI code /o\). Nice work (y) | |
| 12:16:52 | stephenfin | oh, I read it as code | |
| 12:17:03 | stephenfin | I thought they were saying +1 means _someone_ will review it but not me | |
| 12:17:08 | stephenfin | *not necessarily me | |
| 12:17:10 | stephenfin | Oh well :) | |
| 12:17:35 | sean-k-mooney | so the idea was +1 can be set by anyone and they will review | |
| 12:17:51 | sean-k-mooney | an that is an indication that cores can use to perhaps also review it | |
| 12:18:07 | sean-k-mooney | and +2 is a commitmnet form the core reivew to review this | |