Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-19
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
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

Earlier   Later