Posted	Nick	Remark
#openstack-nova - 2018-05-03
21:14:36	mriedem	well,
21:14:43	mriedem	i think there are two cases here:
21:14:46	mriedem	for affintiy
21:15:10	mriedem	1. a group member is already in a host (so a cell) and your lookup finds it, we know our new instance has to go in that cell to affine to the existing member
21:15:28	mriedem	worst case there is we're needlessly iterating multiple cells when we already know which one we want
21:15:40	mriedem	so, perf optimization for later maybe,
21:16:13	mriedem	2. we're creating multiple instances concurrently where there are no existing members in the affinity group - iterating the cells is fine but we can still have the scheduler put the members in the wrong cells
21:16:29	mriedem	i.e. it's the same affinity race for concurrent creates that we've always had,
21:16:34	mriedem	but the late check in the compute can't fix that one
21:16:48	mriedem	because we don't reschedule across cells, only within the same cell we're already in
21:16:50	melwitt	I see, yeah
21:17:52	melwitt	I think we have to check all cells though right? we have to pull instances to find out what host they're on
21:17:59	mriedem	so in case 2 you get a novalidhost / maxretriesexceeded and have to delete and recreate the instance, which at that point should work
21:18:28	openstackgerrit	Oliver Walsh proposed openstack/nova stable/pike: Handle @safe_connect returns None side effect in _ensure_resource_provider https://review.openstack.org/566164
21:18:30	melwitt	that is, I don't think there's a way to avoid having to look in all cells unless you mean do it iteratively instead of a scatter-gather and stop early when found
21:18:43	mriedem	melwitt: yeah i think the latter,
21:18:44	openstackgerrit	karim proposed openstack/nova master: Pushing image traits to ironic node https://review.openstack.org/565620
21:18:47	melwitt	gotcha
21:19:17	mriedem	like, if policy == 'affinity' lookup the instance mappings for the group members, and they should all be in the same cell, so just get hosts from that one
21:19:42	mriedem	or just get one instance mapping for one member in the affinity group and pick it's host, because the new instance *has* to go on that same host
21:19:53	mriedem	anyway, like i said, perf optimization for later if we care
21:20:12	mriedem	i just wanted to make sure i was thinking through this correctly
21:21:46	melwitt	oh, I see what you mean now, get the group members and choose a cell based on that
21:21:51	openstackgerrit	Jay Pipes proposed openstack/nova master: rework how we pass candidate request information https://review.openstack.org/566166
21:22:35	melwitt	yeah, I think the only potential wrinkle there is, that would mean affinity always means one cell forever
21:23:47	melwitt	and I'm not 100% sure that's how it's supposed to work considering cells are transparent, that is, if you imagine you have some application that will run faster if its instances are affined to a host, does that mean you want your application to only be able to run in one cell? or do you want it to run in multiple cells where each cell will have its instances running on the same host
21:24:20	dansmith	if you want the latter you need two groups regardless of cells right?
21:24:23	melwitt	and have multiple instances of your application running from multiple cells? maybe that's not a thing that makes sense but just for the sake of thinking about it
21:24:31	dansmith	if you asked for host affinity, then "same cell" is kinda implied no?
21:24:42	melwitt	maybe, I'm not that much of an affinity expert
21:24:52	dansmith	affinity means "same host" currently
21:24:53	melwitt	if it is, then that's cool. I just didn't know
21:24:59	dansmith	anti-affinity means "any other host"
21:25:04	dansmith	or rather "not the same host"
21:25:56	melwitt	right, okay. that makes it clear then. I don't know why I was thinking about multiple per cell affinity
21:32:08	mriedem	looking at https://github.com/openstack/nova/blob/0ef3c685b9d2e0049f38fcf1a268870e69a5b9cf/nova/compute/manager.py#L1289 again,
21:32:26	mriedem	i think it's possible that you could have 2 affinity group members in different cells and we wouldn't fail
21:33:10	mriedem	because if the group has instA and instB in cell1 and cell2 respectively, group_hosts = group.get_hosts(exclude=[instance.uuid]) for instA won't return the host for instB because it's in another cell, and we're not targeted for that other cell
21:33:20	mriedem	so group_hosts would be empty and we'll say all is clear
21:34:29	melwitt	yeah ... you're talking about the current state of affinity with multiple cells?
21:34:43	mriedem	yeah regardless of this change
21:34:58	melwitt	yeah, what I found is that since we're not targeted, we just get an empty hosts list every time
21:35:28	mriedem	hmm, is the group targeted when we get down to compute? https://github.com/openstack/nova/blob/0ef3c685b9d2e0049f38fcf1a268870e69a5b9cf/nova/compute/manager.py#L1289
21:36:25	melwitt	no because the group is pulled early before we target anything. I hope I wrote comments in the patch about that, I'm having trouble remembering it even now
21:37:06	dansmith	that'd be a bug, of course,
21:37:06	melwitt	oh, that in compute manager, that will be local to a cell
21:37:36	dansmith	if two instances in an affinity group were in different cells
21:37:48	dansmith	which maybe could have happened if we did something dumb in scheduler
21:37:54	dansmith	(maybe related to melwitt's patch)
21:38:03	dansmith	but that would be, like, bad, not a legit arrangement
21:38:12	melwitt	yeah. well, it definitely happens today because the query for hosts will always be empty
21:38:16	mriedem	dansmith: yeah it's totally a bug but i think it's something that could happen today with multiple cells and concurrent requests to create instances in the same affinity group
21:38:25	dansmith	ack, okay
21:39:11	melwitt	it doesn't have to be concurrent because no matter what, when you go to boot an instance and say "give me the hosts that are in this group" it will hand you an empty list because it's an untargeted DB access by nova-api
21:39:15	dansmith	probably worth backporting that fix then and calling it out in a reno that people should audit their groups if they are running multiple cells
21:39:22	dansmith	melwitt: yeah
21:40:28	openstackgerrit	Julia Kreger proposed openstack/nova master: ironic: add instance_uuid before any other spawn activity https://review.openstack.org/563722
21:40:55	mriedem	so i still can't tell,
21:41:00	mriedem	during an instance create,
21:41:11	mriedem	when we get to the late affinity check in compute https://github.com/openstack/nova/blob/0ef3c685b9d2e0049f38fcf1a268870e69a5b9cf/nova/compute/manager.py#L1288
21:41:20	mriedem	is that group going to have a cell-targeted context in it?
21:41:24	dansmith	no,
21:41:27	dansmith	because it's local to the cell
21:41:29	dansmith	if it's compute manager
21:41:34	dansmith	it can't go anywhere other than the local cell
21:41:38	mriedem	so it just reads the [database]/connection
21:41:41	dansmith	right
21:41:41	mriedem	ah right
21:42:06	melwitt	the thing I'm concerned about is whether my patch will make that go to all cells, which would be wrong?
21:42:56	dansmith	does compute manager use setup_instance_group?
21:42:58	dansmith	I didn't think so
21:43:14	melwitt	oh, no it doesn't. okay, my bad
21:43:50	mriedem	setup_instance_group is only called from conductor i think, prior to sending the request spec to the scheduler to pick a host
21:44:27	dansmith	you know that there's still a race though, right?
21:44:31	dansmith	that's what the late-binding check is for,
21:44:39	dansmith	and we can't do that from the cell anyway
21:44:57	dansmith	so melwitt's patch will close the race by not just getting an empty list every time,
21:45:09	dansmith	but if we're concurrently booting several instances, you can still sprinkle them among the cells
21:45:33	dansmith	although, that makes me wonder...
21:45:34	mriedem	right i know for the concurrent boot scenario, we're already in trouble
21:45:41	dansmith	instead of doing the check late like we do now,
21:46:12	dansmith	what if we create the instance mapping (which means we've destined it to a cell) and then do the affinity check right there to make sure that the group hasn't spread across cells,
21:46:29	dansmith	then once we've done that, we can let the late check happen in computemanager like normal, which will only need to consider the local cell anyway
21:47:22	mriedem	there is still a race there though isn't there?
21:47:30	mriedem	it's the same race as for getting on different computes
21:47:59	dansmith	no, because if we know everything is in the same cell, then the late check as it is today works fine yeah?
21:48:07	dansmith	oh, although we can't look up the server group members there
21:48:08	dansmith	that's why
21:48:34	mriedem	my brain hurts
21:49:17	mriedem	so to summarize, i think with the concurrent affinity group create race, before multi-cell, you could race past the scheduler and late affinity check in the compute has to fix the race,
21:49:43	mriedem	with multiple cells, you could race past the scheduler and the late affinity check in compute might not fail because it can't "see" that the group has members in another cell
21:49:47	mriedem	to which it's not affined
21:49:54	mriedem	and you end up with an affinity group with members in multiple cells
21:49:55	dansmith	no,
21:50:21	dansmith	the late check is just for anti-affinity
21:50:22	dansmith	right?
21:50:26	mriedem	no
21:50:37	dansmith	oh, I guess it does check both
21:50:42	dansmith	the comment only says anti-affinity
21:51:10	dansmith	mriedem: so yes, your second comment is right, except that:

Earlier   Later