| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-21 | |||
| 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 | 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:37 | gibi | melwitt: will you update the agenda or shall I do it? | |
| 17:01:53 | melwitt | gibi: I'll update the agenda | |
| 17:02:54 | gibi | melwitt: OK, thanks | |
| 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:02:58 | jgriffith | mriedem: yeah, I get ya | |
| 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 | |
| 17:03:38 | mriedem | sure, good point | |
| 17:03:48 | mriedem | it's a fun incestual relationship | |
| 17:03:55 | geguileo | rofl | |
| 17:04:08 | dansmith | really? I don't enjoy any of it :) | |
| 17:04:09 | dansmith | but you're into some weird shit, granted | |
| 17:04:22 | mriedem | jgriffith: btw, we should remember to talk about this in vancouver about why the new flow is much better.... | |
| 17:04:32 | jgriffith | mriedem: +1 | |
| 17:04:53 | mriedem | alright i'll move onto reviewing the nova patch then, thanks geguileo jgriffith smcginnis | |
| 17:05:23 | geguileo | no problem, and thanks for fixing that :-) | |
| 17:05:52 | jgriffith | mriedem: just for the record one more time and to hope it sticks in peoples heads... "Never update/modify and attachment, just delete it and create a new one" | |
| 17:06:17 | jgriffith | That way you still have the ability to access the old data if you need it and nothing gets funky behind your back | |
| 17:06:23 | jgriffith | hmmm... well, anyway | |
| 17:06:42 | jgriffith | "funky behind your back" isn't the best phrase I guess | |
| 17:06:46 | mriedem | because that would be like calling initialize_connection again | |
| 17:06:57 | jgriffith | mriedem: exact-u-mundo | |
| 17:11:45 | dansmith | mdbooth: did you follow all that ^ ? | |
| 17:13:12 | mriedem | i put the link into the bug for the irc log and a summary | |
| 17:13:21 | dansmith | cool | |
| 17:14:13 | mriedem | while we're on the topic https://review.openstack.org/#/c/549130/2/nova/compute/manager.py@616 | |
| 17:14:21 | mriedem | in case any cinder people are still here and can direct me | |
| 17:16:09 | dansmith | mriedem: btw, I changed this to fixes and left a snarky remark: https://review.openstack.org/#/c/554600/ | |
| 17:16:17 | dansmith | if you say that's good I'll fix the pike backport | |
| 17:18:41 | dansmith | mriedem: melwitt: tssurya: do we need to have a cells meeting today? I know there are a bunch of tssurya's patches to review, but other than that, I don't know of anything else burning that justifies it | |
| 17:19:02 | tssurya | dansmith: not needed | |
| 17:19:22 | dansmith | tssurya: especially if it means I spend the time reviewing your stuff yeah? :) | |
| 17:19:35 | tssurya | yeaaaaa :D | |
| 17:20:24 | melwitt | +1 to skipping today | |
| 17:20:46 | cdent | the whole day | |
| 17:20:48 | dansmith | woot, even if mriedem really wants a meeting, he's outnumbered | |
| 17:21:44 | ildikov | jgriffith: you need to repeat those that thousands of times so they stick in people's heads :) | |
| 17:22:32 | cfriesen | tssurya: you pinged earlier? | |
| 17:25:21 | mriedem | ildikov: or push a patch to nova to add something to the docstring for the attachment_update method so we don't forget | |
| 17:25:26 | mriedem | i'd +2 that | |
| 17:26:21 | ildikov | mriedem: I can when I'm off booth duty and got to the next hotel today | |
| 17:26:22 | mriedem | dansmith: we've changed 'features' to 'fixes' in release notes in backports for other nova-manage changes for optional things, in order to fix bugs, so i think that's fine | |
| 17:27:07 | tssurya | cfriesen: yea just to ask about the third point in the bug description, since you explicitly stated delete service/compute node, I wanted to just confirm we are still doing a soft delete | |
| 17:27:49 | tssurya | cfriesen: saw you review btw, will change it to cascade, working on some tests and then will update the patch | |
| 17:27:51 | ildikov | mriedem: It won't replace the need of repeating the phrase a thousand times everywhere else | |
| 17:28:48 | cfriesen | tssurya: I think the service/node portion can remain as it is currently. what you have looks reasonable to me with the cascade change | |
| 17:29:54 | tssurya | cfriesen: ack, however I think maybe we need to provide a new command at some point to allow compute node deletions ? like we have osc placement allowing resource provider deletions ? | |
| 17:29:57 | cfriesen | tssurya: dansmith is the go-to guy in this area. :) | |
| 17:30:18 | cfriesen | tssurya: you mean compute node deletions without service deletion? | |
| 17:30:27 | dansmith | mriedem: fine, fine | |