| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-05 | |||
| 18:27:45 | dansmith | maybe we should get rid of delete? | |
| 18:27:51 | mriedem | dansmith: that's why i'm holding https://review.openstack.org/#/c/449334/ to the end, | |
| 18:28:04 | mriedem | because i think the fix for that bug is to re-use the _local_delete method in the compute api | |
| 18:28:09 | mriedem | but i think there will be dragons | |
| 18:28:17 | artom | soft-delete in the client is the way to go ;) | |
| 18:28:29 | mriedem | _local_delete is what cleans up ports and volumes | |
| 18:28:38 | mriedem | and was missing from the duplicate 'local delete' that was added for the cells v2 case | |
| 18:29:02 | mriedem | once these ocata regression bugs are fixed, i think we spend some time just removing the transition code in the delete path for cells v2 | |
| 18:29:16 | mriedem | but need the fixes and tests in place before we unwind that onion | |
| 18:30:00 | mriedem | artom: per the deprecation policy, we can deprecate in pike and remove in queens as long as it's been deprecated for at least 3 months | |
| 18:30:16 | mriedem | so you can't deprecate something at the very end of pike and drop it immediately in queens if it's under 3 months | |
| 18:30:24 | mriedem | the 3 months thing is for CD | |
| 18:30:35 | artom | mriedem, I can't argue with the policy | |
| 18:31:03 | artom | I just think it's safer to have a period when both coexist - so that whatever depends on them can change to the new way of doing things while the old way still works | |
| 18:31:23 | mriedem | that's the whole point of the 3 months | |
| 18:31:27 | mriedem | and full stable branch | |
| 18:32:19 | artom | Right, assuming the new way is ready and available during those 3 months ;) | |
| 18:32:24 | artom | Good point for stable though | |
| 18:33:31 | sdague | mriedem: you have 7.0.0 instead of 8.0.0 in https://review.openstack.org/#/c/453819/1 | |
| 18:33:37 | sdague | which is why it's failing tets | |
| 18:33:39 | sdague | tests | |
| 18:33:53 | mriedem | derp | |
| 18:33:55 | mriedem | copy/paste | |
| 18:34:22 | mriedem | it's dependent on a novaclient reno change for now anyway | |
| 18:36:41 | mriedem | dansmith: don't forget the regression test patch below it | |
| 18:37:10 | dansmith | yeah dude, it takes a while :) | |
| 18:37:43 | mriedem | sorry | |
| 18:40:01 | ericyoung | mriedem: eric-aceshome | |
| 18:40:09 | mriedem | ok | |
| 18:41:04 | melwitt | mriedem: is this part not a separate fix on its own? https://review.openstack.org/#/c/452351/4/nova/compute/api.py | |
| 18:41:40 | dansmith | it is and he says that in the commit message, | |
| 18:41:49 | mriedem | i could split that out if needed | |
| 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 | |