Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-05-03
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 melwitt oh, that in compute manager, that will be local to a cell
21:37:06 dansmith that'd be a bug, of course,
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 mriedem ah right
21:41:41 dansmith 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:
21:51:23 mriedem so before multi-cell, you'd reschedule until you find the right host or fail,
21:51:24 dansmith 1. for anti-affinity it doesn't matter since anything it doesn't find is clearly not on the same host and,
21:51:37 dansmith 2. the problem is we can't upcall to get the members, not just that we can't see them in our db
21:51:37 mriedem after multi-cell, we might not even see the problem
21:51:52 dansmith right but with multi-cell #2 gets you anyway
21:51:53 mriedem agree with 1 yes, anti-affinity is totes fine
21:51:55 dansmith because of the upcall
21:52:42 mriedem if the cell isn't configured to hit the api db, then yes we can't upcall to iterate the cells to find hosts for all members in the group
21:53:08 mriedem this https://github.com/openstack/nova/blob/0ef3c685b9d2e0049f38fcf1a268870e69a5b9cf/nova/objects/instance_group.py#L473
21:53:09 mriedem yeah?
21:53:38 dansmith well,
21:53:45 dansmith we won't iterate cells anyway, but yeah
21:58:34 mriedem so the only way to really fix this is for the scheduler to be aware of the location of the group members at the time of picking a host, and that has to be global, so placement,
21:58:59 mriedem and we likely need to model affinity (distance) for the group members using....provider aggregates? i know this is a rathole,
21:59:05 mriedem i keep thinking of cells as a provider aggregate
21:59:45 dansmith well, this is why I've been punting the problem because placement affinity is better for lots of reasons
22:00:04 mriedem melwitt: anyway, i think the functional change in your patch is ok, my main issue / concern is with the test
22:00:23 melwitt mriedem: yeah, in the middle of replying now
22:00:30 mriedem melwitt: a functional test with real services would be ideal
22:00:57 melwitt well, I think we need the unit test to cover all the weird paths, unless I'm just not thinking creatively enough
22:01:32 mriedem the majority of the work in a functional test for stuff like this is in the setup,
22:01:34 melwitt I'm gonna try to take a step back and see if I can come up with a real world scenario(s) that will take all of the paths that we can verify
22:01:37 mriedem after that it's just running scenarios and asserting stuff
22:02:07 mriedem https://review.openstack.org/#/c/565886/6/nova/tests/functional/test_nova_manage.py@357 is an example with 2 working cells
22:03:39 melwitt this is weird because there are two steps: 1) look up the hosts for members of the requested group 2) look up the hosts for members of the group the instance is a member of
22:04:38 melwitt and I was trying to test that both of those queries do the multi-cell thing. but yeah, probably could set this up for a functional test. I'll try it
22:05:37 mriedem with complicated changes like this, i find it's easier to write the functional test to setup the environment like the user would run a use case
22:05:48 mriedem using the actual APIs
22:05:53 mriedem to create the groups and add members to them and such
22:06:16 mriedem otherwise it's too easy to fake things out in the db that aren't accurate
22:07:17 melwitt for whatever reason, I did not expect it would be easy to do a functional test. I agree it would be a lot better to reason about too
22:07:29 mriedem mostly just copy/paste the setup
22:07:31 mriedem pretty easy
22:08:07 mriedem the one thing with multi-cell functional and having different hosts in different cells, you'll need https://review.openstack.org/#/c/558160/
22:08:19 mriedem otherwise the computes all get created in the default cell1

Earlier   Later