Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-05-26
14:51:10 sdague I'm trying to run locally and http://paste.openstack.org/show/610745/
14:51:22 sdague I guess it's all just timing out now
14:52:23 efried sdague It wouldn't surprise me if there are latent bugs; there were in glanceclient. And the last couple weeks have been pretty chaotic around there.
14:53:48 efried I'm pulling & running locally, will let you know what I find.
14:54:28 mriedem \o/ context manager is successfully mocked
14:57:06 sdague mriedem: sweet
14:57:19 efried sdague Okay, I hadn't gotten all the way through my emails - rosmaita called out your patch on the dev ML.
14:57:30 openstackgerrit Jeremy Liu proposed openstack/python-novaclient master: Fix reservation_id not supported by Nova API https://review.openstack.org/468444
14:57:49 sdague efried: https://review.openstack.org/#/c/468443/ is the thing I'd like to also get in on the glance side, because today the pass just overwrites the real request_id
14:59:13 efried sdague Okay. (In case there's been some confusion, I'm not a glance guy - just jumped in there for the first time last week to get service-tokens work moving ;)
15:00:14 mriedem efried: you are now a glance guy
15:00:15 mriedem deal with it
15:00:20 sdague mriedem: ++
15:00:25 efried dangit
15:07:27 openstackgerrit Matt Riedemann proposed openstack/nova master: Use targeted context when burying instances in cell0 https://review.openstack.org/468401
15:07:28 mriedem sdague: melwitt: superdan: ^ now with tests
15:08:05 mriedem https://review.openstack.org/#/c/467447/ passed with that
15:12:11 melwitt mriedem: none of those tests repro the original problem, where the instance can't be deleted, do they?
15:13:32 superdan because they're not actually using the full cell fixture right?
15:14:16 mriedem they aren't using the cells fixtures no
15:14:24 mriedem well,
15:14:37 mriedem they would be via the base test case
15:15:12 mriedem if you want a full regression suite, that's probably a functional test, and probably the rest of my day
15:16:01 mriedem i'm not actually sure why the TestDeleteFromCell0CheckQuota functional test never caught this
15:16:13 mriedem since it's simulating a case where the instance fails to schedule and goes into ERROR state, and goes into cell0
15:17:13 mriedem could have to do with not actually doing notifications in tests
15:17:14 mriedem checking
15:17:23 melwitt okay, just thinking about how this got past tests in the first place
15:18:16 sdague mriedem: any chance that it wasn't magicing the db enough?
15:18:26 melwitt because the problem is the InstanceMapping is pointing at a NULL cell_id
15:19:11 melwitt this is where the issue is right? https://review.openstack.org/#/c/468401/3/nova/conductor/manager.py@893 maybe we could at least assert the right context in that call in unit tests
15:19:36 mriedem melwitt: that's why the unit test changes do
15:19:52 melwitt I thought those were only changes about notifications
15:20:16 efried sdague FYI glance py35: Ran 625 tests in 1216.144s \ OK. py27 cranking up now.
15:20:17 mriedem it doesn't blow up there, it's before that,
15:20:48 mriedem maybe you're asking that the tests also assert InstanceMapping.get_by_instance_uuid
15:20:58 mriedem which is probably raising NotFound and we pass
15:21:43 melwitt maybe. I trust the fix, just saying none of the test changes are covering it. we can add coverage as a follow up if this is urgent
15:21:54 mriedem superdan: so i'm guessing the reason this didn't fail functional tests like TestDeleteFromCell0CheckQuota is because the CellDatabases fixture is patching target_cell?
15:22:10 superdan melwitt: no, the set_vm_state thing does actual changes to the instance right?
15:22:15 sdague melwitt: on the up side we have real tests in trove that at least expose it
15:22:23 sdague which is how we got here
15:22:55 superdan mriedem: I dunno, I need to look at that test.. are you using CellDatabases or SingleCellSimple there?
15:23:11 mriedem CellDatabases since USES_DB=True
15:23:18 superdan yeah okay
15:24:01 mriedem superdan: so for your question,
15:24:04 mriedem i asked the same thing in your series https://review.openstack.org/#/c/458634/13/nova/objects/host_mapping.py@227
15:24:16 melwitt superdan: oh, yeah. set_vm_state makes the changes to the instance, so that explains the wonky states from the bug
15:24:39 melwitt but the failed deletion part is that the InstanceMapping isn't pointing at the cell where the instance is
15:25:03 superdan mriedem: that's different because it also talks to the celldb
15:25:23 mriedem i'll modify one of these tests to make sure the targeted context is passed to InstanceMapping.get_by_instance_uuid
15:25:54 mriedem superdan: it being _check_and_create_host_mappings ?
15:26:10 superdan mriedem: yeah
15:26:18 mriedem the context passed in there is only used for the api db
15:26:26 melwitt is the trove failure because of an inability to delete the instance? so we at least know the instance can be deleted with this fix
15:26:28 mriedem since i thought compute.save() would already be targeted to the cell
15:26:34 mriedem melwitt: yes
15:26:39 melwitt okay, cool
15:26:40 mriedem it hangs in scheduling state for ever and the test times out
15:27:03 superdan mriedem: well, before the target yield it had to remain targeted because it was the shared context
15:27:30 mriedem alright i'll just undo that part
15:27:41 mriedem woe to new contributors
15:27:43 superdan mriedem: I didn't -1 for it I was just gonna think on it
15:27:46 mriedem ok
15:28:00 mriedem i'll update tests in the meantime
15:29:00 superdan mriedem: do you understand why your test didn't catch this?
15:29:09 mriedem the functional one? no
15:29:19 mriedem i didn't dig into the fixture magic
15:29:20 superdan okay I'm still thinking on that
15:29:43 superdan in devstack land, cell0 is the default connection so it would make sense to have gotten a free pass because of that
15:30:10 superdan but with CellDatabases, it chooses cell1 to make the transition easier
15:30:34 melwitt I still don't get why the fix works. how does it make the instance findable via InstanceMapping?
15:31:11 superdan melwitt: it was failing to ever do the assignment because it failed the vm state update right?
15:31:32 superdan explote on L888 means it never ran L892?
15:31:35 superdan *explode
15:31:44 melwitt ohhh
15:32:13 melwitt right, thanks
15:32:55 melwitt so the tests are covering it because it's not only notifications, it's an instance.save() with the right context and verifying that
15:33:58 mriedem so i'm not changing anything right now then?
15:35:01 superdan I don't think we need to hold up on the context thing.. since it's very specifically not needed, I would probably have not used the targeted one, but not sure it's worth changing
15:35:17 superdan do you want to drill down on the functional test not catching it or no?
15:35:23 superdan if this is holding up people,
15:35:28 superdan an the trove test passes, I'd say we go
15:35:54 melwitt I'm cool with doing this now and looking into the functional test as a follow up
15:35:55 mriedem i changed the InstanceMapping.get_by_instance_uuid to use context instead of cctxt and no unit tests failed, so we're not asserting that
15:36:10 mriedem so i can change that back, or add an assertion, or do nothing in this change
15:36:23 mriedem it's an easy switch back to just using context for that call
15:36:27 superdan well, not validating it because it's not a big deal
15:36:37 superdan I'm cool with either, up to you
15:36:55 melwitt ditto
15:37:34 mriedem i'll change it and add comments
15:37:36 mriedem b/c otherwise it's confusing
15:38:27 mriedem i'll recheck the trove change on this one too
15:38:35 openstackgerrit Matt Riedemann proposed openstack/nova master: Use targeted context when burying instances in cell0 https://review.openstack.org/468401
15:39:11 efried sdague py27 finished with one failure for me: http://paste.openstack.org/show/610750/
15:40:22 openstack Launchpad bug 1693829 in OpenStack Compute (nova) "name too long" [Undecided,New]
15:40:22 mriedem leakypipes: i added MOS to this https://bugs.launchpad.net/mos/+bug/1693829
15:40:27 mriedem in case you guys have triage people
15:40:48 leakypipes k
15:42:28 viks Hi seeking attention on https://review.openstack.org/#/c/468089/ . Need one more verification :)

Earlier   Later