| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-02-23 | |||
| 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 | |
| 16:55:27 | bauzas | thanks | |
| 17:03:44 | prometheanfire | we're coming up on the end of the release cycle and nova still hasn't fixed mock :| | |
| 17:05:21 | sean-k-mooney | prometheanfire: its not really broken is it. | |
| 17:05:43 | stephenfin | prometheanfire: I thought we'd said that we didn't need to? | |
| 17:06:11 | stephenfin | mock (the third party lib) has fixes that weren't backported to python3.6 | |
| 17:06:43 | stephenfin | and I'm pretty sure the reason mock was capped at <4.0 (iirc) was because that had its own bug | |
| 17:07:26 | stephenfin | prometheanfire: Can you remind me of the reason nova had to switch? I'd stopped focusing on it because working around python3.6 unittest.mock bugs seemed wrong | |
| 17:07:56 | sean-k-mooney | wel should eventulaly swicht to not useing mock the lib at all | |
| 17:08:02 | sean-k-mooney | and use the standard lib only | |
| 17:08:03 | stephenfin | sean-k-mooney: why | |
| 17:08:14 | sean-k-mooney | because mock the lib has some bugs | |
| 17:08:23 | sean-k-mooney | it does not fully work the same as the stanard lib | |
| 17:08:35 | stephenfin | See above | |
| 17:08:41 | prometheanfire | requirements doesn't cap mock | |
| 17:08:48 | sean-k-mooney | and since we are pythoh 3 only now we dont need it any more | |
| 17:08:55 | stephenfin | To the best of my knowledge, it's a carbon copy of unittest.mock from the latest stable python | |
| 17:09:10 | stephenfin | including all the new features and bugfixes found therein | |
| 17:09:18 | prometheanfire | if the mock-4 releases are bad they should be masked so they don't bug me every day when the constraints bot review fails because nova doesn't work with the new mock (but other projects aparently do) | |
| 17:10:03 | stephenfin | prometheanfire: I'm almost certain we're seeing the exact same issues with unittest.mock on either python3.9 or python3.10 | |
| 17:10:07 | sean-k-mooney | stephenfin: there was deffinlty differnt behavor in the past as code in neutorn would not work in nova | |
| 17:10:32 | stephenfin | and there's an open bug against it but it hasn't gone anywhere | |
| 17:11:02 | stephenfin | prometheanfire: Have you link to a failing constraints bot job? | |
| 17:11:05 | stephenfin | *a link | |
| 17:11:07 | prometheanfire | yep | |
| 17:11:17 | prometheanfire | https://review.opendev.org/776967 | |
| 17:11:25 | stephenfin | ta | |
| 17:12:00 | prometheanfire | was the previous work abandoned? switching to unittest.mock iirc | |
| 17:12:26 | stephenfin | it's not abandoned in Gerrit terms, but nobody is looking at it | |
| 17:12:50 | stephenfin | https://review.opendev.org/c/openstack/nova/+/714676 | |
| 17:12:50 | sean-k-mooney | this is becasue of eventlets and py39 | |
| 17:12:55 | prometheanfire | which is why I pinged :P | |
| 17:12:59 | sean-k-mooney | the websocket proxy is broken on py39 | |
| 17:13:06 | sean-k-mooney | because of dnspython | |
| 17:13:13 | prometheanfire | ya, turtles | |
| 17:13:37 | sean-k-mooney | well actull its brok on 36 too | |