Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-04-05
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 dansmith yeah, I guess asserting it wasn't called is the best actually
18:55:08 mriedem hows about,
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: Add regression test for quota decrement bug 1678326 https://review.openstack.org/452351
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: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 if not (instance.host or
20:12:40 mriedem instance.vm_state == vm_states.SHELVED_OFFLOADED):
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: Set size/status during image create with FakeImageService https://review.openstack.org/449702
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: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
20:43:42 edleafe OK, what am I doing wrong? I'm making a commit to the os-traits library. I have git-review installed, and a .gitreview file in the directory, just as in my nova project. But when I commit, I'm not getting a Change-Id added to the commit message.
20:43:58 mriedem edleafe: git review -s
20:44:02 mriedem in the repo
20:44:12 mriedem you probably don't have the gerrit remote
20:44:31 edleafe mriedem: that worked, but I did have the remote
20:44:45 mriedem maybe git review -s installs a hook or something, idk
20:45:23 edleafe dunno, but thanks for setting me straight
20:46:44 melwitt edleafe: you have to do git review -s anytime you clone a new repo, fwiw
20:47:58 edleafe melwitt: yeah, I thought I had, but I guess not...

Earlier   Later