| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-17 | |||
| 18:50:22 | mriedem | correct | |
| 18:50:29 | mriedem | although i can't recreate it locally with a functional test | |
| 18:52:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add regression test for bug 1781710 https://review.openstack.org/583339 | |
| 18:53:00 | openstack | bug 1781710 in OpenStack Compute (nova) "ServersOnMultiNodesTest.test_create_server_with_scheduler_hint_group_anti_affinity failing with "Servers are on the same host"" [High,Fix released] https://launchpad.net/bugs/1781710 - Assigned to Matt Riedemann (mriedem) | |
| 18:53:00 | mriedem | ^ | |
| 18:56:34 | mriedem | i think this might be causing problems https://github.com/openstack/nova/blob/master/nova/scheduler/filters/affinity_filter.py#L98 | |
| 18:57:22 | mriedem | actually it's something else | |
| 18:57:29 | mriedem | reqspec.instance_group.members is 0 | |
| 18:58:29 | sean-k-mooney | on line 103 | |
| 18:58:39 | mriedem | yes | |
| 18:58:48 | mriedem | but then i'm not sure why my functional regression test would be passing | |
| 18:58:58 | mriedem | b/c i've added a custom weigher which should put each instance on it's own host | |
| 19:00:04 | sean-k-mooney | well its anti afinity so is that not the desired result? | |
| 19:00:52 | sean-k-mooney | im just reading over the regression test code. i dont dig into the functional tests often | |
| 19:01:48 | mriedem | actually the thing in the api doesn't matter, because prior to scheduling, conductor calls setup_instance_group which gets the current group information from the db which should have the current members in it | |
| 19:01:52 | mriedem | before they get to the filter | |
| 19:04:25 | sean-k-mooney | my knoladge of multi create is a little less clear then it should be. all instace in the multicreate are part of the same api request. are all instance schduled indepently or does the schduler process the batch of instance as one request. | |
| 19:05:13 | mriedem | the scheduler processes the batch as one request, | |
| 19:05:21 | mriedem | and as a host is selected per instance, | |
| 19:05:46 | mriedem | the scheduler 'consumers' resources for the in-memory representation of that host, | |
| 19:05:51 | mriedem | then filters the hosts for the next instance in the list | |
| 19:06:44 | mriedem | which is why in my change here https://review.openstack.org/#/c/582976/ i'm mapping the instance to the selected host in its HostState.instances dict | |
| 19:07:00 | mriedem | which is used in the anti-affinity filter to determine which members of the group on a given host | |
| 19:07:04 | sean-k-mooney | right so for the second time true the anti afinity filter the instances = set(host_state.instances.keys()) line retrives the updated inmemory copy of the instance state not the current state | |
| 19:07:08 | mriedem | https://github.com/openstack/nova/blob/master/nova/scheduler/filters/affinity_filter.py#L101 | |
| 19:07:23 | mriedem | that's the idea | |
| 19:10:01 | sean-k-mooney | hum ya reading the code of that change it makes sense in that context. | |
| 19:14:22 | mriedem | i think this is the killer https://github.com/openstack/nova/blob/master/nova/scheduler/filters/affinity_filter.py#L98 | |
| 19:15:46 | mriedem | when we're doing multi-create, we have 1 request spec per instance, so in this failing test we have 2 | |
| 19:15:47 | sean-k-mooney | checking if the instance uuid is already in the current host. | |
| 19:16:01 | mriedem | but the conductor just randomly picks the first request spec from the list when passing it to the scheduler, | |
| 19:16:07 | mriedem | and that request_spec has a random instance_uuid on it | |
| 19:16:33 | mriedem | so if i've got hosts A and B and instances X and Y, and using reqspec with instance X, | |
| 19:16:44 | mriedem | then go through host filtering and select host A for instance X, | |
| 19:17:03 | mriedem | when i go through host filtering for instance Y and host A, it will say: if spec_obj.instance_uuid in host_state.instances.keys(): | |
| 19:17:05 | sean-k-mooney | oh realy should we not be looping over the requests specs in the multicreate request | |
| 19:17:15 | mriedem | if X in A.instances(): return True | |
| 19:17:35 | mriedem | the problem is when this code was added https://github.com/openstack/nova/blob/master/nova/scheduler/filters/affinity_filter.py#L98 it assumes resize, which is a single instance, | |
| 19:17:57 | mriedem | and it worked because before https://review.openstack.org/#/c/571166/ the filter didn't look at host_state.instances within a multi-create create | |
| 19:18:08 | mriedem | s/look at/depend on/ | |
| 19:19:02 | mriedem | i'm going to see if i can make this a predictable order through the scheduler to tickle the failure | |
| 19:23:46 | sean-k-mooney | ya so that looks wrong https://github.com/openstack/nova/blob/039f7e055ee2b47e96be4e86d6f48d9ce469c123/nova/conductor/manager.py#L1191-L1194 | |
| 19:24:05 | sean-k-mooney | its using the request spec from the first instance only | |
| 19:24:22 | mriedem | well, it's not wrong, it's just very brittle assumptions in the anti-affinity filter that didn't used to be a problem | |
| 19:24:38 | sean-k-mooney | fair point :) | |
| 19:27:47 | sean-k-mooney | i guess you do want to use only 1 request spec so that state is preseved across instances | |
| 19:28:22 | mriedem | well, things have just sort of been hacked in over time | |
| 19:28:38 | mriedem | reqspecs are 1:1 with instances, | |
| 19:28:50 | mriedem | but we're scheduling for a list of instances with the reqspec from the first one, | |
| 19:29:01 | mriedem | so any filter relying on reqspec.instance_uuid is out of context | |
| 19:29:07 | mriedem | for a multi-create scenario anyway | |
| 19:30:36 | sean-k-mooney | i wonder are there other edgecase there also. such as multi attach volumes that this would cause issues for | |
| 19:31:45 | mriedem | we currently don't support multi-create bfv with the same multiattach volume | |
| 19:31:52 | mriedem | nor do we do any filtering on volumes | |
| 19:32:16 | mriedem | but that's #1 here https://specs.openstack.org/openstack/nova-specs/specs/rocky/approved/volume-multiattach-enhancements.html#problem-description | |
| 19:33:50 | sean-k-mooney | mriedem: ok just wondering if we will find more edgecase like this if we keep using the same request spec for all instance in the list | |
| 19:34:20 | sean-k-mooney | i could not think of one off the top of my head but multiattached volumes was the close thing that came to mind | |
| 19:35:07 | openstackgerrit | Merged openstack/nova master: Merge server create for scheduler hint extension https://review.openstack.org/579067 | |
| 19:35:27 | openstackgerrit | Merged openstack/nova master: Fix "XLibvirt KVM (ppc64)" typo in feature support matrix docs https://review.openstack.org/583267 | |
| 19:41:38 | sean-k-mooney | mriedem: we have the list of instance_uuids up untill we call self._get_sorted_hosts() here https://github.com/openstack/nova/blob/1f0e2bef296d860e6875aa148d3a86629d872256/nova/scheduler/filter_scheduler.py#L190 | |
| 19:42:10 | sean-k-mooney | mriedem: could we pass in the current instance uuid and feed it true the schduler | |
| 19:43:36 | mriedem | maybe, | |
| 19:43:40 | sean-k-mooney | we would basically have to set the sepc_obj.instance_uuid to instance_uuids[num] | |
| 19:43:45 | mriedem | i was just going to add a spec_obj.num_instances == 1 check to https://github.com/openstack/nova/blob/master/nova/scheduler/filters/affinity_filter.py#L98, | |
| 19:44:07 | mriedem | but, num_instances is persisted across the request spec so it would be re-used on a resize to same host, which would break that logic, | |
| 19:44:30 | mriedem | num_instances should probably *not* be persisted since it's per-operatoin | |
| 19:45:45 | sean-k-mooney | ya i was going to say if we did that it would break on a rezie correct because it could resize to the same host wich is presumable whtat that check is ment to prevent | |
| 19:46:14 | mriedem | right that's what it was added for https://review.openstack.org/#/c/299045/ | |
| 19:47:36 | mriedem | we do some things like reset force_hosts/force_nodes in the request spec during a move operation so it doesn't screw with the filtering | |
| 19:47:56 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/tasks/migrate.py#L184 | |
| 19:50:23 | mriedem | i remember making a similar type of fix for that recently but can't find the patch | |
| 19:52:10 | mriedem | oh it was something to do with failed hosts in the requestspec.retry | |
| 19:52:25 | mriedem | would prevent you from being able to later live migrate the instance to those hosts | |
| 19:52:58 | mriedem | https://review.openstack.org/#/c/559447/ | |
| 19:55:05 | mriedem | num_instances is a bit different in that it's set in the api, persisted to the group in the api, and then conductor reads the group back out of the db and puts it in the request spec sent to the scheduler | |
| 19:55:20 | mriedem | we could just set num_instances in conductor since we know how many instances we're creating at that point | |
| 19:55:22 | openstackgerrit | sean mooney proposed openstack/nova master: random hacking for bug 1781710 https://review.openstack.org/583347 | |
| 19:55:24 | openstack | bug 1781710 in OpenStack Compute (nova) "ServersOnMultiNodesTest.test_create_server_with_scheduler_hint_group_anti_affinity failing with "Servers are on the same host"" [High,Fix released] https://launchpad.net/bugs/1781710 - Assigned to Matt Riedemann (mriedem) | |
| 19:55:38 | sean-k-mooney | mriedem: does ^ make any sense | |
| 19:56:17 | mriedem | yeah but could make that cleaner with enumerate and resetting the instance_uuid field on the request spec so it's not persisted | |
| 19:56:31 | mriedem | i also don't know how to recreate this bug to show this fixes it | |
| 19:57:30 | sean-k-mooney | what about setting the allow server per host to 2 and only have one host? | |
| 19:57:38 | sean-k-mooney | not sure that would work actully... | |
| 19:59:17 | openstackgerrit | Merged openstack/nova master: Add unshelve instance error info to fault table https://review.openstack.org/579747 | |
| 20:02:47 | sean-k-mooney | anyway its time for me to log off for the day. ill try and think of a way to recreate tomorow. | |
| 20:03:44 | mriedem | ok i'll clean up this patch and add a test | |
| 20:03:56 | mriedem | but it'll just be unit test since i can't recreate with functional | |
| 20:20:51 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Additional/alternative fix for bug 1781710 https://review.openstack.org/583351 | |
| 20:20:52 | openstack | bug 1781710 in OpenStack Compute (nova) "ServersOnMultiNodesTest.test_create_server_with_scheduler_hint_group_anti_affinity failing with "Servers are on the same host"" [High,Fix released] https://launchpad.net/bugs/1781710 - Assigned to Matt Riedemann (mriedem) | |
| 20:33:08 | melwitt | mriedem: ack | |
| 21:29:43 | openstackgerrit | Merged openstack/nova master: Call generate_image_url only for legacy notification https://review.openstack.org/564528 | |
| 21:58:34 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Update RequestSpec.instance_uuid during scheduling https://review.openstack.org/583347 | |
| 21:58:35 | mriedem | sean-k-mooney: updated ^ with a giant commit message because this is hairy as hell | |
| 22:02:56 | mriedem | yikun: looks like we have a few more places in nova that are using InstanceGroup.policies | |
| 22:02:58 | mriedem | osboxes@osboxes:~/git/nova$ grep -R spec.instance_group.policies | |
| 22:02:58 | mriedem | nova/scheduler/utils.py: request_spec.instance_group.policies = group_info.policies | |
| 22:02:58 | mriedem | nova/scheduler/weights/affinity.py: policies = request_spec.instance_group.policies | |
| 22:25:19 | mriedem | stephenfin: check out this fun oslo.config output https://docs.openstack.org/nova/latest/configuration/config.html#upgrade-levels | |
| 22:27:10 | mriedem | the group help is dumped multiple times for lots of groups https://docs.openstack.org/nova/latest/configuration/config.html#service-user | |
| 22:28:26 | sean-k-mooney | mriedem: that a slightly more comprehsive commit message then i was expecting. | |