| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-17 | |||
| 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, | |
| 20:37:41 | dansmith | it nicely punishes them hard, | |
| 20:37:45 | dansmith | but probably not the best thing to do :P | |
| 20:38:03 | mriedem | i just sent to the ML btw | |
| 20:38:34 | mriedem | efried: http://lists.openstack.org/pipermail/openstack-dev/2018-May/130624.html | |
| 20:38:47 | efried | mriedem: ack | |
| 20:39:25 | mriedem | also related, we probably no longer need to default join on security_groups since that would only ever be populated for nova-network | |
| 20:40:43 | mriedem | dansmith: if you want, what i do need right now is to handle the NOTE in InstanceList.v | |
| 20:40:44 | mriedem | *get_uuids_by_host | |
| 20:41:28 | dansmith | hah | |
| 20:41:33 | dansmith | yeah I can do that too | |
| 20:41:54 | dansmith | there has to be some animated gif that applies well to that | |
| 20:42:15 | mriedem | i've been meaning to do that since https://review.openstack.org/#/c/563234/1/nova/api/openstack/compute/services.py@222 | |
| 20:42:42 | dansmith | ooh, yessss, I made unit tests fail with infinite recursion | |
| 20:47:10 | dansmith | heh, instance will trigger a double lazy load on itself apparently, which I uncovered by removing this check | |
| 20:47:13 | dansmith | that's nice | |
| 20:48:02 | mriedem | you also have a note in that exact code | |
| 20:48:05 | mriedem | about recursion | |
| 20:48:20 | edmondsw | any stable cores around to review https://review.openstack.org/#/c/567599 ? | |
| 20:48:33 | dansmith | I know | |
| 20:49:11 | edmondsw | been out there a while, and it's an easy review | |
| 20:56:26 | openstackgerrit | Merged openstack/nova master: Remove mox in test_xenapi.py (3) https://review.openstack.org/564645 | |
| 21:11:53 | dansmith | mriedem: funny thing happened on the way to the forum: https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L2596-L2607 | |
| 21:12:40 | mriedem | edmondsw: easy for me to -1 | |
| 21:14:06 | mriedem | ha | |
| 21:14:13 | edmondsw | esberglu ^ | |
| 21:14:27 | mriedem | dansmith: one difference with that, | |
| 21:14:33 | mriedem | the project_only=False kwarg to model_query | |
| 21:14:42 | dansmith | aww | |