| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-19 | |||
| 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 | |
| 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 | |
| 12:18:41 | gibi | I think +1 is not well defined for cores | |
| 12:18:57 | gibi | so sean-k-mooney you are free to use to for a "maybe" | |
| 12:19:16 | sean-k-mooney | sylvain wanted to use +1 as a way for non cores to singal to cores that something might be ready for review too | |
| 12:19:20 | sean-k-mooney | the text in my orgially patch was +1 is core review requested and +2 is core review approved but that was also problematic | |
| 12:19:44 | gibi | I think +1 for non-cores is the same as +2 for cores | |
| 12:19:50 | gibi | both is a promise | |
| 12:19:54 | sean-k-mooney | yep | |
| 12:20:03 | gibi | that I, who set it, will review the patch | |
| 12:20:14 | sean-k-mooney | im using +1 as im going to review this but not nessiarly ping other to review it | |
| 12:20:35 | sean-k-mooney | vs +2 ill review it and when im going to give my +2 ill ping others to review it too | |
| 12:20:59 | gibi | that is OK to me | |
| 12:21:03 | sean-k-mooney | i.e. i not only commit to reviewing but i also care about this not waiting for every | |
| 12:21:33 | sean-k-mooney | kindo fo like feature-liason lite | |