Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-11-17
17:52:46 openstackgerrit Chris Friesen proposed openstack/nova master: WIP: Fail fast if changing image on a volume-backed server rebuild https://review.openstack.org/520660
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.

Earlier   Later