| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-17 | |||
| 18:36:31 | dansmith | mriedem: imagepropertiesfilter is what will kick out your test instance right? | |
| 18:36:49 | mriedem | test_rebuild_with_image_novalidhost | |
| 18:36:49 | mriedem | in which test? | |
| 18:37:11 | dansmith | your new one | |
| 18:37:18 | dansmith | test_rebuild_with_new_image | |
| 18:37:23 | mriedem | in the new test, the scheduler doesn't kick it out | |
| 18:37:32 | mriedem | it asserts that it's incorrectly doubling allocations | |
| 18:37:40 | dansmith | but you're changing a thing that properties filter would kick out/ | |
| 18:37:48 | mriedem | if i maxed out inventory on the node before the rebuild, then placement would kick it out | |
| 18:38:08 | mriedem | ? | |
| 18:38:20 | mriedem | that's the test that went with the cve | |
| 18:38:43 | dansmith | right, what am I thinking. | |
| 18:39:04 | mriedem | are you expecting the new test to create, fill up compute capacity, then rebuild and fail? | |
| 18:39:14 | dansmith | no | |
| 18:39:31 | mriedem | ok - we probably want a test like that anyway, what i'm testing in the new one is something else, and equally bad | |
| 18:39:32 | dansmith | I still have mitaka on the brain, I was forgetting that master is already "fixed" | |
| 18:44:57 | dansmith | mriedem: so your test_rebuild_with_image_novalidhost passes if I remove your requested_destination= change | |
| 18:45:41 | dansmith | does that test actually have more than one compute in it? | |
| 18:45:54 | mriedem | no it doesn't | |
| 18:46:27 | mriedem | it passes because the rebuild fails yes? | |
| 18:47:04 | dansmith | well, I dunno, I'll have to look deeper | |
| 18:47:53 | dansmith | it fails if I also remove the host=None trigger | |
| 18:48:10 | dansmith | so it's failing to schedule even without the destination set | |
| 18:50:03 | dansmith | I guess that makes sense actually if there's only one host | |
| 18:50:08 | mriedem | hmm, if you remove the host=None from the API, we pass instance.host which should bypass the scheduler | |
| 18:51:03 | dansmith | right, which is just the old behavior, which means it doesn't fail and so the test fails | |
| 18:51:12 | mriedem | the test will fail if you remove host=None b/c it's asserting that rebuild raises a 500 | |
| 18:51:17 | mriedem | yeah | |
| 18:51:18 | dansmith | it's just hard to be confident that it's working when you set destination=theonlyone | |
| 18:52:07 | mriedem | yeah, we can easily spin up another compute in the test before the rebuild part | |
| 18:52:20 | dansmith | well, about that | |
| 18:52:32 | dansmith | I'm also wondering if we should have asserted that the host we got from the scheduler is the same host | |
| 18:52:48 | dansmith | because we're assuming that since we asked for the same host we either got it back or got nothing, | |
| 18:52:59 | dansmith | but if we got back something different we'd kindof half evacuate | |
| 18:54:22 | dansmith | you know, another way this breaks existing rebuilds, | |
| 18:54:29 | mriedem | so assert that once the rebuild is done, the original instance.host matches the new instance.host | |
| 18:54:47 | dansmith | is if the admin has set the compute=disabled so that no new things go to that compute, you suddenly can't rebuild anymore, which should be possible | |
| 18:54:59 | dansmith | assert in the test you mean? | |
| 18:55:03 | mriedem | yeah in the test | |
| 18:55:11 | dansmith | that makes it non-deterministic though | |
| 18:55:19 | dansmith | because sometimes we'll get back the same host by chance | |
| 18:55:33 | mriedem | if we had 2 hosts, | |
| 18:55:43 | mriedem | and used a weigher so that the scheduler would normally always pick the *other* host | |
| 18:55:53 | mriedem | then assert that the rebuild never actually moves the instance | |
| 18:56:11 | dansmith | yeah, just more complicated | |
| 18:56:31 | mriedem | also, yes, i didn't think about rebuilding on a disabled compuete | |
| 18:56:33 | mriedem | *compute | |
| 18:56:47 | mriedem | suppose you thought about that b/c of the evacuate thing | |
| 18:57:08 | dansmith | I was thinking about ways to ensure we get the other host in the test :) | |
| 18:58:16 | dansmith | actually, the default fake driver only has one vcpu, so we might always get the other one by virtue of that | |
| 18:58:51 | mriedem | SmallFakeDriver has 2 vcpus | |
| 18:58:58 | mriedem | that had to be updated b/c of our doubling up tests | |
| 19:00:00 | dansmith | I made them use the Medium one | |
| 19:00:46 | dansmith | https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L1216 | |
| 19:00:47 | dansmith | we changed the small one too? | |
| 19:02:14 | mriedem | yeah, when the tests were switched from using the chance scheduler to the filter scheduler | |
| 19:02:25 | mriedem | since filter scheduler is using placement and does the claims | |
| 19:02:50 | mriedem | I12de2e195022593ea2a3e2894f2c3b5226930d4f | |
| 19:05:32 | dansmith | hrm | |
| 19:09:03 | dansmith | hmm, seems like all weighers are enabled by default, which would mean we'd pick the other node by default | |
| 19:13:32 | mriedem | b/c packing? | |
| 19:13:51 | mriedem | we have some functional tests which provide a custom weigher so the test can control which hosts are picked | |
| 19:15:20 | mriedem | blarg, i can't change https://review.openstack.org/#/c/521153/ to use the hypervisors api to check for a doubling up of allocations using the *_used values on the compute node b/c rebuild doesn't do an RT claim | |
| 19:15:41 | mriedem | so there goes that idea | |
| 19:16:17 | dansmith | I think I'm not really starting up the other computes like I think | |
| 19:21:16 | ericyoung | I've got a review up for adding ScaleIO ephemeral volume support. | |
| 19:21:31 | ericyoung | it needs to be rebased, any idea when people might start looking at it? | |
| 19:21:33 | ericyoung | https://review.openstack.org/#/c/407440/ | |
| 19:21:46 | ericyoung | if soo, I will prioritize the rebase | |
| 19:24:20 | dansmith | mriedem: also note the use of samehost=foo will require SameHostFilter to be enabled, which isn't by default | |
| 19:26:50 | mriedem | yeah | |
| 19:28:47 | openstackgerrit | Chris Friesen proposed openstack/nova master: WIP: Fail fast if changing image on a volume-backed server rebuild https://review.openstack.org/520660 | |
| 19:31:09 | fried_rice | leakypipes Talk to me about how the resources from get_available_resource are dealt with today, vis-à-vis placement. | |
| 19:31:34 | fried_rice | Do we *just* peel out the CPU, mem, and disk, and let the rest percolate through the old-school way? | |
| 19:31:45 | leakypipes | fried_rice: depends on the virt drive. | |
| 19:31:47 | leakypipes | driver... | |
| 19:32:11 | leakypipes | fried_rice: for libvirt, yes, that's what we do. for ironic, we also handle the custom resource class if the Ironic node's resource_class attribute is set. | |
| 19:32:58 | leakypipes | fried_rice: get_available_resource() is still called for both of those drivers, though. the result of get_available_resource() is used to track NUMA and PCI device resources. | |
| 19:34:15 | fried_rice | leakypipes The ironic thing is done via get_inventory, though, nah? | |
| 19:34:41 | leakypipes | fried_rice: yes, sorry, that's what I thought you were asking about. | |
| 19:35:35 | leakypipes | fried_rice: we don't "peel anything out" of the get_available_resource() response. rather, the get_available_resource() return is simply ignored by placement/reportclient. | |
| 19:36:10 | fried_rice | leakypipes Eh? /me scours code... | |
| 19:36:31 | fried_rice | leakypipes I can easily see how one would convert from get_inventory() to update_provider_tree() -- what I'm wondering about (more long term, really) is how/when one would be able to get rid of get_available_resource() altogether. | |
| 19:36:40 | fried_rice | Sounds like this series won't be the thing that allows that to happen. | |
| 19:37:07 | leakypipes | fried_rice: no, it won't. we will need to handle NUMA stuff before that's a reality. | |
| 19:37:33 | leakypipes | fried_rice: but no *new* code is being allowed in the get_available_resource() calls... | |
| 19:39:18 | fried_rice | leakypipes Humph, indeed I can't find where get_available_resource is parlayed into placement inventory -- but then how does the scheduler work for non-get_inventory()-implementin virt drivers? | |
| 19:40:53 | openstackgerrit | Merged openstack/nova master: begin refactor AllocCandidates._get_by_filters() https://review.openstack.org/516778 | |
| 19:41:03 | fried_rice | leakypipes Woot! ^ | |
| 19:41:47 | leakypipes | :) | |
| 19:42:11 | leakypipes | fried_rice: I think all virt drivers implement get_inventory() by now. | |
| 19:42:24 | fried_rice | leakypipes Nope - PowerVM for sure not. | |
| 19:42:30 | fried_rice | in tree or out of tree. | |
| 19:43:18 | fried_rice | leakypipes Yeah, and yet when I look in the placement DB on my PowerVM compute node, I see a RP entry for the compute node and inventory for VCPU, MEMORY_MB, and DISK_GB. | |
| 19:43:22 | fried_rice | At least, I thought I did. Looking again... | |
| 19:44:07 | leakypipes | fried_rice: then there would be no resource providers in the placement/API DB. which AllocationCandidates.get_by_filters() would return [], [], which will trigger shceudler to do a ComputeNodeList.get_all() which will use the vcpus, memory_mb, and local_gb fields in the cell compute_nodes tables and those fields are populated with the get_available_resource() results. | |
| 19:44:54 | fried_rice | leakypipes And that... populates the placement DB with those values? | |
| 19:45:05 | fried_rice | Cause I confirmed | |
| 19:45:36 | fried_rice | leakypipes http://paste.openstack.org/show/626656/ | |
| 19:46:34 | fried_rice | leakypipes And no get_inventory in https://github.com/openstack/nova-powervm/blob/master/nova_powervm/virt/powervm/driver.py | |