| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-19 | |||
| 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 | |
| 12:21:38 | gibi | stephenfin: I needed the small step in the PCI work for myself too to see what is missing :) The inventory part is self contained mostly in the new translator. The scheduling part will be less easy to read (once I write it :D) | |
| 12:21:53 | gibi | sean-k-mooney: that make sense | |
| 12:24:17 | stephenfin | gibi: I got as far as https://review.opendev.org/c/openstack/nova/+/851358 +2 on everything I think | |
| 12:24:53 | gibi | stephenfin: thank you, have a good one | |
| 12:27:39 | sean-k-mooney | my plan for there rest of the day is finish the vdpa seriese, review the pci serise and if melwitt has updated the encyption series review that. my plan for next week assuming vdpa is done is 100% upstream review so please ping as needed | |
| 12:29:20 | gibi | sean-k-mooney: ack. I will do another vdpa round today if needed | |
| 12:30:31 | gibi | stephenfin: thanks for noticing the TODO in https://review.opendev.org/c/openstack/nova/+/851358 I forgot it. Actually the patches above that are also ready until https://review.opendev.org/c/openstack/nova/+/850468 | |
| 12:31:05 | opendevreview | Amit Uniyal proposed openstack/nova stable/wallaby: add regression test case for bug 1978983 https://review.opendev.org/c/openstack/nova/+/853811 | |
| 12:31:06 | gibi | but it is more than fair to stop there now | |
| 12:31:06 | opendevreview | Amit Uniyal proposed openstack/nova stable/wallaby: For evacuation, ignore if task_state is not None https://review.opendev.org/c/openstack/nova/+/853812 | |
| 12:32:58 | stephenfin | ack, I'll keep going with it this afternoon so. If you could cobble together a follow-up I can finish that | |
| 12:38:58 | gibi | stephenfin: I will hold off with the follow up until sean-k-mooney reviews it | |
| 12:42:10 | opendevreview | Amit Uniyal proposed openstack/nova master: Adds check for VM snapshot fail while quiesce https://review.opendev.org/c/openstack/nova/+/852171 | |
| 13:38:37 | opendevreview | Merged openstack/nova master: Add reno for fixing bug 1941005 https://review.opendev.org/c/openstack/nova/+/853265 | |
| 14:54:50 | JayF | Good morning; thanks for the reviews I've already been getting. I do have a couple of other PRs open in Gerrit I'd appreciate reviews on that are ready to go: https://review.opendev.org/c/openstack/nova/+/853529 and https://review.opendev.org/c/openstack/nova/+/853540 - thanks in advance. | |
| 14:58:12 | opendevreview | Merged openstack/nova stable/wallaby: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/821349 | |
| 14:59:21 | opendevreview | Jay Faulkner proposed openstack/nova stable/ussuri: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/821351 | |
| 14:59:58 | JayF | I need to re-learn my alphabet, apparently. Rebased the wrong one lol | |
| 15:00:03 | opendevreview | Jay Faulkner proposed openstack/nova stable/victoria: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/821350 | |
| 15:34:33 | opendevreview | Merged openstack/nova master: block_device: Add DriverImageBlockDevice to block_device_info https://review.opendev.org/c/openstack/nova/+/826527 | |
| 15:40:23 | gibi | 2022-08-19 17:35:22,086 DEBUG [nova.pci.stats] Dropped 1 device(s) as they are on the wrong NUMA node(s) | |
| 15:40:26 | gibi | 2022-08-19 17:35:22,086 DEBUG [nova.pci.stats] Dropped 1 device(s) that are not part of the placement allocation | |
| 15:40:29 | gibi | 2022-08-19 17:35:22,086 DEBUG [nova.pci.stats] Not enough PCI devices left to satisfy request | |
| 15:40:47 | gibi | ... and the NUMATopologyFilter now works with placement allocation candidates \o/ | |
| 15:40:53 | sean-k-mooney | nice | |
| 15:41:36 | gibi | there are some raw edges but the general idea seems to work | |
| 15:42:07 | gibi | now I have like 10 WIP commits locally to clean up :D | |
| 15:42:10 | sean-k-mooney | ack im sure we can flesh that out via review and or cleanups | |
| 16:23:38 | opendevreview | Merged openstack/nova master: block_device: Add encryption attributes to image and ephemeral disks https://review.opendev.org/c/openstack/nova/+/826528 | |
| 17:50:00 | opendevreview | melanie witt proposed openstack/nova master: Follow up changes for ephemeral encryption https://review.opendev.org/c/openstack/nova/+/853254 | |
| 18:23:59 | opendevreview | Balazs Gibizer proposed openstack/nova master: DNM: why I cannot set request_id on InstancePCIRequiest https://review.opendev.org/c/openstack/nova/+/853835 | |
| 18:24:26 | gibi | sean-k-mooney: if you are still around ^^ I totally don't get this | |
| 18:30:54 | sean-k-mooney | i dont think you want to set request_id you want to set requester_id | |
| 18:31:31 | sean-k-mooney | ill take a look quickly but we set this in teh neutorn module somewhere i think | |
| 18:33:12 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/objects/instance_pci_requests.py#L43-L44 | |
| 18:33:22 | sean-k-mooney | but actully it depens on what you want to track ther | |
| 18:33:47 | sean-k-mooney | are you tryign to add the placment request group or resouce provider there | |
| 18:33:55 | gibi | I need a unique id | |
| 18:34:07 | sean-k-mooney | well the request_id should be unique | |
| 18:34:22 | gibi | for neutron based InstancePCIRequests we generate a uuid for request_id | |
| 18:34:31 | gibi | I try to do the same for the flavor based requests | |
| 18:34:41 | sean-k-mooney | yes and we set requester_id to the neutorn port uuid | |
| 18:34:48 | sean-k-mooney | ah ok | |
| 18:35:04 | sean-k-mooney | am that should be posible | |