Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-05-26
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 mriedem leakypipes: i added MOS to this https://bugs.launchpad.net/mos/+bug/1693829
15:40:22 openstack Launchpad bug 1693829 in OpenStack Compute (nova) "name too long" [Undecided,New]
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 :)
15:44:05 viks thanks mriedem
15:51:47 openstackgerrit Claudiu Belu proposed openstack/nova master: POC: tests: autospecs all the mock.patch usages https://review.openstack.org/447505
15:54:09 mriedem mtreinish: uh oh! https://bugs.launchpad.net/nova/+bug/1693257
15:54:09 openstack Launchpad bug 1693257 in OpenStack Compute (nova) "Policy doesn't allow os_compute_api:os-cloudpipe to be performed" [Undecided,New]
15:54:16 mriedem there might have been 1 cloudpipe user in the wild
15:55:15 mtreinish mriedem: haha, well they're in for a surprise in pike
15:56:52 sean-k-mooney hi o/
15:58:24 sean-k-mooney mriedem: leakypipes sfinucan can i ye take a look at https://bugs.launchpad.net/nova/+bug/1691178 and https://review.openstack.org/#/c/465160/
15:58:24 openstack Launchpad bug 1691178 in OpenStack Compute (nova) "virt_type=qemu does not support cpu affinity." [Undecided,New] - Assigned to sean mooney (sean-k-mooney)
15:58:55 mriedem mtreinish: i don't think they understand policy anyway,
15:59:01 mriedem it's admin-only by default and they are probably using non-admin
15:59:29 mriedem sean-k-mooney: you realize that's going to require tests
15:59:35 sean-k-mooney mriedem: leakypipes superdan just wondering if this is a bug you want to fix/ should i also adress this when selecting the host?
15:59:38 sean-k-mooney mriedem: yes
15:59:47 mriedem i don't know anything about this code
16:00:08 sean-k-mooney mriedem: qemu without kvm/zen does not support cpu affinity
16:00:41 sean-k-mooney we generate vcpu-pin ellement anyway and the vm fails to boot with a livbirt error
16:00:50 cfriesen sdague: does the api-ref changes in https://review.openstack.org/#/c/434514 match what you were looking for? (incidentally, it's lame that the min/max version is in parameters.yaml instead of the *.inc files, it leads to duplicated entries differing only in min/max version)
16:00:53 leakypipes sean-k-mooney: on the bug report, you put expected result that the VM starts with a virt topology. however, TCG doesn't support NUMA so how is that right?
16:01:04 mriedem sean-k-mooney: do you need to have that refactor in there?

Earlier   Later