| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-02-23 | |||
| 14:33:12 | sean-k-mooney | bauzas: gibi just wanted to be a bit more explcit about the contained types then dict of anything | |
| 14:33:26 | sean-k-mooney | whcih he can do explitcly with the : syntax | |
| 14:33:26 | bauzas | that's one of the reason why I prefer to signal an nullable variable as None | |
| 14:33:46 | sean-k-mooney | well its not really nullable | |
| 14:33:51 | gibi | bauzas: yes, {} and None has a different meaning | |
| 14:34:00 | gibi | even if we tend to mix them | |
| 14:34:06 | sean-k-mooney | they are different types | |
| 14:34:19 | bauzas | sean-k-mooney: I know, I'm just saying that I prefer to have an object with a None value rather than an id and which is empty | |
| 14:34:47 | sean-k-mooney | there are pros and cons to both | |
| 14:36:05 | sean-k-mooney | using Empty collections is generally perfered using None for literals or classes is also genereally perfered | |
| 14:36:26 | sean-k-mooney | None tened to be used for singal values and empty collection fo correct type for aggreates | |
| 14:49:12 | sean-k-mooney | stephenfin: it will be a while before i push anything but how do you feel about hw:mem_locked=True|False as the name of the new extra spec required for vdpa | |
| 14:50:17 | sean-k-mooney | so vdpa guest woudl need hw:mem_locked=true and hw:mem_page_size=<anything> set | |
| 14:50:42 | sean-k-mooney | mem_page_size to account for no over commit and correct tracking | |
| 14:50:56 | jkulik | hi, we just upgraded to rocky. is the "sync aggregates" command supposed to be usable with ironic nodes? | |
| 14:50:58 | sean-k-mooney | and locked for the the dma limiations | |
| 14:52:49 | jkulik | because it raises an TooManyComputesForHost ... is there an official way to migrate those aggregates? | |
| 15:03:19 | stephenfin | gibi: Not a clue. That seems like an odd request | |
| 15:03:42 | gibi | stephenfin: I stick to dict (aka ty.Dict) | |
| 15:03:49 | gibi | it works | |
| 15:03:52 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Turn on mypy for nova/network/neutron.py https://review.opendev.org/c/openstack/nova/+/777146 | |
| 15:04:06 | gibi | stephenfin, bauzas: turned on mypy for nova/network/neutron.py | |
| 15:04:14 | bauzas | gibi: yeah saw it | |
| 15:04:24 | gibi | I will rebase that on top of the routed net patch | |
| 15:04:24 | bauzas | will look at that and thanks for working on it | |
| 15:05:56 | openstackgerrit | Lucas Alvares Gomes proposed openstack/nova master: [OVN] Explicitly set nova-next job to ML2/OVS https://review.opendev.org/c/openstack/nova/+/776944 | |
| 15:05:57 | openstackgerrit | Lucas Alvares Gomes proposed openstack/nova master: DO NOT REVIEW: Test OVN devstack module https://review.opendev.org/c/openstack/nova/+/776945 | |
| 15:07:10 | sean-k-mooney | i kind of think ^ is inverted | |
| 15:07:39 | sean-k-mooney | we shoudl proably explcity set nova-next to ovn and keep the rest on ml2/ovs for now and then consider swaping to ovn on more jobs later | |
| 15:16:06 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Turn on mypy for nova/network/neutron.py https://review.opendev.org/c/openstack/nova/+/777146 | |
| 15:20:15 | gibi | stephenfin: I've replied in https://review.opendev.org/c/openstack/nova/+/772381/6/nova/virt/libvirt/driver.py#263 | |
| 15:24:51 | stephenfin | ta | |
| 15:35:36 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Move the guest.get_disk test to test_guest https://review.opendev.org/c/openstack/nova/+/777151 | |
| 15:37:58 | lpetrut | Hi, about the Windows RBD patch: would you guys be ok with skipping the os-brick dependency until we sort out the lower constraints issue? https://review.opendev.org/c/openstack/nova/+/763550 | |
| 16:02:59 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Enable mypy on libvirt/guest.py https://review.opendev.org/c/openstack/nova/+/777155 | |
| 16:03:15 | gibi | stephenfin: this is an interesting one ^^ | |
| 16:04:03 | stephenfin | looking | |
| 16:06:15 | gibi | stephenfin: I did that as a followup for the detach bugfix series | |
| 16:06:53 | stephenfin | gibi++ Yup, realize that. Thanks :) | |
| 16:07:01 | stephenfin | So the unusual thing is the assertion, I assume? | |
| 16:08:54 | stephenfin | gibi: Is it just me, or are all those functions in the exception handlers than are accepting a 'guest' parameter broken? | |
| 16:09:11 | stephenfin | If so, I bet I was the one that broke it too. I recall reworking that stuff recently | |
| 16:09:27 | stephenfin | and attempts to bury the evidence ;) | |
| 16:10:27 | gibi | hm, you have a point, mypy only found the last usage of gues but the expcetion handler code also depends on the guest variable | |
| 16:11:33 | gibi | _cleanup_failed_start checks the guest before use it | |
| 16:11:47 | gibi | so the exception handlers are good | |
| 16:11:58 | stephenfin | the guest is useless though | |
| 16:12:08 | stephenfin | there's no way a function that raises an exception can return something | |
| 16:12:21 | stephenfin | so guest will either always be None or it will be set and an exception won't be raiseed | |
| 16:12:23 | stephenfin | *raised | |
| 16:12:46 | stephenfin | I suspect there was another caller after '_create_guest' previously | |
| 16:12:49 | stephenfin | power on or something | |
| 16:14:05 | gibi | hm, yeah, this can be cleaned up to only run the code that is currently in the finally block of _cleanup_failed_start | |
| 16:15:06 | stephenfin | yeah, https://github.com/openstack/nova/blob/stable/train/nova/virt/libvirt/driver.py#L6351-L6352 | |
| 16:15:55 | stephenfin | there was stuff done after '_create_guest' (formerly '_create_domain') previously | |
| 16:16:20 | stephenfin | so I suspect the real solution to that is to stop passing 'guest' to '_cleanup_failed_start' and remove any code that relied on it? | |
| 16:16:38 | stephenfin | which is what you just said, sorry :) | |
| 16:16:46 | gibi | I don't know why we try to stop the guest | |
| 16:16:57 | gibi | nvm I see it | |
| 16:17:09 | stephenfin | why or where? | |
| 16:17:10 | gibi | so in the past there was a case when the guest was fully started | |
| 16:17:16 | gibi | and then we failed | |
| 16:17:19 | gibi | so we need to stop is | |
| 16:17:20 | gibi | it | |
| 16:17:37 | gibi | but now it either starts without exception or fails before it is started | |
| 16:17:37 | stephenfin | if the firewall driver couldn't apply filters, right? | |
| 16:17:42 | gibi | yepp | |
| 16:17:49 | stephenfin | yeah, that's my reading of this too | |
| 16:18:04 | stephenfin | I did the firewall cleanup too but missed that. Apologies | |
| 16:18:53 | gibi | hm, I don't know what happens if create_guest fails at libvirt_secret.undefine() in the finally block, at that point we have a running guest I assume | |
| 16:19:20 | gibi | but we don't get back the guest object from the caller | |
| 16:19:25 | gibi | to stop it | |
| 16:19:59 | stephenfin | yeah, an exception will still be raised | |
| 16:20:44 | stephenfin | the finally block just executes stuff before the finally happens, iirc | |
| 16:20:52 | stephenfin | akin to save_and_reraise | |
| 16:22:58 | gibi | so we have to catch possible exceptions from libvirt_secret.undefine(), LOG them, and still return the geust object | |
| 16:23:07 | gibi | guest | |
| 16:23:13 | stephenfin | apparently not | |
| 16:23:13 | stephenfin | https://docs.python.org/3/reference/compound_stmts.html#finally | |
| 16:23:22 | stephenfin | If finally is present, it specifies a ‘cleanup’ handler. The try clause is executed, including any except and else clauses. If an exception occurs in any of the clauses and is not handled, the exception is temporarily saved. The finally clause is executed. If there is a saved exception it is re-raised at the end of the finally clause. If the finally clause raises another exception, the saved exception is set as the context of | |
| 16:23:22 | stephenfin | the new exception. If the finally clause executes a return, break or continue statement, the saved exception is discarded: | |
| 16:23:32 | stephenfin | Sorry | |
| 16:23:32 | stephenfin | If the finally clause raises another exception, the saved exception is set as the context of the new exception. | |
| 16:23:49 | stephenfin | so it'll basically do raise undefine_exception from other_exception | |
| 16:24:15 | gibi | yeah that is how I understand | |
| 16:24:48 | stephenfin | yeah http://paste.openstack.org/show/802935/ | |
| 16:25:49 | gibi | if we let the exception propagate from the finally block then we will leave a running guest behind | |
| 16:25:59 | gibi | and the caller cannot stop it | |
| 16:26:08 | gibi | as it has no reference to that guest object | |
| 16:26:26 | stephenfin | Hmm, good point | |
| 16:26:41 | stephenfin | so if the undefine call fails | |
| 16:27:24 | stephenfin | we won't have a running guest before that since launching it is the last thing we do (returning won't cause an exception) | |
| 16:28:47 | gibi | you are right sorry | |
| 16:29:10 | gibi | so no need to change here | |
| 16:33:49 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Follow up type hints for a634103 https://review.opendev.org/c/openstack/nova/+/777159 | |
| 16:44:29 | bauzas | woah, the gate pipeline is late | |
| 16:54:24 | gibi | maybe it is already the churn before the feature freeze | |
| 16:55:11 | stephenfin | bauzas: There are issues. They were discussing it on #openstack-infra earlier | |
| 16:55:19 | stephenfin | Still are, maybe | |
| 16:55:23 | bauzas | kk | |