| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-09 | |||
| 12:41:34 | openstackgerrit | Jan Gutter proposed openstack/nova-specs master: Spec to implement vRouter HW offloads https://review.openstack.org/567148 | |
| 13:01:15 | mdbooth | efried: Morning! https://review.openstack.org/#/c/564017/ | |
| 13:01:51 | mdbooth | That's causing me a problem, and I considering proposing a revert. However, I think it only exists due to bad naming. | |
| 13:02:15 | efried | mdbooth: stand by. | |
| 13:02:30 | efried | mdbooth: Okay, do tell. | |
| 13:02:47 | mdbooth | efried: I see you proposed it because of the loop in the ironic driver | |
| 13:03:13 | efried | mdbooth: Well, actually because of [2] (in the commit message), but [1] was a good excuse. | |
| 13:03:25 | mdbooth | efried: Sec, lemme check 2 | |
| 13:03:26 | openstackgerrit | Aditya Vaja proposed openstack/nova master: remove IVS plug/unplug as they're moved to separate plugin https://review.openstack.org/534371 | |
| 13:03:50 | efried | mdbooth: TL;DR, we were checking for volume-ness in a different (and apparently wrong | |
| 13:03:55 | efried | ) way in the powervm driver. | |
| 13:04:15 | mdbooth | efried: Thing is, the ironic driver doesn't need that check | |
| 13:04:16 | efried | mdbooth: mriedem pointed out that we should be using is_volume... which led us to find out that it didn't exist. | |
| 13:04:38 | efried | mdbooth: Well, is the new version of the check doing something the old one wasn't? | |
| 13:04:47 | mdbooth | Because, confusingly, block_device_info_get_mapping() should actually be called block_device_info_get_volumes() | |
| 13:04:55 | efried | mdbooth: I.e. did we break something afresh, or just leave something that (we didn't know) was broken before? | |
| 13:05:11 | mdbooth | If it was called *_get_volumes(), it would be obvious that an is_volume call was redundant subsequently | |
| 13:05:39 | jaypipes | stephenfin: yes, sorry, I understand the differences between gre, vlan, vxlan, local, etc. what I'm wondering about is why you used the term "mechanism". Is that because of the use of "mechanism driver" to describe the implementation module that Neutron uses under the hood for constructing a particular network? | |
| 13:06:10 | mdbooth | Where's _vol_drv_iter() called in your patch? | |
| 13:06:21 | stephenfin | jaypipes: I think I saw it used in documentation somewhere and couldn't think of a better term :) | |
| 13:07:41 | mdbooth | efried: Where I'm going with that is... are you sure they aren't all volumes anyway? And if they're not, why not just get the volumes in the first place? | |
| 13:08:35 | jaypipes | stephenfin: the reason I asked is because neither of those pages mentions the term "mechanism", and it seems to me (I may very well be wrong) that you are conflating two things (segmentation protocol and whether overlapping IP address ranges can occur on the network) | |
| 13:09:12 | mdbooth | efried: Ah, yes. You're doing the same thing. | |
| 13:09:18 | mdbooth | efried: So, they're all volumes. | |
| 13:09:28 | efried | mdbooth: So let's back up just for a sec. | |
| 13:09:45 | mdbooth | Have a look in driver.get_block_device_info(), which is what generates that data | |
| 13:09:49 | mdbooth | See: | |
| 13:09:55 | mdbooth | 'block_device_mapping': | |
| 13:09:56 | mdbooth | virt_block_device.convert_all_volumes(*block_device_mapping) | |
| 13:09:58 | efried | mdbooth: 1) Is there actually a problem here, or just some redundancy? and 2) was anything actually broken (or changed at all) by the patch you referenced? | |
| 13:10:19 | mdbooth | 'block_device_mapping' should really be called 'volumes' | |
| 13:10:57 | stephenfin | jaypipes: Hmm, protocol would probably have done. I definitely saw 'mechanism' used somewhere but of course I can't find where now | |
| 13:11:08 | mdbooth | Well, the change is redundant, but due to bad naming that's not at all obvious. | |
| 13:11:11 | efried | mdbooth: It is my understanding that there is quite a lot of horribleness in the bdm code that could be improved upon. | |
| 13:11:18 | jaypipes | stephenfin: no worries, it's not a huge deal really. | |
| 13:11:41 | mdbooth | The issue is that you added is_volume, which is a property, to the inherited list | |
| 13:11:46 | efried | mdbooth: The change itself is (was intended to be) a no-op. It was only supposed to get rid of the need to ask that @property through a private member of the outer bdm. | |
| 13:12:01 | stephenfin | jaypipes: I don't think I'm conflating those things. The main difference is whether it's an L2 network (uses physnets) or L3 network (uses a tunnel endpoint). tbh, the whole section on tenant networks and provider networks could be dropped | |
| 13:12:09 | mdbooth | So it breaks when something attempts to write to it | |
| 13:12:25 | efried | mdbooth: Write to what, .is_volume ? | |
| 13:12:29 | mdbooth | Yeah | |
| 13:12:34 | stephenfin | I'll probably do that as a follow-up. At this point I just want to get the spec in so I can grab a runway :) | |
| 13:12:37 | efried | mdbooth: Who was doing that, and where? | |
| 13:12:44 | efried | and why? | |
| 13:13:07 | efried | and based on what source info, if not _bdm_obj.is_volume itself? | |
| 13:13:18 | efried | mdbooth: sounds like we need a fup to remove... whatever that was. | |
| 13:13:26 | efried | mriedem: see email | |
| 13:13:34 | mdbooth | efried: Let me reload that context, but it broke my patch https://review.openstack.org/#/c/528362/ | |
| 13:13:57 | mdbooth | efried: I believe the issue is due to something iterating over properties to initialise the object | |
| 13:14:02 | mdbooth | Let me just remind myself... | |
| 13:14:10 | mriedem | nothing like an immediate pounce | |
| 13:14:23 | efried | mriedem: No hurry, just responding to your question about the granular bp | |
| 13:14:37 | efried | I suppose I didn't need to pounce for that. Sorry. | |
| 13:14:55 | ShilpaSD | stephenfin: Hi | |
| 13:14:59 | efried | mdbooth: Okay, so it kicked your patch out of the gate, didn't like melt CERNs supercollider, that's a relief anyway. | |
| 13:15:23 | mdbooth | efried: My patch is more important than somebody's magnet set. | |
| 13:15:29 | efried | clearly | |
| 13:17:59 | jaypipes | stephenfin: understood. | |
| 13:25:06 | mdbooth | efried: Ok, so I think I can unpick this from my end. | |
| 13:25:23 | mdbooth | efried: However, the point about is_volume being redundant in that context still stands | |
| 13:25:43 | efried | mdbooth: Okay. From what I've been able to tell so far, it's not clear that we should be setting things on _bdm_obj rather than on the outer object itself. | |
| 13:25:47 | mdbooth | Which, as I say, would be obvious if we called 'block_device_mappings' 'volumes' | |
| 13:26:18 | mriedem | lyarwood: can you poke this regression fix for queens? https://review.openstack.org/#/c/564461/ we're going to do a release soon for another regression fix | |
| 13:26:19 | efried | mdbooth: I'll be on the lookout for that review :) | |
| 13:26:32 | mriedem | this one https://review.openstack.org/#/q/If9e1581db9c1ae14340b787d03c815d243d5a50c | |
| 13:26:54 | mdbooth | efried: Meh, I can't be arsed renaming things I'm not actively hitting. It's painful and nobody cares. | |
| 13:27:09 | lyarwood | mriedem: yeah np, looking now | |
| 13:27:33 | efried | mdbooth: Just so. Also, that's a slippery slope, as I mentioned before. Once we start "fixing" the block device code, there'll be no stopping. | |
| 13:28:55 | jmccarthy | mriedem: Morning ! I checked this out and it worked out great https://review.openstack.org/#/c/566367 ! | |
| 13:32:42 | openstackgerrit | Jay Pipes proposed openstack/nova master: add consumer.generation/create missing consumers https://review.openstack.org/557958 | |
| 13:32:43 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: add Project, User and Consumer objects https://review.openstack.org/565403 | |
| 13:32:44 | openstackgerrit | Jay Pipes proposed openstack/nova master: Add create() methods to Project, User and Consumer https://review.openstack.org/565404 | |
| 13:32:45 | openstackgerrit | Jay Pipes proposed openstack/nova master: move consumer ensure to API layer https://review.openstack.org/565405 | |
| 13:32:47 | openstackgerrit | Jay Pipes proposed openstack/nova master: Add a microversion for consumer generation support https://review.openstack.org/565604 | |
| 13:32:47 | openstackgerrit | Jay Pipes proposed openstack/nova master: rework allocation handler _allocations_dict() https://review.openstack.org/565407 | |
| 13:33:10 | mdbooth | efried: Added a comment to the PowerVM patch. Not really worth a -1, tbh, but I figure somebody's going to have to touch it anyway. | |
| 13:34:00 | lyarwood | mriedem: right, so this is a stable/queens cherry-pick of a revert of a bugfix that only ever landed on master, fun. | |
| 13:35:06 | lyarwood | mriedem: it's valid as bugfix on stable/queens AFAICT, do we have enough time to drop the `This reverts commit` line? | |
| 13:36:45 | mriedem | lyarwood: which one? https://review.openstack.org/#/c/564461/ ? | |
| 13:36:49 | mriedem | it's not really a revert, it's a redo | |
| 13:37:02 | mriedem | jmccarthy: great, thanks | |
| 13:37:12 | lyarwood | mriedem: yeah the revert line just confused me | |
| 13:37:19 | mriedem | jaypipes: i think you have some context on this https://review.openstack.org/#/c/566367 | |
| 13:37:27 | mriedem | would be good to get that fixed since we backported the regression to ocata | |
| 13:37:57 | mriedem | lyarwood: right, so we revert the one workaround hack to remove the instance dir after the by_name init thing | |
| 13:38:12 | mriedem | because if we don't do that hack, we don't need to remove the dir, and that was the original bug anyway | |
| 13:39:07 | mriedem | lyarwood: https://review.openstack.org/#/c/524208/ landed when queens was master | |
| 13:39:11 | lyarwood | mriedem: right, ignore me, the fscking gerrit UI didn't list it as in Queens | |
| 13:39:23 | mriedem | "included in" dropdown | |
| 13:39:26 | mriedem | is what i check | |
| 13:39:41 | mriedem | lyarwood: you and mdbooth might also be interested in https://review.openstack.org/#/c/566367 | |
| 13:39:42 | lyarwood | mriedem: yeah it didn't display correctly, checked it again and there it is | |
| 13:39:45 | mriedem | dealing with libvirt imagebackend fun | |
| 13:42:14 | efried | mdbooth: ack, and thanks. | |
| 13:46:25 | mdbooth | mriedem: Somebody should clean that thing up ;) | |
| 13:52:22 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix detach_volume calls when rolling back a failed attach https://review.openstack.org/563213 | |
| 13:52:23 | mriedem | efried: gibi: test fixed, ^ - nice catch gibi | |
| 13:52:31 | mriedem | that's one that also needs to go back to queens as a regression fix | |
| 13:53:29 | mriedem | dansmith: can you hit this queens backport? https://review.openstack.org/#/c/564461/ | |
| 13:54:04 | dansmith | mriedem: yeah | |