| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-07 | |||
| 17:11:42 | bauzas | efried: ok, gtk | |
| 17:11:55 | bauzas | efried: so the driver will know the tree, all good with me | |
| 17:12:43 | efried | bauzas Well, yes, the driver will mostly be responsible for *creating* the tree. What it receives initially will probably just be the compute node provider (and maybe its traits). | |
| 17:12:56 | bauzas | k | |
| 17:13:21 | efried | bauzas At that point it'll be responsible for creating nested RPs (e.g. numa nodes, SR-IOV PFs, etc.) and possibly even sharing RPs; and setting up traits & aggregate associations. | |
| 17:14:32 | bauzas | yeah I remember now that convo | |
| 17:18:53 | efried | jaypipes cdent Here's a wrinkle: Can sharing RPs be trees? | |
| 17:19:02 | efried | Hint: it'd be a lot cooler if they couldn't. | |
| 17:19:20 | efried | I mean, it's clear that nothing is preventing you from doing that today. | |
| 17:19:27 | efried | I'm saying we should consider whether that's a good idea. | |
| 17:23:33 | mriedem | cdent: do you think https://review.openstack.org/#/c/518223/ is something we need to backport to pike? | |
| 17:24:16 | cdent | mriedem: is a nice to have but not a need to have | |
| 17:25:27 | cdent | efried: I think you and I have different mental models of rp trees. I think it should be theoretically fine for an rp which _happens_ to be part of a tree to be shareable but that it is in a tree shouldn’t be particularly meaningful | |
| 17:25:48 | efried | cdent Okay, yeah, I think I'm discovering that. | |
| 17:26:16 | efried | cdent This is coming from me trying to make sure the ProviderTree is fully populated before we hand it off to the compute driver. | |
| 17:26:25 | cdent | what tree-ness would mean in that context is a path to get to | |
| 17:26:59 | efried | I have to spider out from the compute RP's tree, grab all the RPs associated via aggregate, and populate the ProviderTree with those guys. | |
| 17:27:16 | mriedem | cdent: ok | |
| 17:27:23 | efried | So I was wondering what happens if one of those sharing RPs happens to be a child - do I *also* have to grab his whole tree and populate the ProviderTree with *those* guys. | |
| 17:27:39 | efried | The answer I'm coming to is: No, I just pretend he's a root. | |
| 17:27:45 | cdent | yes | |
| 17:28:12 | cdent | at least at a first pass. it feels like you are working a bit ahead of yourself. solve the problem you have in front of your face, not all the problems you can imagine? | |
| 17:28:13 | efried | So I may end up with multiple agg-associated sharing RPs that happen to be in the same tree, but they'll appear flattened as roots in the ProviderTree. | |
| 17:28:40 | efried | cdent I agree with that in principle, but I have to code *something* here. | |
| 17:29:22 | cdent | efried: I assume you saw my comments on your commit messages needing to say more about why you are coding these *somethings*? | |
| 17:30:14 | efried | cdent One of them, which I updated - see if it satisfies: https://review.openstack.org/#/c/521097/ | |
| 17:31:36 | cdent | efried: is there a spec or something where the reasons for the reason you gave (ComputeDriver.update_provider_tree) are explained? | |
| 17:31:55 | efried | jaypipes ^? | |
| 17:32:24 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: [placement] Fix an error message in API validation https://review.openstack.org/526465 | |
| 17:33:03 | cdent | efried: I mean, I’m willing to take it as good simply because you guys are committed to it, but it easier to revie when I have a bigger picture | |
| 17:33:28 | efried | cdent I agree it would be neat to have a spec that calls out ComputeDriver.update_provider_tree. | |
| 17:34:00 | efried | cdent Does the WIP patch that introduces that method help at all? https://review.openstack.org/#/c/521187/8/nova/virt/driver.py | |
| 17:34:46 | efried | not much, I guess. | |
| 17:36:08 | cdent | The “supersedes get_inventory()” is probably the most meaningful bit | |
| 17:36:57 | cdent | and interesting in the sense that for most of the virt drivers we’ll introduce and then supersede get_inventory in the same cycle (if lucky, subsequent otherwise) | |
| 17:37:43 | efried | cdent Yeah, I think PowerVM will get to skip get_inventory() | |
| 17:37:55 | efried | but it'll be the exception. | |
| 17:38:04 | efried | Meh, actually I don't know. | |
| 17:38:33 | efried | We should have update_provider_tree ready to go this cycle; any virt drivers that don't already have get_inventory() wouldn't *need* to implement it. Though I suppose they may choose to. | |
| 17:39:51 | jaypipes | efried: haven't thought about that, frankly | |
| 17:40:27 | efried | jaypipes I think cdent and I talked ourselves into relative sanity on the subject. | |
| 17:41:53 | mriedem | melwitt: https://review.openstack.org/#/c/526182/ and the one above it have +2s from gibi | |
| 17:41:58 | mriedem | for the new volume attach stuff | |
| 17:53:54 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Fix NoneType error when [service_user] is misconfigured https://review.openstack.org/526468 | |
| 18:43:05 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Enable limiting GET /allocation_candidates https://review.openstack.org/513526 | |
| 18:43:15 | cdent | one down, one to go | |
| 18:43:43 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Only log not correcting allocation once per period https://review.openstack.org/526477 | |
| 18:58:52 | efried | jaypipes cdent edleafe dansmith I'm about to go through SchedulerReportClient and make all the methods raise execptions when appropriate instead of returning None. Would you prefer I did that all in one patch, or split it into many (e.g. one per method or group of related methods)? | |
| 18:59:37 | dansmith | is there something we need to be catching that we can't without raising there? | |
| 18:59:45 | cdent | my preference would be for many: makes any booms a bit more obvious | |
| 18:59:54 | dansmith | but, I'm sure multiple patches per call would be way easier | |
| 18:59:58 | dansmith | also what cdent said | |
| 19:01:56 | edleafe | efried: yeah, smaller chunks are better | |
| 19:02:34 | efried | dansmith Well, we caught a grenade bug (it went "boom") with the last one. | |
| 19:03:35 | efried | dansmith It's not that we *couldn't* catch the errors without raising; it's that we *don't*. We're not paying attention to the returns from these methods in many cases, so the errors slide on by without anyone noticing. | |
| 19:04:13 | dansmith | well, in some cases that's intentional, where we get allocations and if none, then do a thing | |
| 19:04:26 | dansmith | just saying all those places will need to be changed. I'll reserve judgment until I see them I guess | |
| 19:06:18 | efried | dansmith Roger wilco. I think the important ones are where the caller doesn't necessarily have to do anything with the return value from the method - just invoking for the side effect. Methods like get_allocation_candidates don't fit that, so I guess it makes sense to leave 'em alone. | |
| 19:09:48 | dansmith | I just hadn't thought much about it so seeing them all will help | |
| 19:11:18 | efried | dansmith With that discussion, it's actually going to be pretty limited. So thanks for that. | |
| 19:29:31 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Object changes to support last-modified headers https://review.openstack.org/521639 | |
| 19:29:32 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Add info about last-modified to contrib docs https://review.openstack.org/526084 | |
| 19:29:32 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Add cache headers to placement api requests https://review.openstack.org/521640 | |
| 19:38:55 | jaypipes | dansmith, mriedem: https://review.openstack.org/#/c/385693/ should be ready to go. | |
| 19:39:43 | mriedem | it's placement | |
| 19:39:45 | mriedem | i'll tell you now | |
| 19:40:06 | jaypipes | :( | |
| 19:40:10 | mriedem | jaypipes: i'll trade you that for https://review.openstack.org/#/c/525787/ | |
| 19:40:42 | dansmith | I dunno what jaypipes' is, but it's not an even trade | |
| 19:40:52 | jaypipes | mriedem: sure, deal. | |
| 19:41:45 | mriedem | dansmith: is that a slight against mine or his? | |
| 19:41:56 | dansmith | mriedem: yours :) | |
| 19:42:01 | mriedem | ha | |
| 19:42:11 | mriedem | everyone loves some good block device'n | |
| 19:42:14 | efried | dansmith Yeah, don't take the trade. jaypipes' is easy. | |
| 19:42:44 | dansmith | I clicked on mriedem's link to see if it was one of my reviews he just just finished and was looking for a +W | |
| 19:42:49 | dansmith | but alas, no | |
| 19:43:00 | jaypipes | efried: not *just* BDM. BFV + BDM, otherwise known as BDSM. | |
| 19:43:05 | dansmith | haha | |
| 19:43:12 | mriedem | dansmith: you can get this easy refactor to complete the set https://review.openstack.org/#/c/526182/ | |
| 19:43:52 | dansmith | mriedem: I'll trade you for revisiting my live migration allocations one | |
| 19:44:12 | dansmith | that will even out your karma for trading jay something easy for something hard | |
| 19:44:33 | mriedem | fwiw, https://review.openstack.org/#/c/385693/ has a problem in the commit message | |
| 19:46:05 | efried | mriedem Will y'all fast-approve if I make that edit? Hate to lose gibi's +2 | |
| 19:46:19 | mriedem | i'm still reviewing | |
| 19:46:24 | mriedem | there are other....concerns | |
| 19:46:28 | efried | ight | |
| 19:46:34 | mriedem | the ... is for intended dramatic effect | |
| 19:46:46 | efried | jaypipes FYI I have this series locally, with lots of deltas, so *please* don't re-publish. | |
| 19:46:55 | jaypipes | efried: roger | |
| 19:50:20 | mriedem | efried: jaypipes: replied https://review.openstack.org/#/c/385693/ | |
| 19:50:22 | mriedem | pick your poison | |
| 19:51:07 | mriedem | it's probably premature to say in a release note what our minimum required version of placement is at this point | |
| 19:51:08 | efried | mriedem Nice. But FYI, I believe that ship already sailed. | |
| 19:51:11 | mriedem | since that's likely to change | |
| 19:51:28 | efried | That's what we discovered with that grenade bug. | |
| 19:51:46 | mriedem | so we currently say we require 1.10 | |
| 19:51:54 | mriedem | what do we actually require? | |
| 19:52:01 | mriedem | or were we just using 1.10? | |
| 19:52:02 | efried | Yuh, that's a lie at this point. I believe it's 1.14. | |