Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-27
15:24:16 kashyap I held my nose, and cleaned up the excrements
15:24:17 mdbooth kashyap: What code's this?
15:24:35 kashyap mdbooth: Okay, now you're Cced :D it is this: https://review.openstack.org/#/c/620327/
15:24:50 mriedem mdbooth: fyi, some nits in https://review.openstack.org/#/c/591733/ which might be worth a follow up
15:24:50 kashyap mdbooth: BTW, mriedem was pinging you for something else, perhaps.
15:25:11 mdbooth mriedem: Looking
15:25:21 kashyap artom: Since you did click, please go the next step and review. And if need be, complain if you need a test, etc
15:25:32 artom kashyap, yep, doing that now
15:25:37 artom (reviewing, not complainig)
15:25:48 kashyap Thanks!
15:26:00 mdbooth mriedem: Awesome, thanks!
15:26:52 kashyap artom: That TODO item
15:27:04 kashyap (Err hit send too soon)
15:27:10 artom I call that PCRS
15:27:17 artom Premature Carriage Return Syndrome
15:27:40 kashyap artom: ... is part of a larger work item that needs cleaning up of assumptions about machine types in Nova. And coordinating w/ relevant libvirt work
15:28:04 kashyap artom: Heh
15:38:03 mriedem mdbooth: what's the difference between the 'disk' and 'disk.local' entries in the block_device_info disk 'mapping' entry?
15:38:17 mdbooth mriedem: 'disk' is the root disk
15:38:33 mdbooth 'disk.local' is legacy, and no longer generated
15:38:53 mdbooth But it used to be the single ephemeral disk
15:38:54 mriedem unless...
15:39:06 mdbooth Now we have disk.eph0, disk.eph1, etc
15:39:11 mriedem if we have code for it, we must support it somehow...
15:39:18 mriedem is it for bdmv1?
15:39:27 mriedem or just really old bdm records in the db?
15:39:46 mdbooth It's a long time since I've checked the details on this, but I recall convincing myself in the past the current code can't generate this
15:39:49 mdbooth Yeah, it's old BDM records
16:10:02 mriedem mdbooth: comments inline https://review.openstack.org/#/c/578846/
16:14:34 mriedem dansmith: efried: mdbooth: i'd like to move forward with getting this evacuate-related bug fixed https://review.openstack.org/#/c/606122/ - i think the open question was do we use dansmith's more generic solution https://review.openstack.org/#/c/575190/ or keep my "tactical" fix for backports and then rebase dan's on top to replace my specific fix with the generic one
16:15:11 mriedem because that's holding up getting us to do evacuate testing with both rbd and lvm backends https://review.openstack.org/#/c/604400/
16:18:36 dansmith mriedem: if you'll do the rebase of mine and cleanup of this spot after we merge yours that's fine
16:18:49 dansmith I would backport my generic solution too, but if you don't want to that's fine
16:20:32 mdbooth mriedem: ^^^ wfm.
16:23:04 mriedem dansmith: ack will do
16:29:17 efried I'll need to reread everything to refresh my memory of what's going on here.
16:33:38 efried so wait, which are we doing? just dan's thing, master & backports; or matt's thing for backports and dan's thing for master?
16:35:06 dansmith we're doing whichever of those mriedem wants
16:35:14 dansmith which I think is the latter
16:35:21 mriedem the latter
16:36:15 efried okay, so my job is to +2 mriedem's patch (done) and then wait for mdbooth to be satisfied with dansmith's patch before reviewing it.
16:37:18 dansmith efried: I think the -1 on mine isn't actually a thing
16:39:04 efried yeah, but I'm drowning, so looking for any excuse to defer
16:41:31 mdbooth dansmith: It's not a thing?
16:41:39 dansmith mdbooth: the security groups thing?
16:42:27 mdbooth dansmith: Yeah. IIRC I would have been happy with a command saying: # We broke this, but we don't care because it's not a thing any more.
16:42:38 mdbooth s/comment
16:43:10 dansmith I don't understand why it's a thing at all anyway.. if the instance is deleted, we're not going to do anything with the security groups we do fetch out of the db (if there are any anyway),
16:43:21 dansmith and, in any case where we did load the instance with a deleted=yes context,
16:43:24 dansmith we'd have gotten the same
16:44:55 dansmith so the only case where we'd do something different is where we had an instance, which wasn't deleted when we started, but was deleted underneath us and we go to lazy-load security groups.. if we were to go on and save that instance later, it'd fail and I can't think of anything else we'd do with it that would trigger some different behavior than we have now
16:45:01 dansmith which I think was also mriedem's reply
16:47:03 mriedem i was specifically worried about instance.services but mel said it was something else there
16:47:12 mriedem i.e. the bug she was working on
16:47:48 mdbooth dansmith: I think think so. I think the bug is that if you've got an instance which isn't deleted and you lazy load security groups, you get deleted security groups. I don't think you need to delete the instance in between or anything.
16:47:56 mdbooth s/think think/don't think/
16:48:47 mdbooth dansmith: But we only care about that if we care about nova security groups, which IIUC are nova-network only.
16:49:59 mriedem they are nova-net only
16:50:07 dansmith mdbooth: ah, right, I remember you clarifying this again post initial review
16:50:25 dansmith they're n-net only, and don't work with cellsv1, right? so we really shouldn't have any supported cases where they're there anymore
16:51:00 mriedem so,
16:51:21 mriedem SecurityGroup.get_by_instance calls db api security_group_get_by_instance which hard-codes read_deleted="no"
16:51:33 mriedem *SecurityGroupList.get_by_instance
16:51:44 mriedem so isn't the hard-coding in the DB API preventing that
16:52:47 dansmith oh does it?
16:53:08 mdbooth mriedem: Looks like I missed that. That's convenient :)
16:53:13 dansmith sure does
16:57:15 dansmith recorded for posterity
16:57:15 sean-k-mooney speaking of things in nova net the function that enables trusted vfs was added to the nova net module
16:57:32 sean-k-mooney it proably should be moved to an sriov plugin in os-vif
16:59:01 sean-k-mooney specifically https://github.com/openstack/nova/blob/c6218428e9b29a2c52808ec7d27b4b21aadc0299/nova/network/linux_net.py#L1773 should be moved
17:00:43 sean-k-mooney mriedem: by the way i havent forgot about looking into can we remove NeutronLinuxBridgeInterfaceDriver too but i havent got around to it yet either
17:01:17 mriedem ken'ichi says it's ok https://review.openstack.org/#/c/616995/
17:01:33 mdbooth mriedem: Going back a patch, I was wondering about adding an 'intent' to driver.cleanup and/or driver.destroy, i.e. driver.destroy(reason=MIGRATION|DELETE). We currently have destroy_disks, but the problem with this is that we second guess it in ComputeManager, which makes it basically useless.
17:02:22 mdbooth What we want the driver to do is, for delete: delete everything. For migrate/evacuate: delete everything local to this hypervisor.
17:03:03 mdbooth I think the definition of 'local to the hypervisor' is more cleanly defined internally by the hypervisor.
17:03:05 slaweq hi nova cores, can You take alook at https://review.openstack.org/#/c/619061/ - it should fix neutron-tempest-postgres-full periodic job in gate so it's quite important for us :)
17:03:08 slaweq thx in advance
17:03:27 sean-k-mooney mriedem: oh ok cool i think there is more in that file we are not using that we could remove too
17:03:56 mriedem mdbooth: i don't expect you'll get much in the way of replies on that in the ML
17:04:08 mriedem people, like me, will likely say they don't want to think about it
17:04:25 mdbooth mriedem: It's a source of a class of real bugs, thoough
17:04:26 mriedem mdbooth: so having said that, i assume you aren't proposing to change your current fix to incorporate that right?
17:04:32 mriedem mdbooth: oh i don't doubt it
17:04:34 mdbooth mriedem: Hell, no
17:04:55 mdbooth mriedem: I was thinking about lyarwood's original proposal to fix an extra thing in my patch
17:04:58 mriedem there is definitely a russian roulette of disk deletion in compute/virt
17:05:16 mdbooth I looked harder at that, and I don't think we can fix it like that
17:07:42 openstackgerrit Jack Ding proposed openstack/nova master: Improve libvirt image and snapshot handling https://review.openstack.org/616692
17:12:03 openstackgerrit Jack Ding proposed openstack/nova-specs master: [WIP] Flavor Extra Spec and Image Properties Validation https://review.openstack.org/618542
17:14:48 mdbooth mriedem: Practically speaking, what would be the most practical way to go about making that sort of change? Just post a patch? Problem is it wouldn't be a small amount of work, so I'd be cautious of that without some level of buy in beforehand. BP?
17:17:19 mriedem bp doesn't give you much in the way of details, spec allows you to really write the details down and get review on it without getting too far into implementation, but idk
17:17:47 mriedem gonna be a hard sell to get people to want to spend time/brain power on this
17:38:15 sean-k-mooney mdbooth: blueprints tend to be more for tracking then design if you want input you will want a spec or at least a etherpad + hangout if you have a quorum of people that are interested
17:44:07 openstackgerrit Matt Riedemann proposed openstack/nova master: Refactor TestEvacuateDeleteServerRestartOriginalCompute https://review.openstack.org/608705
17:44:07 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix InstanceNotFound during _destroy_evacuated_instances https://review.openstack.org/606122
17:44:08 openstackgerrit Matt Riedemann proposed openstack/nova master: Run evacuate tests with local/lvm and shared/rbd storage https://review.openstack.org/604400
17:44:08 openstackgerrit Matt Riedemann proposed openstack/nova master: Always read-deleted=yes on lazy-load https://review.openstack.org/575190
17:44:21 mriedem dansmith: rebasearoo
17:45:53 openstackgerrit Matt Riedemann proposed openstack/nova master: DNM: Run against mriedem's evacuate test https://review.openstack.org/604423

Earlier   Later