| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-26 | |||
| 14:21:21 | melwitt | if you want to pick which one to dupe | |
| 14:21:34 | mriedem | i'll dupe his | |
| 14:21:44 | melwitt | k | |
| 14:22:32 | mriedem | so given takashin is probably done for the weekend | |
| 14:22:32 | danpawlik | mriedem: Ok, sorry for pushing you. I just want to move forward.I'll be patient | |
| 14:22:35 | mriedem | and his patch is incomplete | |
| 14:22:41 | mriedem | i'm not sure if i'll just continue working on mine or clean his up | |
| 14:22:45 | mriedem | plus the trove patches are dependent on mine | |
| 14:29:08 | mriedem | sdague: melwitt: superdan: looks like the trove job failed but it's not failing on the bury in cell0 InstanceNotFound thing with my patch http://logs.openstack.org/47/467447/15/check/gate-trove-scenario-dsvm-mariadb-multi-ubuntu-xenial-nv/30d878e/logs/screen-n-cond.txt.gz?level=TRACE | |
| 14:29:27 | mriedem | now i just need to get unit tests fixed up | |
| 14:30:49 | openstackgerrit | Jay Pipes proposed openstack/nova master: integrate trait cache with TraitList and Trait https://review.openstack.org/462770 | |
| 14:31:14 | sdague | mriedem: nice | |
| 14:32:08 | efried | leakypipes Just saw a pop from an hour and a half ago - something about unicode mismatches? | |
| 14:33:07 | leakypipes | efried: ya, it's cool though. alex_xu is gonna do it tomorrow morning. | |
| 14:33:17 | efried | ight | |
| 14:48:08 | efried | sdague Not sure if you saw, but glanceclient plans to cut a release next Wed. (This is regarding global req id work items) | |
| 14:48:22 | sdague | efried: I did not see that | |
| 14:48:41 | sdague | efried: do glance unit tests run for you? | |
| 14:48:46 | efried | You're wanting https://review.openstack.org/#/c/467592/ to get in there? | |
| 14:48:47 | sdague | because they seem to be hanging for me | |
| 14:48:53 | sdague | efried: yes please | |
| 14:48:59 | efried | sdague They did last week. Which env? | |
| 14:49:52 | sdague | py27 | |
| 14:50:59 | efried | sdague oh, glance, not glanceclient. Haven't done that, possibly ever. | |
| 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 | |