| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-07 | |||
| 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. | |
| 19:55:43 | cdent | that sounds right | |
| 19:56:30 | mriedem | ok, +W the | |
| 19:56:31 | mriedem | *then | |
| 19:56:33 | cdent | i agree we’ve not been careful about versions, but I’m not sure it is a huge problem in reality | |
| 19:56:40 | melwitt | mriedem: FYI I added some test coverage here and stacked the remove old quotas code follow up on top https://review.openstack.org/#/c/526270 | |
| 20:00:56 | mriedem | melwitt: comments inline | |
| 20:02:46 | jaypipes | mriedem: reviewed https://review.openstack.org/#/c/525787/. +Wd | |
| 20:03:18 | melwitt | mriedem: I didn't have access to self.flags in fixtures.py, I assumed it's on the base TestCase class. but let me check | |
| 20:04:22 | mriedem | melwitt: you likely have to pass the test's self into the fixture | |
| 20:04:27 | mriedem | like we do in some other fixtures | |
| 20:04:37 | mriedem | i care less about the self.flags thing | |
| 20:04:46 | mriedem | and more about doing the cleanup after the thing you change, and removing the unused CONF in the sample test | |
| 20:04:54 | melwitt | oh, got it. I can do that then | |
| 20:05:04 | mriedem | jaypipes: thanks, replied about the setup thing | |
| 20:05:11 | melwitt | k | |
| 20:08:09 | mriedem | jaypipes: btw, i haven't dealt with that much mox in a long time... | |
| 20:08:29 | mriedem | the strictness with which mox makes sure you hit bdm.save() cost me about 2 hours | |
| 20:08:58 | jaypipes | mriedem: I know, right? :) | |
| 20:09:54 | cdent | good night | |
| 20:12:56 | mriedem | jaypipes: efried: want me to do the doc and nova-status stuff from https://review.openstack.org/#/c/385693/ ? | |