| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-17 | |||
| 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 | |
| 19:22:29 | efried | edleafe, cdent, dansmith: y'all may want to take a look at these ^ | |
| 19:22:51 | efried | the bottom two anyway (which are intended to fix tempest on the top two) | |
| 19:24:14 | efried | bhagyashris_: FYI, hoping those bottom two patches clear up the tempest failures. | |
| 19:24:53 | efried | bhagyashris_: We were running out of hosts in tempest because we were no longer setting allocation ratios. I imagine the 16.0 allocation ratio on VCPU was what was killing us. | |
| 19:33:41 | cdent | seems sane, efried, left a query on the second one | |
| 19:34:36 | efried | cdent: Thanks. Seems like we're reworking the allocation ratio thing - but I think it still is (or can be) driven from conf and/or via the compute node record, and the virt still doesn't necessarily have access to the information. | |
| 19:34:58 | efried | cdent: I really wanted to catch Jay to get the skinny on that, but he seems to be out today. | |
| 19:35:53 | efried | cdent: As it stands, if the virt driver thinks it knows better, it can set those and _normalize... won't overwrite them. | |
| 19:35:55 | cdent | Yeah, that was the source of my confusion: given _now_, what you've got seems right | |
| 19:36:20 | efried | Glad we agree on that. | |
| 19:36:45 | efried | And I don't know what all the future is supposed to hold for this. So _now_ is about all I can account for... | |
| 19:37:22 | cdent | Indeed | |
| 19:38:19 | efried | See you in 'couver. | |
| 19:39:33 | efried | mriedem: I caught up with Gerald. He cited a customer scenario with 500 volumes on an instance. So 1024 seems like a safe and relatively future-proof number. | |
| 19:52:07 | dansmith | efried: um, I had already provided a customer scenario with 600, although no other detail, so clearly 1024 makes sense in a real-world situation | |
| 19:52:49 | efried | dansmith: Okay, I didn't follow that conversation to its end. Just got an answer so thought I would post it. | |
| 19:53:50 | dansmith | efried: how is "some customer wants to use 500" a reasonable argument? I guess my overt sarcasm wasn't overt enough | |
| 19:54:50 | efried | now you've lost me. | |
| 19:55:26 | dansmith | efried: I want to hear the *reason* for wanting to use 500 volumes on a single guest, not just that someone thinks it's a good idea | |
| 19:55:27 | efried | You mean we should require said customers to give justification that we agree with, as opposed to letting us tell them how many volumes they should be using instead? | |
| 19:55:39 | efried | yeah, got it. | |
| 19:56:18 | dansmith | the bug/bp being filed seems to indicate that someone somewhere wants to do it | |
| 19:56:20 | efried | In this case, the customer is using IBMi and has 16 FC adapters and is doing huge striping to get desired performance. | |
| 19:56:48 | dansmith | hah, I was going to say raid0ing of 1GB volumes | |
| 20:07:09 | openstackgerrit | Merged openstack/nova master: Deduplicate config/policy reference docs from main index https://review.openstack.org/564846 | |
| 20:09:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Trim the fat on HostState.instances https://review.openstack.org/569247 | |
| 20:10:11 | mriedem | i thorougly enjoy what you have to say for this new config option if you enable it | |
| 20:10:15 | mriedem | ^ | |
| 20:10:25 | cdent | striping across 500 volumes sounds kind of fun. Like: whatever they are doing, they must think it is pretty serious. Thus: fun. | |
| 20:11:21 | dansmith | more fun for the operator that gave a tenant 500 volume quota and realized they attached them all to a single instance to hack around performance limits | |
| 20:14:16 | efried | mriedem: Fast-approved | |
| 20:14:36 | mriedem | ha | |
| 20:17:09 | tssurya | mriedem: sorry was afk, but thanks for fixing the nits in both the patches | |
| 20:19:40 | dansmith | mriedem: you really want to provide a workaround knob for that? | |
| 20:20:36 | mriedem | dansmith: as in just let it break? | |
| 20:20:41 | dansmith | yeah | |
| 20:20:50 | dansmith | I mean, I appreciate the care, but.. | |
| 20:21:00 | mriedem | i didn't see any HostInstance.instances usage in the huawei product code but... | |
| 20:21:17 | dansmith | also, | |
| 20:21:53 | dansmith | if we change how we do that query, we could populate the id field, which would make it actually lazy-load the things they want | |
| 20:21:54 | dansmith | right now it will fail with | |
| 20:21:57 | dansmith | "orphaned object" errors | |
| 20:22:14 | dansmith | or make instance lazy-load by uuid if it doesn'talready | |
| 20:22:39 | artom | Or just outright own the full thing and change the object to be a list of instance UUIDs? | |
| 20:22:52 | artom | Not sure how that would work for backwards compatibility tho | |
| 20:23:02 | mriedem | so first fellas, WIP clearly | |
| 20:23:04 | dansmith | oh it does load by uuid, so make the context right and it will lazy-load | |
| 20:23:16 | dansmith | mriedem: I know, I'm just asking :) | |
| 20:23:23 | mriedem | second, yeah long term i want to do away with the dict and just make HostState.instances a list of uuids | |
| 20:23:34 | artom | mriedem, too bad, you gave me an excuse to procrastinate real work. Fell the full wrath of my nitpicking. | |
| 20:23:42 | mriedem | dansmith: so {uuid: objects.Instance(cctxt, uuid=uuid)}? | |
| 20:23:49 | dansmith | mriedem: yup | |
| 20:23:51 | mriedem | i fell it real good | |
| 20:24:10 | dansmith | mriedem: then you can just not do the conf option, it'll be slow if they touch it, and we can remove it later | |
| 20:24:11 | mriedem | dansmith: yeah that's agood point | |
| 20:24:19 | mriedem | damn good point | |
| 20:24:53 | dansmith | mriedem: snarky comments still required though | |
| 20:25:05 | dansmith | # NOTE(mriedem): Set the timer on this bomb and walk out casually | |
| 20:31:35 | mriedem | dansmith: setting the context on the instance object won't lazy-load all fields | |
| 20:31:40 | mriedem | just those in INSTANCE_OPTIONAL_ATTRS | |
| 20:31:54 | dansmith | should call _load_generic, which will do anything | |
| 20:32:03 | mriedem | if attrname not in INSTANCE_OPTIONAL_ATTRS: | |
| 20:32:08 | mriedem | raise exception.ObjectActionError( | |
| 20:32:17 | dansmith | oh, heh, | |
| 20:32:31 | dansmith | I dunno why we did that | |
| 20:32:33 | mriedem | so now my filter on instance.default_ephemeral_device no longer works | |
| 20:32:34 | dansmith | because load_generic will do it | |
| 20:33:30 | dansmith | it's from 2013, nuke it | |
| 20:34:33 | dansmith | it looks like that was just trying to make it not do an empty load for something it wasn't going to be able to satisfy, | |
| 20:34:44 | dansmith | since it was trying to figure out what to put in extra_attrs | |
| 20:35:36 | dansmith | and that was before we had load_generic | |
| 20:35:52 | dansmith | so when we added the generic thing we probably just didn't remove the thing at the top to let you use it for anything | |
| 20:36:05 | dansmith | we should also have that fill in any field it gets that isn't already set, | |
| 20:36:15 | dansmith | so you don't load the instance twice for two regular fields | |
| 20:36:20 | dansmith | I can make those changes in one patch if you want | |
| 20:36:40 | mriedem | "we should also have that fill in any field it gets that isn't already set," - i believe it will do that for all non-joined fields anyway | |
| 20:36:49 | mriedem | because it just does a normal get and load from db | |
| 20:36:56 | dansmith | load_generic won't, | |
| 20:37:02 | dansmith | it will only copy over the one thing you asked for | |
| 20:37:29 | mriedem | self[attrname] = instance[attrname] | |
| 20:37:29 | mriedem | if instance.obj_attr_is_set(attrname): | |
| 20:37:29 | mriedem | oh i see | |
| 20:37:33 | dansmith | yeah | |
| 20:37:33 | openstackgerrit | Merged openstack/nova master: Cleanup ugly stub in TestLocalDeleteAllocations https://review.openstack.org/561997 | |
| 20:37:35 | dansmith | I mean, | |