| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-21 | |||
| 16:35:44 | mriedem | what if we should be using the latest connection_info from cinder for the source host to disconnect? | |
| 16:35:53 | mriedem | like, what if the rbd driver changed the auth information for the ceph backend? | |
| 16:36:00 | mriedem | and the original stuff in the source host connection_info is stale | |
| 16:36:31 | dansmith | mriedem: did you check out the cinder bug and patch? | |
| 16:36:41 | dansmith | gorka says we shouldn't sending the latest | |
| 16:36:52 | mriedem | looking | |
| 16:37:04 | dansmith | I dunno if that means we shouldn't be sending the thing we grabbed before the migration if it changed right after, | |
| 16:37:10 | dansmith | but that'd be fairly obscure and hard to do I think | |
| 16:37:23 | dansmith | we'd have to catch it before it gets set and overwritten | |
| 16:38:12 | dansmith | well, I guess gorka's comments are in our downstream bug actually | |
| 16:38:27 | mriedem | yeah was going to say | |
| 16:38:38 | mriedem | i feel like i'll get different answers based on which cinder cores i ask | |
| 16:38:42 | mriedem | jgriffith: smcginnis | |
| 16:38:46 | mriedem | hemna | |
| 16:39:16 | mriedem | having said that, for the new flow, the original connection_info from the source host is stored in cinder with the attachment record for the source host, | |
| 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, | |