| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-17 | |||
| 17:22:07 | mriedem | problem with that is, we could have backlevel computes, assuming there would need to be a change in the computes to handle that | |
| 17:22:13 | cfriesen | mriedem: alternately we'd have to switch to some other method of representing BFV, or else have nova-api null it out for BFV instances | |
| 17:22:23 | mriedem | but it shouldn't be a problem b/c i think we pass the new image_id down through rpc | |
| 17:24:09 | cfriesen | mriedem: just realized that _get_bdm_image_metadata() will give the image metadata, but not the image id itself. | |
| 17:26:09 | mriedem | yeah it should because of compute.utils.get_image_metadata_from_volume | |
| 17:26:30 | mriedem | oh nvm you're right | |
| 17:26:32 | mriedem | i think | |
| 17:26:43 | cfriesen | that loops over VIM_IMAGE_ATTRIBUTES and pops them | |
| 17:30:52 | cfriesen | mriedem: I think we want something like this...totally untested and partly copied from_get_bdm_image_metadata() : http://paste.openstack.org/show/626644/ | |
| 17:33:22 | cfriesen | could even move it down a bit after we calculate root_bdm and save doing it twice. | |
| 17:41:41 | cfriesen | mriedem: new version, cleaned up a bit: http://paste.openstack.org/show/626648/ | |
| 17:42:19 | mriedem | you shouldn't need "if snapshot_id:" | |
| 17:42:30 | mriedem | a bdm that is attached to an instance should always have volume_id set | |
| 17:42:42 | mriedem | if you bfv with a snapshot, nova creates a volume from the snapshot and stores the created volume_id on the bdm | |
| 17:43:10 | cfriesen | ah, okay | |
| 17:43:21 | mriedem | feel free to update my change with a co-author | |
| 17:43:25 | mriedem | i'm in the middle of something else | |
| 17:43:36 | cfriesen | sure | |
| 17:43:58 | cfriesen | should solve the other bug as well I think, by not going through the scheduler if they're the same | |
| 17:48:49 | 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:50:49 | cfriesen | crud, messed that up | |
| 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 | |