| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-05 | |||
| 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 | |
| 20:10:32 | mriedem | and assumes the instance never got to a cell | |
| 20:12:11 | dansmith | if it's offloaded it's in a cell yeah | |
| 20:12:37 | mriedem | this is literally the fix: | |
| 20:12:40 | mriedem | instance.vm_state == vm_states.SHELVED_OFFLOADED): | |
| 20:12:40 | mriedem | if not (instance.host or | |
| 20:13:06 | melwitt | yeah, later code does "if not instance.host and not shelved_offloaded:" | |
| 20:13:10 | openstackgerrit | Eric Fried proposed openstack/nova master: PowerVM Driver: SSP emphemeral disk support https://review.openstack.org/443189 | |
| 20:13:42 | melwitt | and vm_states.ERROR should be in there too, which is what the patch I've had up for ages does | |
| 20:14:21 | mriedem | so i was trying to think of what we miss now in the first local delete block for cells v2 where instance.host is None | |
| 20:14:24 | mriedem | and it's not shelved offloade | |
| 20:14:28 | mriedem | i think that's just the cell0 case | |
| 20:15:05 | mriedem | where i don't think we need to worry about cleaning up bdms, because conductor doesn't create the bdms in cell0 | |
| 20:15:48 | mriedem | _bury_in_cell0 creates the instance in cell0, then deletes the build request | |
| 20:23:37 | mriedem | crap we might have this same issue in newton https://github.com/openstack/nova/blob/stable/newton/nova/compute/api.py#L1682 | |
| 20:31:23 | openstackgerrit | Scott Moser proposed openstack/nova master: Limit exposure of network device types to the guest. https://review.openstack.org/400883 | |
| 20:31:33 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Regression test for local delete with an attached volume https://review.openstack.org/449334 | |
| 20:31:34 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Perform old-style local delete for shelved offloaded instances https://review.openstack.org/453859 | |
| 20:31:34 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Set size/status during image create with FakeImageService https://review.openstack.org/449702 | |
| 20:31:44 | mriedem | thar she blar | |
| 20:32:56 | dtp | mriedem how do you guys do rollbacks w/o downgrade on the migrations? | |
| 20:33:03 | mriedem | dtp: we dont | |
| 20:33:22 | mriedem | roll forward to the maxxx | |
| 20:33:33 | mriedem | i usually do the dew while rolling forward | |
| 20:34:07 | dtp | nova-manage db sync supports downgrade | |
| 20:34:31 | mriedem | it might say it does | |
| 20:34:35 | mriedem | which would be a bug | |
| 20:34:57 | dtp | i think it actually does....or else my upgrade is already idempotent | |
| 20:35:18 | dtp | because 358 > 359 > 358 > 359 works | |
| 20:35:40 | mriedem | well it works for your migration b/c you implemented downgrade | |
| 20:36:09 | mriedem | as i said in the review, i'm surprised we didn't have a test that would fail saying we don't support downgrade | |
| 20:36:18 | dtp | ok ok | |
| 20:36:38 | dtp | thanks for the review, btw | |
| 20:37:12 | mriedem | np | |
| 20:37:57 | mriedem | dtp: so i guess we weren't idempotent when adding the compute_nodes.uuid column either https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/migrate_repo/versions/314_add_resource_provider_tables.py#L78 | |
| 20:38:43 | mriedem | but we should be, e.g. https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/migrate_repo/versions/358_bdm_attachment_id.py#L27 | |
| 20:39:24 | dtp | makes sense | |
| 20:40:04 | mriedem | dtp: btw, when you said nova-manage db sync supports downgrade, you mean by passing in the --version right? | |
| 20:40:37 | dtp | yep | |
| 20:40:44 | mriedem | yeah i think if you tried going further, it'd explode | |
| 20:42:01 | dtp | surely | |
| 20:43:39 | mriedem | huh clearly test_no_migrations_have_downgrade failed it's job | |