| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-21 | |||
| 15:01:16 | bauzas | mriedem: It's just a long day that I haven't provided a new release | |
| 15:01:20 | bauzas | meh | |
| 15:01:25 | mriedem | psh | |
| 15:01:25 | jaypipes | efried: k. it's gonna be low priority though :) | |
| 15:01:28 | mriedem | excuses excuses | |
| 15:01:52 | cdent | efried: noted | |
| 15:02:05 | bauzas | mriedem: no excuses, I'm a d-bag, that's it | |
| 15:02:21 | stephenfin | bauzas: Well we didn't want to say anything, but... | |
| 15:02:53 | efried | cdent jaypipes Would I be correct in assuming that we get a significant performance improvement by reducing the number of placement API calls, assuming everything else (amount of db work, etc.) remains the same? | |
| 15:03:14 | bauzas | there, fixed | |
| 15:03:19 | jaypipes | efried: for the nova-computes running Ironic virt driver, yeah. | |
| 15:03:41 | jaypipes | efried: for the KVM ones, probably won't notice much of a difference. | |
| 15:03:42 | cdent | efried: performance _where_ ? | |
| 15:04:38 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Transform missing delete notifications https://review.openstack.org/410297 | |
| 15:04:42 | efried | jaypipes cdent I'm talking about just placement calls. The above change set allows us to reduce 3N+1 calls down to one, to get the same data. (N is the number of providers in the tree.) | |
| 15:04:46 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Send soft_delete from context manager https://review.openstack.org/476459 | |
| 15:05:18 | efried | jaypipes cdent The data as a whole is identical, the work being done on the placement side is identical, etc. - just being done in one API call instead of many. | |
| 15:05:30 | cdent | efried: you’re limiting the opportunities for the placement service to parallize across multiple instances of placement itself, which may or may not be a concern. I tend to prefer lots of smaller calls on many servers | |
| 15:05:39 | jaypipes | efried: understood. and what I'm saying is that N is not a large number for KVM. For Ironic (and maybe powervm) N can be a large number. | |
| 15:05:49 | openstackgerrit | Merged openstack/os-traits master: Update the documentation urls https://review.openstack.org/501122 | |
| 15:06:16 | efried | jaypipes Ah, I understand. Nevertheless, even for a one-node tree, it would be taking 4 calls down to 1. | |
| 15:06:29 | efried | jaypipes I guess I'm asking if the overhead of the API calls is significant. | |
| 15:06:29 | jaypipes | efried: understood. :) | |
| 15:07:31 | cdent | efried: if it is possible to measure instead of speculate that would be great, but my speculation would be that http overhead is not the limiting factor in the big picture in these situations | |
| 15:08:05 | cdent | certainly as a micro optimization, at webscale™, http request overhead matters | |
| 15:08:25 | efried | cdent Okay, interesting. Well, if for no other reason than it makes the client code easier... | |
| 15:08:32 | cdent | but only if it cannot be ameliorated by parallelization | |
| 15:09:30 | cdent | my gut reaction is that your prematurely optimizing, and that your efforts would be better served by trying to break stuff | |
| 15:09:41 | cdent | so that when we fix stuff, we’re fixing the right stuff | |
| 15:09:55 | cdent | but I’m sure telling grandma how to suck eggs | |
| 15:10:12 | cdent | efried: Are you familiar with that phrase? I didn’t know it when I first heard it and it totally confused me. | |
| 15:10:39 | efried | cdent No, but google helped me out. | |
| 15:10:58 | efried | cdent In the context of the SchedulerReportClient, parallelization would entail some nontrivial rework, likely including spawing extra threads, which doesn't seem likely. | |
| 15:11:26 | cdent | no, I mean parallization at the global level, not the per “query” level | |
| 15:11:58 | cdent | we are more concerned with the performance of placement being good and predictable for everyone, at large | |
| 15:12:15 | cdent | and given N is almost always quite small... | |
| 15:13:06 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 15:13:07 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: libvirt: Allow multiple volume attachments https://review.openstack.org/267587 | |
| 15:13:08 | efried | cdent Though even at its smallest, we would be reducing the number of calls by 4x | |
| 15:13:35 | cdent | I hear you, but as we have no evidence, yet, that it matters, it’s early optimization, right? | |
| 15:14:01 | efried | cdent If I was doing it just for performance, yeah. But I have *hard* evidence that it's gonna make my coding easier :) | |
| 15:14:23 | cdent | while making the server more complex | |
| 15:15:00 | efried | cdent See this series: https://review.openstack.org/#/c/521605/ | |
| 15:16:28 | efried | cdent But also https://review.openstack.org/#/c/521098/4/nova/scheduler/client/report.py as we discussed yesterday. | |
| 15:17:20 | efried | Anyway, I did happen to notice a dearth of functional tests in test_report_client, so maybe I will do as you suggest and go back to breaking things. | |
| 15:18:51 | cdent | efried: I’m not against your idea at all, I just don’t think it is urgent in the face of the usual constraints about review bandwidth, getting other stuff merged in this cycle, etc etc. I’d love it if we operated on a “this is a good idea, let’s just do it” mode but we’re not quite there. | |
| 15:20:09 | efried | cdent I'm with you. Though TBH I'm not sure the amount of code will be more one way or the other. We either collate on the server side or the client side. | |
| 15:21:06 | cdent | I’m biased to doing collation on the client side because I want the server to be as simple as possible, and will consistently resist adding new stuff, until overwhelmed and then blithely acquiese | |
| 15:25:49 | openstackgerrit | Merged openstack/nova master: placement: Document request headers in api-ref https://review.openstack.org/521563 | |
| 15:26:00 | openstackgerrit | Merged openstack/nova master: placement: Document `in:` prefix for ?member_of= https://review.openstack.org/521216 | |
| 15:26:10 | artom | dansmith, mriedem, is it worth it to "argue" about which filters should be policy at this point? | |
| 15:26:34 | dansmith | artom: if you see something glaringly wrong, point it out | |
| 15:26:40 | artom | I see there are many comments to that effect already, and I haven't read all of them yet | |
| 15:27:18 | artom | dansmith, I wouldn't say glaringly. To me it's "does policy == things can change through a rebuild"? | |
| 15:27:26 | artom | Or rather, is that what we want? | |
| 15:27:51 | artom | Or are we going with the "real" semantic definition of policy, even it it means running some filters needlessly? | |
| 15:28:11 | efried | cdent Are you aware of a bug complaining that GET / of the placement API should be unauthenticated? | |
| 15:28:43 | dansmith | artom: well, as we discussed on that review, it's hard to draw a neat box around that | |
| 15:28:53 | efried | cdent AFAIK, we want every service to permit version discovery (which is what's at that URI) for free. <== mordred True? | |
| 15:29:19 | artom | dansmith, yeah, which is why I'm asking here before leaving a bunch of somments | |
| 15:29:21 | artom | *comments | |
| 15:29:21 | dansmith | artom: I want it to be "affects _where_ a thing should land" vs. "affects whether a host is "allowed" or "can" land" or something | |
| 15:29:34 | dansmith | artom: but clearly for the purposes of this backport, it's mostly just "does this break rebuild" | |
| 15:29:46 | artom | For instance, the AZ filter... the AZ can't change with a rebuild, can it? Do we need to run it? | |
| 15:30:03 | dansmith | artom: and I'm not running it on rebuild right? | |
| 15:30:18 | dansmith | oh I am | |
| 15:30:46 | dansmith | that might be a mistake.. I was thinking that certain aggregate lines could have changed such that the operator wants to block the rebuild, | |
| 15:30:51 | dansmith | but az is probably not right | |
| 15:31:14 | dansmith | artom: I was thinking https://review.openstack.org/#/c/521186/5/nova/scheduler/filters/aggregate_instance_extra_specs.py so I might have confused the az one | |
| 15:31:14 | mriedem | heh, maybe the variable should just be, RUN_ON_REBUILD | |
| 15:31:18 | mordred | efried: yes - we want all the things to be able to do version discovery - lemme read more scrollback though | |
| 15:31:25 | artom | The aggregate stuff as well... | |
| 15:31:38 | efried | mordred Just the comment two up from that one | |
| 15:31:44 | artom | I grok that aggregate metadata can be changed by an operator | |
| 15:31:54 | efried | mordred Today placement's GET / requires auth | |
| 15:32:08 | artom | But nothing about the instance can change to affect its aggregate placement, I think? | |
| 15:32:16 | dansmith | artom: so we have a test that makes sure that rebuild fails if the operator has manually migrated one of your anti-affinity brothers to your same host.. I figure that means that late violation of such a policy should make rebuild fail | |
| 15:32:23 | mriedem | artom: the image can change on rebuild | |
| 15:32:39 | mriedem | so the image meta can change the host aggregates you can be in | |
| 15:32:43 | mriedem | which was the original bug | |
| 15:32:48 | artom | mriedem, ah, does that specify aggregate stuff? OK, that's ignorance on my part then | |
| 15:33:55 | mriedem | efried: didn't you have a devstack patch at one point that depended on https://review.openstack.org/#/c/490057/ and removed the [glance] auth options from nova.conf to see it passing? | |
| 15:34:04 | efried | mriedem one sec... | |
| 15:34:06 | mriedem | artom: good luck | |
| 15:34:24 | mriedem | oh wait, nevermind, we codify image meta | |
| 15:34:30 | mriedem | nova.objects.image_meta | |
| 15:34:34 | dansmith | yeah it's in there | |
| 15:34:45 | mriedem | was thinking extra specs | |
| 15:35:06 | artom | Those are flavour though... (right?) | |
| 15:35:07 | efried | mriedem https://review.openstack.org/#/c/490031/ (It was sdague's) | |
| 15:35:08 | mordred | efried: yes. please make to all version discovery unauthenticated | |
| 15:35:10 | dansmith | however, I think the point is that you can put extra_specs on the aggregate that looks at image properties to keep things in place | |
| 15:35:13 | artom | And flavour can't change with a rebuild... | |
| 15:35:25 | mriedem | artom: correct, | |
| 15:35:28 | mriedem | flavor can only change on resize | |
| 15:35:33 | mriedem | and we run through the scheduler on resize already | |
| 15:35:34 | dansmith | artom: that's the point of the original bug, you might have landed on a linux host, rebuild to a windows image, and blow a license for the operator | |
| 15:37:10 | sdague | efried: those test results current? if so we can get that moved in | |
| 15:37:26 | efried | sdague They're ~6w old. I'll recheck. | |
| 15:37:34 | sdague | efried: cool | |