Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-21
16:39:27 dansmith mriedem: can you see this? https://bugzilla.redhat.com/show_bug.cgi?id=1552232
16:39:28 openstack dansmith: Error: Error getting bugzilla.redhat.com bug #1552232: NotPermitted
16:39:29 mriedem so when we disconnect post live migration, we'll be using that original value, not something new
16:39:43 mriedem dansmith: nope
16:39:43 dansmith guess not
16:40:22 dansmith mriedem: yeah, using the original attachment id is definitely better than having to have these connectors line up and potentially be changed if you're saying that could happen mid-migration
16:41:54 mriedem i guess my point is just, 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:42:21 mriedem sort of like the discussion in denver about always refreshing connection_info whenever we do stuff
16:42:29 mriedem to get things like latest ceph mon address and creds
16:42:32 dansmith mriedem: so actually, re-reading gorka's comments,
16:43:07 dansmith mriedem: what he's saying we shouldn't be doing is calling initialize_connection again to get the connector from the volume
16:43:18 dansmith which we're then using to detach
16:43:30 dansmith so old line 7724 here: https://review.openstack.org/#/c/551302/6/nova/virt/libvirt/driver.py
16:44:25 mriedem yeah i understand, but as noted, we have to do that because right now with the old flow, the bdm.connection_info is from the dest host, and the only way to get the connection_info with the old flow for the source host is to call initialize_connection with the source host connector
16:44:50 dansmith yeah I know
16:45:25 mriedem so i guess the point is calling initialize_connection when the volume is already connected on another host can cause problems if you call it again, for some cinder backends
16:45:38 mriedem but we've been doing this forever
16:45:43 dansmith mriedem: "So Nova should not be making a second initialize connection for a volume that is already attached to the node and use the information it already has in the DB to do the disconnect"
16:46:07 mriedem except we didn't have that information in the db
16:46:08 dansmith ^ gorka's comments from the downstream bug
16:46:11 mriedem hence the workaround
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 dansmith geguileo: it's about this: https://bugs.launchpad.net/nova/+bug/1754716
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: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 geguileo dansmith: it's wrong because Cinder never specified that initialize_connection should be idempotent :-(
16:50:37 smcginnis Or rather, it wasn't, until we found out around liberty that nova was initializing repeatedly.
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

Earlier   Later