| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-17 | |||
| 17:08:40 | mriedem | but at that point, you either (1) live with it (2) fork the host manager code (3) upstream your filter | |
| 17:09:16 | dansmith | it'd be nice to get some clarification from them that this is indeed where their hotspot is (grabbing that large instance list) | |
| 17:10:38 | mriedem | yeah and they are both gone now | |
| 17:10:41 | mriedem | tired of our antics | |
| 17:11:00 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Update instance.availability_zone during live migration https://review.openstack.org/567701 | |
| 17:14:46 | melwitt | did y'all look at the bug I linked earlier? https://bugs.launchpad.net/nova/+bug/1737465 | |
| 17:14:47 | openstack | Launchpad bug 1737465 in OpenStack Compute (nova) "[cellv2] the performance issue of cellv2 when creating 500 instances concurrently" [Undecided,Confirmed] - Assigned to Jiang (jiangpf) | |
| 17:14:58 | dansmith | yeah | |
| 17:15:52 | dansmith | I'm sure it's the same thing, but I can't really make sense of most of the text in there | |
| 17:16:00 | melwitt | okay, I was thinking they're hitting the same thing in _get_instance_info and that's doing the instance list, it doesn't do anything else | |
| 17:16:41 | melwitt | yeah, it's not easy to read but they did point out it was the call to _get_instance_info where they get the slowdown | |
| 17:17:12 | dansmith | yeah, that's why I say I'm sure it's the same | |
| 17:17:29 | mriedem | "Because cellv2 have disable instance info cache." - i assume that means they disabled track_instance_changes | |
| 17:17:55 | melwitt | yeah, me too. so you're thinking there could possibly be something else in _get_instance_info that could cause a slowdown other than the instance list, when you said it would be good if they could clarify? | |
| 17:18:24 | dansmith | no, I'm just saying it would be good to clarify, since he linked at a rather high-up spot | |
| 17:18:29 | melwitt | mriedem: I think it means because the upcall is disallowed by default, yeah? | |
| 17:18:39 | melwitt | dansmith: oh, okay | |
| 17:18:45 | mriedem | melwitt: no | |
| 17:18:58 | mriedem | track_instance_changes is on by default, | |
| 17:19:07 | mriedem | if he's using devstack, then yes that upcall is disabled | |
| 17:19:21 | mriedem | https://docs.openstack.org/nova/latest/configuration/config.html#workarounds.disable_group_policy_check_upcall is not disabled by default though | |
| 17:19:30 | melwitt | but with cells v2 it won't cache it, because the upcall is disabled by default, right? oh nevermind | |
| 17:19:34 | mriedem | so out of the box brand new install that's not devstack is going to be doing the rpc blast upcall | |
| 17:19:37 | mriedem | no | |
| 17:19:48 | mriedem | has nothing to do with cells v2 | |
| 17:21:00 | melwitt | yeah, I was thinking maybe that upcall was disabled by default because of cells v2. but it's not | |
| 17:21:16 | dansmith | it just won't ever make it to the scheduler | |
| 17:21:25 | mriedem | if you're doing split MQ | |
| 17:21:41 | mriedem | i'd be kind of surprised if someone was following that setup if they didn't know what they were doing | |
| 17:21:44 | melwitt | ohh... | |
| 17:24:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Metadata-API fails to retrieve avz for instances created before Pike https://review.openstack.org/567878 | |
| 17:32:17 | dansmith | edleafe: hmm, so I'm a bit confused actually | |
| 17:45:16 | edleafe | dansmith: do tell | |
| 17:46:12 | dansmith | edleafe: something is off by one, one way or the other and I'm not sure I get it | |
| 17:46:24 | dansmith | either the test is wrong or needs some other tweak, or | |
| 17:46:26 | dansmith | something | |
| 17:46:39 | edleafe | I have a few minutes - let me look | |
| 17:46:41 | dansmith | edleafe: anyway, I've been working on it in parallel to other things and I think I've confused myself out of the way | |
| 17:46:50 | dansmith | so yeah, maybe you should look without me telling you what I'm confused about :) | |
| 17:47:15 | edleafe | I'd much rather confuse myself :) | |
| 17:56:05 | edleafe | dansmith: Looks like this needs to be adjusted, too: https://github.com/openstack/nova/blob/master/nova/scheduler/filter_scheduler.py#L367 | |
| 17:56:22 | dansmith | edleafe: yeah, I did that | |
| 17:56:37 | dansmith | and I can make the tests pass at that point, | |
| 17:56:49 | dansmith | but ...isn't that just subtracting one from A and adding it to B? | |
| 17:56:58 | edleafe | nope | |
| 17:57:06 | dansmith | well, assuming you mean changing >= to > | |
| 17:57:27 | edleafe | before your change, "num_alts" really was the total returned | |
| 17:57:36 | edleafe | not the number of alternates | |
| 17:58:06 | edleafe | change that line to: >= num_alts + 1 | |
| 17:58:27 | edleafe | the '1' is the 'selected' in 'selected_plus_alts' | |
| 17:59:17 | dansmith | ah, heh, that actually makes it click | |
| 17:59:23 | dansmith | changing it to >, which is what I did felt wrong | |
| 17:59:36 | dansmith | but leaving >= and putting the +1 makes it logically make sense to me | |
| 17:59:43 | edleafe | yeah | |
| 18:00:09 | edleafe | I'm re-running the unit tests | |
| 18:00:23 | edleafe | I can push that assuming that the tests pass | |
| 18:00:57 | dansmith | alright | |
| 18:02:21 | mriedem | melwitt: +2 on lyarwood's nova-lvm fix https://review.openstack.org/#/c/569062/ | |
| 18:07:22 | edleafe | dansmith: should that patch still be a WIP? | |
| 18:07:32 | dansmith | edleafe: nah I just WIP'd it because no tests | |
| 18:08:09 | edleafe | k | |
| 18:08:14 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fakelibvirt migrateToURI3 should provide args according to libvirt doc https://review.openstack.org/569047 | |
| 18:08:58 | openstackgerrit | Ed Leafe proposed openstack/nova master: Fix interpretation of max_attempts for scheduling alternates https://review.openstack.org/569127 | |
| 18:09:01 | edleafe | dansmith: Done ^^ | |
| 18:09:59 | melwitt | mriedem: you don't want to wait for lyarwood's test additions? | |
| 18:10:10 | dansmith | edleafe: cool | |
| 18:10:54 | mriedem | unit test? | |
| 18:10:59 | mriedem | was he going to add one? | |
| 18:11:14 | melwitt | on the review he said "nova-lvm is looking good so I'll respin with tests later today." | |
| 18:11:19 | mriedem | yeah, ok | |
| 18:11:36 | edleafe | mriedem: the tests already covered that | |
| 18:11:50 | edleafe | that's why they broke when he made the change | |
| 18:12:11 | dansmith | edleafe: well, they did, but we probably need one to make sure that we're doing the different thing | |
| 18:12:18 | dansmith | I'm not sure where that should go really though | |
| 18:13:06 | mriedem | edleafe: ? | |
| 18:13:21 | mriedem | edleafe: the lvm thing? | |
| 18:13:40 | dansmith | mriedem: oh we thought you were talking about the alternates thing | |
| 18:14:32 | dansmith | edleafe: we probably just need to validate that max_attempts=1 gets you 1 result | |
| 18:14:55 | mriedem | no i don't talk about that | |
| 18:22:15 | edleafe | dansmith: does this test satisfy? https://github.com/openstack/nova/blob/master/nova/tests/unit/scheduler/test_filter_scheduler.py#L915-L917 | |
| 18:23:52 | dansmith | edleafe: no, because the essential change is that max_attempts=1 should end up with zero alternates to filter | |
| 18:25:55 | edleafe | ok, let me add that | |
| 18:38:37 | openstackgerrit | Ed Leafe proposed openstack/nova master: Fix interpretation of max_attempts for scheduling alternates https://review.openstack.org/569127 | |
| 18:38:52 | edleafe | dansmith: ^^ now with shiny new unit test | |
| 18:42:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Avoid unnecessary joins in HostManager._get_instances_by_host https://review.openstack.org/569218 | |
| 18:44:00 | dansmith | edleafe: I forgot about how that one test has num_alternates=0 | |
| 18:44:06 | dansmith | which is kiiinda the same thing | |
| 18:44:26 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Avoid unnecessary joins in HostManager._get_instances_by_host https://review.openstack.org/569218 | |
| 18:44:38 | mriedem | dansmith: melwitt: ^ the small optimization noted earlier | |
| 18:55:21 | dansmith | edleafe: you know, actually I think what we probably want is to mock out the interface between _schedule() and _get_alternate_hosts() and make sure that num_alts out and in from those (respectively) includes the new offset | |
| 18:55:39 | dansmith | edleafe: because testing them together just hides that we made a change, which is why that new unit test is kinda forced | |
| 18:55:48 | dansmith | edleafe: I'm happy to do that if you want | |
| 18:55:58 | dansmith | ...if you agree | |
| 18:57:02 | melwitt | mriedem: ack | |
| 19:06:26 | edleafe | dansmith: go for it. I have meetings for the next two hours | |
| 19:07:37 | edleafe | dansmith: and looking at the code, it *was* working correctly, just poorly named. If we had simply changed 'num_alts' to 'num_returned', it would have continued to work, and made more sense to someone reading it | |
| 19:22:08 | openstackgerrit | Eric Fried proposed openstack/nova master: Normalize inventory from update_provider_tree https://review.openstack.org/569132 | |
| 19:22:09 | openstackgerrit | Eric Fried proposed openstack/nova master: libvirt: get_inventory => update_provider_tree https://review.openstack.org/560444 | |
| 19:22:10 | openstackgerrit | Eric Fried proposed openstack/nova master: libvirt: Don't report DISK_GB if sharing https://review.openstack.org/560459 | |
| 19:22:11 | openstackgerrit | Eric Fried proposed openstack/nova master: ProviderTree.has_inventory_changed for new fields https://review.openstack.org/569228 | |