Posted	Nick	Remark
#openstack-nova - 2017-12-06
15:41:21	mriedem	efried: jaypipes: i think empty list is the right thing
15:41:31	mriedem	404 is for a specific resource not being found
15:41:42	mriedem	e.g. GET /resource_providers/{id} where the RP for {id} isn't found
15:42:08	mriedem	something not found in a body is a 400, but this is a filter parameter
15:42:20	mriedem	filtering on something which doesn't exist gets you nothing back
15:42:43	mriedem	i seem to remember this same quandary in another spec, maybe for the traits stuff
15:42:47	belmoreira	dansmith: I need to test it. But still not sure why it's required.
15:43:00	dansmith	belmoreira: not sure why the cell mapping is required?
15:43:31	efried	mriedem jaypipes okay, that makes sense, thanks.
15:44:11	belmoreira	dansmith: I understood that you said that in nova_api global I need to have a mapping to the nova DB (father cell)
15:44:36	dansmith	right
15:45:36	edleafe	efried: what mriedem said. Empty list is correct.
15:46:02	efried	thanks
15:46:14	belmoreira	dansmith: I completely forgot to do that mapping. And in my early tests I haven't seen any problem
15:46:59	dansmith	belmoreira: cellsv1 will probably side-step some things that would make things continue to work without that mapping, but not all
15:47:15	dansmith	belmoreira: just because it overrides certain code paths that we're not updating for cellsv2
15:47:36	belmoreira	dansmith: fair enough. Must be that
15:47:37	dansmith	belmoreira: I wouldn't think it would override enough to make it seem like things are really working, but I'd have to test, as you say
15:48:00	mriedem	in ocata,
15:48:15	mriedem	it looks like the only things that use cell mappings are the api when listing instances, and conductor when burying an instance that fails to shedule in cell0
15:48:23	mriedem	for cellsv1, are those things that only happen in the child api?
15:48:31	dansmith	no, that's the thing,
15:48:36	openstackgerrit	Huan Xie proposed openstack/nova master: XenAPI: Don't use nicira-iface-id for XenServer VIF https://review.openstack.org/451641
15:48:42	dansmith	listing instances should be mostly the same,
15:48:47	dansmith	althouuuuugh...
15:49:00	dansmith	we do have some "if cellsv1: then do_old_thing()" escapes in that listing logic
15:49:01	dansmith	so maybe that's it
15:49:07	mriedem	oh wait,
15:49:08	mriedem	yup
15:49:15	dansmith	yeah, okay
15:49:16	mriedem	when listing instances, if cellsv1 we do the old thing
15:49:26	mriedem	we do look in cell0 though
15:49:30	dansmith	yeah
15:49:31	mriedem	so you'll need the cell0 mapping at the top yes?
15:49:32	mriedem	ok
15:49:34	dansmith	yes
15:49:38	mriedem	belmoreira: ^
15:49:46	dansmith	belmoreira: you will want the record there anyway, at some point
15:49:47	mriedem	scheduler and api don't use cell mappings until pike
15:50:02	dansmith	belmoreira: also, while you're here.. this fix is tanking the cellsv1 job: https://review.openstack.org/#/c/523187/
15:50:06	dansmith	which is, like, not a good sign,
15:50:11	belmoreira	yeah, cell0 I defined
15:50:13	dansmith	but I'm not sure why, especially since you said it works
15:50:36	dansmith	like, 100% fail on anything that hits nova
15:50:40	dansmith	which seems hard to believe
15:55:22	belmoreira	dansmith: I'm running this patch already. Haven't detected any issue yet. However, the functionality that we expose is very limited. Maybe that's why.
15:55:24	huanxie	Hi jaypipes, I have updated the reno to make it precise and please help review it again https://review.openstack.org/#/c/451641/ Thanks a lot :)
15:55:45	mriedem	belmoreira: can't even build an instance it looks like
15:55:47	jaypipes	huanxie: already +2d.
15:55:48	dansmith	belmoreira: I dunno, this seems to fail everything all the time, but I can't really see any errors in the logs
15:55:49	mriedem	times out waiting to go to ACTIVE
15:56:17	huanxie	Many thanks jaypipes :)
15:56:21	belmoreira	dansmith: but, as you said, not a good sign :) Will keep eyes open on this
15:56:58	dansmith	well, anyway, I don't want to spend too much time on it, but unless we figure that out, we can't merge it
15:57:09	belmoreira	dansmith: definitely create/delete instances is working for us with this patch
15:57:44	dansmith	although that's ocata I guess
15:57:45	belmoreira	but I'm running newton
15:57:49	dansmith	oh, newton
15:58:07	dansmith	well, still, I'd expect to see an error somewhere if this was actually blowing something up
15:58:19	belmoreira	in pike I will not need this patch
15:59:12	openstackgerrit	Merged openstack/nova master: Add a new check to volume attach https://review.openstack.org/525622
15:59:43	mriedem	dansmith: looking at the instance create flow, if cellsv1 the api will create the instance before casting to build_instances in conductor,
15:59:49	mriedem	build_instances in conductor creates the instance mapping
16:00:13	mriedem	well, api creates the instance mapping
16:00:38	mriedem	conductor gets the host mapping for the chosen host and pulls the cell mapping from that host mapping to set on the instance mapping
16:00:41	mriedem	then deletes the build request
16:00:53	mriedem	ah,
16:01:03	mriedem	and if the build request is already destroyed when conductor tries to do it,
16:01:07	mriedem	conductor deletes the instance mapping
16:01:11	mriedem	which the api relies on i think
16:01:40	mriedem	https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L599-L605
16:01:43	dansmith	oh you think we're deleting the build request early enough that it just nukes the instance when it tries because it assumes the user did it?
16:02:34	mriedem	maybe
16:02:46	mriedem	and when getting the instance from the api, we'll pull it from the top cell https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2221
16:02:52	dansmith	the update at top thing should really happen pretty late, after the cell sync, so I didn't think that was a problem
16:03:06	mriedem	update at top happens for any state change doesn't it?
16:03:23	mriedem	https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L573
16:03:41	mriedem	so ^ should trigger an update at the top
16:03:44	mriedem	which will delete the build request
16:04:01	mriedem	and then later we'll get build request not found which will make conductor delete the instance mapping https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L604
16:04:07	mriedem	thinking the instance was deleted by the user during build
16:04:42	mriedem	yup, and https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L605 means we don't cast to the compute to build the instance
16:04:43	dansmith	yeah, but I expected it to happen late enough
16:05:52	mriedem	i think i get a cookie now
16:06:00	mriedem	frosted sugar with sprinkles please
16:06:01	dansmith	weyl.. I'm not sure what to do, other than to weaken conductor's interpretation of the BR being missing if cellsv1
16:06:21	mriedem	can we conditionally delete the build request in cells/messaging based on the instance state?
16:07:00	dansmith	well, which state?
16:07:06	ildikov	mriedem: I'm at a place selling cupcakes one of which is called 'Uniporn & Rainho', sucks I can't send over one through IRC :/
16:07:21	mriedem	dansmith: is ACTIVE too late?
16:07:47	dansmith	I would think,
16:07:53	dansmith	since we might not even make it to ACTIVE
16:08:10	mriedem	i'm not even sure why we need this
16:08:14	dansmith	what if we do it only if save() succeeded
16:08:15	mriedem	given https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L597
16:08:50	dansmith	mriedem: I think it's because that might've happened in a separate api db
16:08:57	mriedem	we could alternatively check if we've set the cell mapping on the instance mapping, meaning we've picked a host, https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L509 but that's racy
16:09:19	mriedem	because then conductor and cells/messaging are racing to delete the build request
16:09:30	mriedem	but cells/messaging will only do it on the next update at top
16:09:40	mriedem	which presumably would come after conductor has set the mapping and already deleted the build request

Earlier   Later