| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-21 | |||
| 20:39:32 | jaypipes | mriedem: which is kinda what I said in the original review.. | |
| 20:39:57 | jaypipes | mriedem: but the alternative is to unpack the JSON in the Selection.allocation_request field blobified goop. | |
| 20:40:31 | mriedem | that won't work, | |
| 20:40:39 | mriedem | oh wait | |
| 20:40:47 | mriedem | maybe it will because that has the resource provider uuid in it right? | |
| 20:41:03 | mriedem | it's just a json version of https://developer.openstack.org/api-ref/placement/#update-allocations | |
| 20:41:22 | mriedem | yeah dude that's way faster than hitting placement for each instance | |
| 20:41:38 | mriedem | well, *should be* faster | |
| 20:44:53 | mriedem | that's also potentially the same Selection object (allocation_request) for multiple servers in the same build operation | |
| 20:45:19 | mriedem | so deserialize once rather than GET /allocations/{consumer_id} x number of instances | |
| 20:47:38 | jaypipes | mriedem: ack. | |
| 20:48:16 | mriedem | we can follow up also and avoid that GET /resource_provider/{rp_id}/traits call yeah? by stashing the GET /a_c provider_summaries traits per provider on the Selection object we return from the scheduler | |
| 20:48:17 | jaypipes | mriedem: which violates our original intention that the allocation_request would be a BLOBby thing that could be sent as-is as the request payload to placement on the event of a reschedule/retry operation] | |
| 20:48:20 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: De-shim PlacementFixture https://review.openstack.org/637959 | |
| 20:48:39 | jaypipes | mriedem: but as you say, it's considerably faster than re-querying placement for each instance | |
| 20:48:47 | mriedem | i'm not sure how it violates that | |
| 20:49:16 | mriedem | for the first selection in the list, it's allocatoin request has already been 'consumed' | |
| 20:49:18 | jaypipes | mriedem: well, it violates the principle that the allocation_request's structure could change and nothing in nova would need to be changed. | |
| 20:49:26 | jaypipes | mriedem: yes, understood. | |
| 20:49:43 | jaypipes | again, this was the original thought... things have changed/softened in our thinking since then | |
| 20:49:44 | jaypipes | :) | |
| 20:49:53 | mriedem | i'm not sure what you're saying :) | |
| 20:50:11 | mriedem | it's still a string on the selection object, in this code we just deserialize that field to get the rp uuids | |
| 20:50:22 | mriedem | well, the actual allocations i guess | |
| 20:50:28 | jaypipes | it's not just the rp uuids. | |
| 20:50:33 | jaypipes | yeah, it's all the allocation info | |
| 20:50:47 | mriedem | i guess what you're saying is we'd be treating the allocatoin request like a response | |
| 20:50:49 | mriedem | when it's not | |
| 20:51:04 | jaypipes | and of course, if we change the structure of the allocation_request response, boom, that "deserialize this blob and inspect it" code would of course need to change | |
| 20:51:11 | jaypipes | mriedem: yes. | |
| 20:51:37 | jaypipes | mriedem: I'm just trying to give you some background on why that's a JSON string and not an object :) | |
| 20:51:54 | mriedem | yeah i remember | |
| 20:52:00 | mriedem | and why we also provider the version | |
| 20:52:01 | jaypipes | ok, I will go away now | |
| 20:52:03 | mriedem | *provide | |
| 20:52:07 | mriedem | don't go away jay | |
| 20:52:09 | mriedem | please stay | |
| 20:52:09 | jaypipes | :P | |
| 20:52:29 | openstackgerrit | Merged openstack/nova stable/queens: Fix a missing policy in test policy data https://review.openstack.org/637112 | |
| 20:52:37 | openstackgerrit | Merged openstack/nova stable/rocky: tox: Don't write byte code (maybe) https://review.openstack.org/636917 | |
| 20:55:10 | mriedem | so maybe what i'll do is pull this down, put in the very easy "if the request spec doesn't have requested_resources, don't do anything with this new code" thing, leave a bunch of todos for gibi to follow up | |
| 20:55:24 | mriedem | that solves 99% of my performance worry | |
| 20:55:44 | mriedem | jaypipes: ^ how do you like that? | |
| 20:57:01 | efried | mriedem: That sounds like a pretty reasonable solution to me. And you would be doing it rather than letting gibi do it because... you want to get it merged today? | |
| 20:57:14 | mriedem | yes | |
| 20:57:21 | mriedem | time is as they say of the essence | |
| 20:57:44 | jaypipes | mriedem: ✔ | |
| 20:58:34 | efried | makes me a little nervous for the author of the patch not to have a nod on a change of that magnitude, logic-wise. | |
| 20:58:39 | efried | I guess worst case it can be reverted | |
| 20:58:47 | efried | but then again why couldn't it be done in a fup anyway? | |
| 21:00:03 | mriedem | i can be, but i just really don't want to get close to freeze with this like it is | |
| 21:00:21 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Remove _legacy_dict methods https://review.openstack.org/636210 | |
| 21:00:21 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Add migration param to check_can_live_migrate_destination https://review.openstack.org/634605 | |
| 21:00:22 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Introduce live_migration_claim() https://review.openstack.org/635669 | |
| 21:00:22 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Use live_migration_claim() to check dest resources https://review.openstack.org/634606 | |
| 21:00:23 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: New objects to transmit NUMA config from dest to source https://review.openstack.org/634827 | |
| 21:00:23 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: LM: Make dest send NUMAMigrateData to the source https://review.openstack.org/634828 | |
| 21:00:24 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: LM: update NUMA-related XML on the source https://review.openstack.org/635229 | |
| 21:00:53 | mriedem | btw the change i'm talking about is 1 line | |
| 21:01:13 | mriedem | do this https://review.openstack.org/#/c/616239/33/nova/objects/request_spec.py@781 | |
| 21:01:30 | mriedem | at the beginning of https://review.openstack.org/#/c/619528/27/nova/conductor/manager.py@1248 | |
| 21:02:03 | artom | efried, mriedem, hey, so I know it's late in the cycle, and I'd be trampling all over the runways process, but what are the chances of pushing for reviews/merge of ^^ before FF? | |
| 21:02:23 | jaypipes | mriedem: that is 2 lines Matt. Un. Friggin. Acceptable. | |
| 21:02:59 | mriedem | artom: there are more cores here than me | |
| 21:03:19 | mriedem | artom: at this point i'm pushing to get the bw provider stuff in before FF | |
| 21:03:25 | artom | mriedem, I know. You reviewed the spec is why. But I get that you're oversubscribed as is. | |
| 21:03:44 | sean-k-mooney | jaypipes: he did say about 1 line :P | |
| 21:04:07 | artom | Which is why I mentioned efried :) Intel are apparently keep on it. | |
| 21:04:51 | mriedem | artom: what's the test story on that series? is https://review.openstack.org/#/c/634606/24/nova/tests/functional/compute/test_live_migration.py the most functional we have? | |
| 21:05:02 | artom | mriedem, in-tree, yeah | |
| 21:05:20 | artom | I wrote https://review.rdoproject.org/r/#/c/18832/ that I've been using in my devstack | |
| 21:05:24 | efried | artom: I have been requested both internally and externally to have a look at that. Unfortunately, it's behind a number of other things, as well as requiring quite a bit of homework on my part just to grok the background. I think alex_xu is involved though, right? | |
| 21:05:51 | jaypipes | sean-k-mooney: :P | |
| 21:05:51 | artom | efried, not really, you're our only hope (Obi Wan) | |
| 21:06:51 | artom | So, it's easy for me to get stephenfin to look at it, being the same company and all | |
| 21:06:57 | sean-k-mooney | artom: the code freeze is still a few weeks away. we shoudl be able to get to it once bw feature is in | |
| 21:07:02 | artom | It's the other non-RH core that's the problem | |
| 21:07:16 | mriedem | i also have like a 50 change series of my own | |
| 21:07:34 | artom | ... and who groks NUMA? jaypipes I guess? But you're as oversubscribed as mriedem. | |
| 21:08:00 | mriedem | i definitely know how the live migration flow in compute manager and claims shit works, so i should look at it | |
| 21:08:05 | mriedem | but god | |
| 21:08:25 | artom | To be fair, there's very little actual NUMA, bizarely | |
| 21:08:32 | artom | In the sense that it uses a lot of existing code | |
| 21:08:40 | artom | So if you assume those bits works, it's just about the glue | |
| 21:09:08 | sean-k-mooney | artom: that because your useing the existing code to calulate the topology on the destination then just sending it back to the source and updating the xml | |
| 21:09:20 | artom | Yep, what sean-k-mooney said | |
| 21:09:22 | mriedem | if it's ready for review just put it in runways | |
| 21:09:30 | sean-k-mooney | so most of that code is common | |
| 21:09:38 | mriedem | you can have other RH cores review it and do the honorable and not +W | |
| 21:09:43 | mriedem | like dansmith and melwitt | |
| 21:10:19 | artom | mriedem, yep :) | |
| 21:10:32 | artom | Was trying to gauge interest/chances. So not a slam dunk, to put it mildly ;) | |
| 21:10:57 | artom | ;) | |
| 21:11:07 | mriedem | sorry | |
| 21:11:18 | mriedem | i'll get to what i can when i can | |
| 21:11:21 | melwitt | yes, I should help review that. currently toiling on my own patch series for counting quota usage from placement | |
| 21:11:28 | artom | mriedem, heh, no need to apologise :) | |
| 21:12:41 | sean-k-mooney | artom: has cfriesen reviewed the code by the way. he will be interested in it. im setting up at test env currently and will fully re review it proably early next week | |
| 21:13:31 | artom | sean-k-mooney, I poked him a while ago, when it was still all WIP | |
| 21:13:43 | cfriesen | I'm totally swamped with a downstream cust issue | |