| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-05 | |||
| 18:41:58 | dansmith | but there's not much to indicate what the thinking is there | |
| 18:42:11 | melwitt | okay | |
| 18:42:11 | melwitt | like, continuing on to the other delete code seems like a bug in itself | |
| 18:42:12 | mriedem | if you undo that and run the functional test, you'll see | |
| 18:42:14 | dansmith | I think I have it worked out in my head.. we would just go on to try to delete bdms and maybe fail or something? | |
| 18:42:20 | mriedem | yes, the problem is it continues to the rest of the other delete code | |
| 18:42:24 | mriedem | yes | |
| 18:42:53 | mriedem | i can split that out if it helps | |
| 18:43:02 | dansmith | so the user would see it as a 500 or something on the racing second delete attempt? | |
| 18:43:23 | dansmith | mriedem: since these are for backports, I'm fine with either, but might be good to just add more words about what that change is | |
| 18:43:33 | mriedem | i think it results in a 404 | |
| 18:43:34 | melwitt | I was just thinking out loud | |
| 18:43:37 | mriedem | looking up the bdms | |
| 18:43:43 | mriedem | i can test here locally quick to be sure | |
| 18:43:52 | dansmith | mriedem: "500 or something" includes "404" :P | |
| 18:44:02 | mriedem | "sh is f'ed" | |
| 18:44:05 | mriedem | includes both | |
| 18:44:10 | dansmith | heh | |
| 18:45:13 | melwitt | I understand what the change is for, I was just asking if that needs to be documented as a separate bug I guess | |
| 18:45:44 | mriedem | and return from _delete if we hit InstanceNotFound." | |
| 18:45:44 | mriedem | "This also fixes another bug where we would not short-circuit | |
| 18:45:44 | mriedem | i just lazily lumped it in in the commit message | |
| 18:46:13 | dansmith | right | |
| 18:46:22 | dansmith | that's the thing I think is a little, uh, light on the detail | |
| 18:46:42 | mriedem | heh, well, spent a half day writing the recreate functional regression test, | |
| 18:46:48 | mriedem | i was too tired to care about that detail :P | |
| 18:46:55 | mriedem | psh | |
| 18:47:01 | melwitt | yeah, I see it now. was just a surprise to see a major "let's stop continuing on to the other delete code we shouldn't be running" lumped in with "add regression test" :P | |
| 18:47:02 | dansmith | I haven't even gotten to that part yet :P | |
| 18:47:06 | mriedem | let me see what it does when i remove the return and rerun the func test | |
| 18:47:36 | mriedem | oh, well, | |
| 18:47:45 | mriedem | so the func test fails with a 500 because consoleauth isn't running, | |
| 18:47:53 | mriedem | because it tries going further down that path | |
| 18:48:08 | melwitt | it's fine, I was just confused | |
| 18:49:31 | mriedem | context, instance.uuid) | |
| 18:49:31 | mriedem | self.consoleauth_rpcapi.delete_tokens_for_instance( | |
| 18:49:31 | mriedem | it's this | |
| 18:49:55 | mriedem | let me start consoleauth and see | |
| 18:50:09 | mriedem | ok, so if i start consoleauth, then the functional test is ok if i remove the return statement to short-circuit | |
| 18:50:55 | mriedem | so it's up to you guys i guess | |
| 18:51:11 | mriedem | i think if we hit InstanceNotFound, it makes sense to short-circuit and return, but i can also remove it | |
| 18:52:18 | melwitt | mriedem: I think the return definitely needs to be there. I was just thinking about if we needed that broken out and documented as a separate bug fix. but it looks like that bug is sort of harmless, like extra stuff that's effectively a no-op | |
| 18:52:33 | melwitt | and since that's the case, I think you can just leave it the way it is | |
| 18:52:45 | dansmith | I think the commit message should get more words | |
| 18:53:02 | dansmith | but I'm okay with them being together if you want 'em that way | |
| 18:53:06 | dansmith | the other thing I guess, | |
| 18:53:13 | dansmith | is this covers up the fact that you're not unit testing this change | |
| 18:53:21 | dansmith | and if you split them, it'd be more obvious | |
| 18:53:27 | dansmith | maybe that's a reason to not split, I dunno | |
| 18:53:31 | dansmith | but more words at a minimum I think | |
| 18:53:32 | mriedem | the return? | |
| 18:53:36 | mriedem | that's https://review.openstack.org/#/c/452351/4/nova/tests/unit/compute/test_compute_api.py | |
| 18:53:43 | dansmith | because in six months we'll not remember this | |
| 18:53:46 | mriedem | "# This just lets us exit the test early." | |
| 18:54:03 | dansmith | oh sorry, I thought the functional test was the last file | |
| 18:54:04 | dansmith | I seee now | |
| 18:54:11 | mriedem | i had to put that in that compute api unit test earlier because of the lack of the short circuit | |
| 18:54:22 | mriedem | what i could do is rather than remove that, assert bdms isn't called | |
| 18:54:28 | mriedem | to enforce the return is there | |
| 18:54:31 | mriedem | you like that better? | |
| 18:54:42 | dansmith | I just wasn't seeing that you had any unit test change to account for it | |
| 18:54:44 | dansmith | so I don't care | |
| 18:55:02 | dansmith | well | |
| 18:55:08 | mriedem | hows about, | |
| 18:55:08 | dansmith | yeah, I guess asserting it wasn't called is the best actually | |
| 18:55:33 | mriedem | i split it out, give it it's own special 3 paragraph commit message, and change the removal in the unit test to assert_not_called() | |
| 18:55:45 | mriedem | i think we'll all be happy then | |
| 18:55:47 | dansmith | heh, yeah | |
| 18:55:56 | mriedem | easy peasy | |
| 18:57:23 | melwitt | but ... I'm already happy ... just look at me --> :) | |
| 18:57:29 | mriedem | o/. | |
| 18:57:36 | mriedem | o/~ | |
| 18:57:54 | mriedem | btw, laura has some new amazing deodorant she wants everyone to try, now that i'm thinking about o/~ | |
| 18:57:59 | mriedem | she's constantly shoving her bare pits in my face | |
| 18:58:03 | mriedem | telling me to sniff | |
| 18:58:04 | melwitt | lol | |
| 19:19:36 | mriedem | melwitt: dansmith: here it comes | |
| 19:19:57 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Commit usage decrement after destroying instance https://review.openstack.org/452359 | |
| 19:19:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Short-circuit local delete path for cells v2 and InstanceNotFound https://review.openstack.org/453840 | |
| 19:19:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add regression test for quota decrement bug 1678326 https://review.openstack.org/452351 | |
| 19:19:59 | openstack | bug 1678326 in OpenStack Compute (nova) ocata "Quota is decremented during instance delete in cell0 even if the instance destroy fails" [High,In progress] https://launchpad.net/bugs/1678326 - Assigned to Matt Riedemann (mriedem) | |
| 19:42:35 | mriedem | dansmith: melwitt: thanks for the reviews | |
| 19:42:59 | melwitt | mriedem: thanks for fixing local delete. again | |
| 19:43:08 | mriedem | well, still got this other one to fix now | |
| 19:43:12 | mriedem | will start working on that | |
| 19:43:25 | mriedem | i suppose we have to get the bdms from the cell db that the instance is in | |
| 19:45:21 | mriedem | the callback method might be a pita | |
| 19:52:05 | mriedem | eww this gets into the whole soft delete mess | |
| 19:52:16 | mriedem | but i think we already figured out that you can't soft delete an instance in cell0 | |
| 19:52:45 | mriedem | remember getting caught up in this on the notification patch | |
| 19:52:46 | dansmith | correct | |
| 19:53:03 | dansmith | because there's no way to undelete it | |
| 19:53:14 | mriedem | in the 'normal' flow we set the task_state to 'deleting' before cleaning up bdms and all that | |
| 19:54:28 | mriedem | w/o that we just go from ERROR/None to DELETED/None to destroyed | |
| 19:57:09 | mriedem | god also https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1913-L1930 | |
| 19:58:26 | mriedem | https://review.openstack.org/#/c/61462/ | |
| 19:59:20 | mriedem | so because someone thought we should deleted the shelved snapshot image when the instance is deleted, we did | |
| 20:05:02 | mriedem | this might be easier than i think for shelved offloaded. | |
| 20:10:13 | mriedem | because if the instance is shelved offloaded, it's in a cell | |
| 20:10:26 | mriedem | so the if not instance.host check at the very beginning is just not considering that case | |