| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-17 | |||
| 17:54:43 | alaski | \quit | |
| 17:54:46 | alaski | \quit | |
| 17:54:54 | alaski | grr | |
| 17:54:59 | dansmith | cover. blown. | |
| 18:10:33 | leakypipes | mriedem: bottom five patches in validate_provider_summaries series are signed off by alex now, starting here: https://review.openstack.org/#/c/516778 | |
| 18:19:41 | mriedem | dansmith: ^ | |
| 18:19:41 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add regression test for rebuild with new image doubling allocations https://review.openstack.org/521153 | |
| 18:20:01 | mriedem | gonna work on the todo in there to not use placement for getting the usage | |
| 18:20:28 | dansmith | mriedem: you wanna I work on the actual fix then? | |
| 18:20:42 | mriedem | if you have a pretty clear idea on how to start it, sure | |
| 18:20:59 | dansmith | I don't have pretty clear anything these days, but I'll get it rolling | |
| 18:21:12 | dansmith | mriedem: you specifically don't want to revert and re-fix then? | |
| 18:21:23 | mriedem | idk | |
| 18:21:36 | dansmith | for my backports it'd be much cleaner if we reverted, | |
| 18:21:36 | mriedem | also, bypassing filters is one thing, but we'd also have to apply that to bypassing placement | |
| 18:21:51 | dansmith | but I understand the optics of reverting a cve | |
| 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. | |