Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-21
16:46:33 dansmith mriedem: well, it's just one backend remember, and that one is fixed now
16:46:35 mriedem if initialize_connection is supposed to be idempotent, which i always thought it was, i don't see the issue
16:46:43 jgriffith mriedem: you have a link to the bug?
16:46:51 mriedem jgriffith: nova bug https://bugs.launchpad.net/nova/+bug/1754716
16:46:52 openstack Launchpad bug 1754716 in OpenStack Compute (nova) "Disconnect volume on live migration source fails if initialize_connection doesn't return identical output" [Undecided,In progress] - Assigned to Matthew Booth (mbooth-9)
16:46:56 mriedem jgriffith: cinder bug https://bugs.launchpad.net/cinder/+bug/1756914
16:46:57 openstack Launchpad bug 1756914 in Cinder "Dell EMC SC: Initialize_connection returns all connections" [Undecided,Fix released] - Assigned to Tom Swanson (tom-swanson)
16:47:06 mriedem plus a red hat bz that we don't have access to
16:47:14 dansmith yeah, sorry :(
16:47:28 dansmith jgriffith: gorka has context on this but I don't see him around here
16:47:46 mriedem he's in -cinder
16:47:53 jgriffith Yeah, not in this channel, I'll ping him
16:47:58 dansmith I looked, maybe I don't know his nick?
16:48:05 dansmith I thought it was geguilar or something
16:48:08 mriedem he's been pung
16:48:27 dansmith huh, sorry, I looked and just missed it
16:48:36 geguileo mriedem: jgriffith hi
16:49:10 mriedem who wants to update him?
16:49:19 mriedem i can start
16:49:23 mriedem geguileo: so you're familiar with https://bugs.launchpad.net/nova/+bug/1754716 right?
16:49:25 openstack Launchpad bug 1754716 in OpenStack Compute (nova) "Disconnect volume on live migration source fails if initialize_connection doesn't return identical output" [Undecided,In progress] - Assigned to Matthew Booth (mbooth-9)
16:49:25 dansmith geguileo: it's about this: https://bugs.launchpad.net/nova/+bug/1754716
16:49:39 dansmith geguileo: which was a cinder backend fix for the dell driver, plus making nova not call initialize_connection() a second time
16:49:46 mriedem geguileo: i'm trying to understand if initialize_connection is supposed to be idempotent or not
16:49:48 smcginnis Swanson: You may be interested too. ^^
16:49:55 Swanson smcginnis, already there
16:50:03 dansmith geguileo: you asserted in our downstream bug that calling initialize_connection really late during disconnect after a live migration was wrong and we should avoid doing it
16:50:04 geguileo mriedem: the problem is that Cinder never asked drivers to be idempotent
16:50:05 Swanson here
16:50:06 jgriffith mriedem: you're just begging for 3 different answers :)
16:50:11 smcginnis mriedem: It is, but that was a surprise to most around the Liberty timeframe.
16:50:37 smcginnis Or rather, it wasn't, until we found out around liberty that nova was initializing repeatedly.
16:50:37 geguileo dansmith: it's wrong because Cinder never specified that initialize_connection should be idempotent :-(
16:50:48 jgriffith ummm.....
16:50:55 mriedem smcginnis: yeah we do it all the time during migrations because we only have one copy of the connection_info at any time with the old flow
16:51:19 mriedem so the only copy of the connection_info is in the nova bdm table, and it's host-specific
16:51:25 mriedem on which was the last host to update it
16:51:34 mriedem with the new flow, that's all stored in cinder per-host attachment
16:51:37 mriedem so nova doesn't have to care
16:51:43 jgriffith initialize_connection has never been designed as an idempotent thing, in fact it was intentionally abused for that very reason
16:51:47 geguileo mriedem: but if Nova is the source of truth it shouldn't overwrite their data
16:52:02 geguileo for old API I mean
16:52:09 mriedem geguileo: cinder is the source of truth when it comes to volume informatoin imo
16:52:17 mriedem but,
16:52:20 geguileo mriedem: not for attach information on the old API
16:52:44 mriedem if 'source of truth' means, when you disconnect a volume, regardless of host, you need to use the exact same connection_info as you used when you connected it,
16:52:50 mriedem then yes nova needs to track that for the old flow
16:53:10 geguileo yup, that's what I meant by 'source of truth'
16:53:13 mriedem it's just a surprise to me that after all these years it's a new problem
16:53:26 dansmith mriedem: with one (known) driver, remember
16:53:32 smcginnis mriedem: Why is this a new problem?
16:53:40 jgriffith mriedem: so the trick is that early drivers (and a large percentage) don't care here
16:53:42 mriedem and, as i said to dan earlier here, "i wouldn't be surprised if at some later date, someone reports a bug saying something in the storage backend or source host changed and they actually need to get the latest information to disconnect"
16:54:01 smcginnis I thought we mostly had fixed all instances we identified when we realized the call was being used this way.
16:54:11 jgriffith mriedem: they don't have any interaction with the initiator or it's settings, that's just been extra *data*
16:54:16 dansmith smcginnis: all but one, which was recently fixed:
16:54:21 dansmith https://review.openstack.org/#/c/552933/
16:54:25 jgriffith but now with new fangled iSCSI stuff and LIO drivers that's not the case any more
16:54:35 geguileo mriedem: the big problem here is not as much the data you pass to disconnect to Cinder, it's what you pass to os-brick
16:54:48 mriedem geguileo: yeah i realize
16:55:05 mriedem detach api in cinder is just flipping the volume status in the db
16:55:18 mriedem so another question in that regard,
16:55:32 mriedem we use a host connector with the original initialize_connection call,
16:55:38 mriedem and we pass a host connector to terminate_connection,
16:55:45 mriedem do those host connectors have to be the same?
16:55:59 jgriffith mriedem: but terminate con has implications for some devices, that's the part that matters (which is what I think you're saying right now)
16:56:21 mriedem nova just gets the host connector from os-brick on the fly
16:56:46 mriedem except in some weird edge cases like when the source compute is down (evacuate)
16:56:55 geguileo mriedem: For the RBD driver (which is the one I primarily touch) it doesn't matter, but I don't know about other drivers
16:57:13 geguileo in theory it should not matter
16:57:27 jgriffith mriedem: for some drivers it matters, they modify and access entry for the initiator iqn
16:57:36 mriedem ok. i'm alright with us being told that we shouldn't assume initialize_connection is idempotent and we should use the same connection_info to disconnect the volume as was used to connect the volume
16:57:37 jgriffith mriedem: and they get that from the connector
16:57:41 geguileo as long as you haven't changed the initiator name or something crazy like that (I've seen bugs around this)
16:58:05 mriedem i just don't want to find out later that someone relied on us always getting the latest info
16:58:30 jgriffith mriedem: +1 for NOT assuming idempotency, AND +2 for suggesting upgrades and using the new API's instead :)
16:58:32 mriedem with the new flow, we don't get the latest info anyway, it's all stored in cinder and we just re-use it
16:58:33 dansmith mriedem: "much later" we expect to always be using the new API that doesn't suffer from this right?
16:58:47 mriedem dansmith: yeah
16:58:48 jgriffith of course, assumign we're not broken in there as well somewhere
16:59:06 mriedem any existing attached volumes would have the old flow stuff until they get migrated
16:59:24 jgriffith because I think we may be now in our efforts to emulate the old behavior
16:59:26 mriedem which now this makes me paranoid about https://review.openstack.org/#/c/549130/
16:59:37 melwitt gibi: are you able to run the nova meeting tomorrow?
17:00:10 mriedem jgriffith: this is the problematic code https://github.com/openstack/nova/blob/2ec8c49f6cb4a0e7dba217e824c20d9c703d2105/nova/virt/libvirt/driver.py#L7743
17:00:29 mriedem that runs on the source host post-live migration
17:00:35 gibi melwitt: yes I can run it
17:00:38 mriedem for the new flow, we just get the connection_info out of the attachment record
17:00:45 melwitt gibi: cool, ty for confirming
17:00:55 jgriffith ahh, yeah
17:01:26 mriedem so the old flow thing there to call init_connectoin was a workaroudn
17:01:37 gibi melwitt: will you update the agenda or shall I do it?
17:01:37 mriedem b/c at this point in the flow, in the nova db, the bdm.connection_info is actually for the dest host
17:01:53 melwitt gibi: I'll update the agenda
17:02:54 gibi melwitt: OK, thanks
17:02:58 jgriffith mriedem: yeah, I get ya
17:02:58 mriedem ok so to summarize, i think i'm hearing the majority of the cinder peeps in here saying doing https://github.com/openstack/nova/blob/2ec8c49f6cb4a0e7dba217e824c20d9c703d2105/nova/virt/libvirt/driver.py#L7749 is a bad workaround and only works for some drivers b/c we're getting lucky
17:03:08 melwitt gibi: no, thank YOU :)
17:03:10 mriedem even though we've always done it....but anyway
17:03:25 dansmith mriedem: well, we've always done it which ended up in them changing a bunch of drivers to tolerate it

Earlier   Later