| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-07 | |||
| 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 cache headers to placement api requests https://review.openstack.org/521640 | |
| 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: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. | |
| 19:52:10 | mriedem | yes, ^ requires 1.14 | |
| 19:52:12 | mriedem | my point is, | |
| 19:52:24 | mriedem | was anything in nova before this change using something higher than 1.10? | |
| 19:52:29 | mriedem | because if not, grenade was doing it's job | |
| 19:52:30 | efried | Oh, I thought it was the patch before that one. Never mind, you're right. | |
| 19:52:59 | efried | Does that mean cdent's change was incorrect? | |
| 19:53:01 | mriedem | we are essentially side stepping any form of version discovery still with placement and doing the ironic thing and saying you just have to have external services upgraded first, period | |
| 19:53:13 | mriedem | not necessarily, | |
| 19:53:21 | mriedem | we'll need grenade upgrading things for us to have sane CI | |
| 19:53:32 | mriedem | e.g. queens nova doesn't test against pike cinder | |
| 19:54:11 | efried | mriedem So what are our actual options here, since we don't yet know what the minimum microversion will be? We create the reno with 1.14 and just remember to bump it with each patch that uses something higher? | |
| 19:54:16 | mriedem | i think my point is just we aren't doing a good job about being careful with versions | |
| 19:54:34 | mriedem | unlike we do with other external services | |
| 19:54:57 | mriedem | regarding a release note, i said i think that's premature right now | |
| 19:55:04 | mriedem | since it's likely to bump again before we release queens | |
| 19:55:15 | mriedem | so https://github.com/openstack/nova/blob/master/nova/cmd/status.py#L202 needs to change in a follow up | |
| 19:55:26 | mriedem | and we should start working on Queens notes for https://docs.openstack.org/nova/latest/user/placement.html#upgrade-notes, in a follow up | |
| 19:55:28 | mriedem | agreed? | |
| 19:55:42 | jaypipes | works for me. | |