| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-27 | |||
| 16:04:54 | mdbooth | imacdonn: Did you get the chance to try with the very latest os-brick? | |
| 16:05:24 | mdbooth | Ignore me, I see you did | |
| 16:05:40 | imacdonn | mdbooth: I applied the patch for that specific fix... I suppose it's possible that something else changed in another commit, but I doubt it | |
| 16:10:36 | sfinucan | johnthetubaguy: This patch hit a merge conflict. Fancy reviewing it again to get it in? https://review.openstack.org/#/c/418489 | |
| 16:10:50 | sfinucan | (previously +w'd, but very prone to merge conflict) | |
| 16:11:15 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Remove dead code https://review.openstack.org/460662 | |
| 16:11:15 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Replace reserved_host_disk_mb with reserved_host_disk_gb https://review.openstack.org/460661 | |
| 16:11:24 | mdbooth | imacdonn: So you're referring to _disconnect_volume_iscsi in os-brick, right? | |
| 16:11:34 | sfinucan | ^ edleafe, jaypipes: You might like the first of those | |
| 16:11:40 | mdbooth | imacdonn: The block starting: | |
| 16:11:46 | mdbooth | # target are in use. | |
| 16:11:46 | mdbooth | # NOTE(vish): Only disconnect from the target if no luns from the | |
| 16:11:52 | sfinucan | or you might not. It's kind of hacky, but oslo.config doesn't give me anything more powerful :( | |
| 16:12:02 | mdbooth | imacdonn: That's definitely looks racy | |
| 16:12:15 | edleafe | sfinucan: thx - will review | |
| 16:12:18 | jaypipes | sfinucan: cool, added to my queue. :) | |
| 16:12:54 | imacdonn | mdbooth: yes .. I understand why they want to clean up the session if it's not needed any more, but it reopens that window where another attachment cause zombie devices to appear | |
| 16:16:45 | mdbooth | imacdonn: So again the issue here is that we drop and then re-establish the connection to the target before terminate_connection has completed | |
| 16:17:17 | mdbooth | And on re-establishing the connection, we rescan and get the lun on the host | |
| 16:17:34 | mdbooth | And then terminate_connection does its thing and it goes stale | |
| 16:17:42 | mdbooth | Have I got the sequence right? | |
| 16:18:30 | imacdonn | mdbooth: there's no explicit rescan, but I suppose there is an effective one ... i.e. on new session establishment, *all* LUNs offered by the target are discovered | |
| 16:18:38 | mdbooth | Right | |
| 16:19:26 | mdbooth | So again we want a per-target lock which covers attach, detach, and terminate connection | |
| 16:19:54 | imacdonn | mdbooth: That seems to be the only way to really cover it | |
| 16:20:17 | imacdonn | mdbooth: I'm also a little bit wary of the os-brick patch .. it seems a bit sketchy, they way he's doing it now | |
| 16:20:33 | mdbooth | The problem being that at the scope where we could currently put that lock we have no knowledge of targets | |
| 16:23:21 | mdbooth | imacdonn mriedem johnthetubaguy: How many iscsi targets would typically exist in a deployment? | |
| 16:23:57 | mdbooth | I personally baulk at the idea of a Global ISCSI Lock | |
| 16:24:24 | mdbooth | But if a typical deployment would only have a single iscsi target anyway, then getting exercised about it is a bit pointless | |
| 16:24:48 | mriedem | mdbooth: 0 idea | |
| 16:24:54 | mriedem | mdbooth: you'd have to ask jgriffith or hemna probably | |
| 16:24:58 | imacdonn | in my deployments, I have two per AZ ... they are the two halves of a ZFSSA cluster | |
| 16:25:11 | mdbooth | Are there deployments out there doing lots of connect/disconnect which would be knobbled if you just serialised all iscsi operations regardless? | |
| 16:25:22 | dansmith | you have at least one target per cinder-volume worker I think, | |
| 16:25:23 | hemna | mdbooth, what do you mean? | |
| 16:25:44 | dansmith | so if you're running a bunch of cinder-volumes with local LVM type stuff, you could have lots of targets right? | |
| 16:25:51 | hemna | yes | |
| 16:26:11 | hemna | and you'd have lots of targets if you used a storage array behind cinder too | |
| 16:26:17 | dansmith | yeah | |
| 16:26:30 | hemna | a target per exported volume | |
| 16:26:32 | mdbooth | Ok, so a global iscsi lock would be a significant difference in practise to a per-target lock | |
| 16:26:57 | mdbooth | It would still be per-host though, I guess | |
| 16:27:00 | mdbooth | Perhaps it's not that bad | |
| 16:27:07 | imacdonn | depends on the type of storage array perhaps .. the ZFSSA has one target per control-head, and lots of volumes on that target with different LUN numbers | |
| 16:27:51 | hemna | but that's one target portal | |
| 16:28:02 | hemna | with multiple targets (one per lun) | |
| 16:28:28 | hemna | either way, yes, you can have multiple targets per storage backend going to the same initiator | |
| 16:28:54 | hemna | so there is a potential race between calling os-brick's disconnect_volume and calling cinder's terminate_connection. | |
| 16:29:06 | hemna | where a new attach comes in on the same backend and an scsi bus rescan can happen. | |
| 16:29:10 | mdbooth | Hmm, no. Even scoped to just iscsi is problematic because we need the lock to cover terminate_connection | |
| 16:29:33 | imacdonn | hemna: that's exactly what I've been experiencing (since the icehouse days) | |
| 16:29:42 | hemna | not surprising | |
| 16:29:56 | hemna | this can happen for FC based volumes too | |
| 16:30:20 | mdbooth | terminate_connection really needs to live with disconnect. Soon... | |
| 16:31:13 | hemna | not sure what you mean by that | |
| 16:31:22 | hemna | os-brick calling cinder's terminate_connection ? | |
| 16:31:44 | mdbooth | hemna: I haven't thought that far down, but maybe | |
| 16:31:46 | hemna | or just do a lock around disconnect_volume() and terminate_connection() | |
| 16:32:01 | hemna | os-brick has no business calling cinder | |
| 16:32:08 | mdbooth | That's the issue. Those things are all over the place. | |
| 16:32:27 | mdbooth | I'm not sure I agree with that assertion. | |
| 16:32:30 | imacdonn | it's not that easy .. terminate_connection() is not called from disconnect_volume() | |
| 16:32:40 | imacdonn | maybe it could be, though | |
| 16:33:04 | hemna | os-brick is not going to call cinder. | |
| 16:33:10 | hemna | I'll -2 that immediately | |
| 16:33:34 | smcginnis | +1! os-brick is a utiltiy library. | |
| 16:33:38 | mdbooth | If terminate_connection is relevant to the correct operation of the driver (to be determined, but if...) then we would at the very least need to establish a way to make it happen | |
| 16:33:47 | hemna | besides, we are changing the entire attachment API process between nova and cinder | |
| 16:33:47 | imacdonn | is there some way that os-brick could do a call back into some part of nova that then calls cinder ? | |
| 16:34:06 | hemna | I don't want os-brick knowing anything about who's calling it | |
| 16:34:12 | hemna | it's a library used outside of openstack too. | |
| 16:34:44 | imacdonn | so it'd be a generic "do what you need to do to make this LUN to way on whatever your backend is" interface | |
| 16:34:44 | mdbooth | hemna: I was also thinking the other day that locking requirements are driver-specific | |
| 16:34:51 | imacdonn | go away* | |
| 16:35:02 | mdbooth | hemna: So if we don't want os-brick to do any locking, I get that | |
| 16:35:19 | mdbooth | However, we still need a way for os-brick to tell us what locks we need to hold | |
| 16:35:33 | mdbooth | Take this case, for eg, the lock we want is per-target | |
| 16:35:43 | mdbooth | The caller has no idea what a target is, only os-brick knows that | |
| 16:36:04 | hemna | just lock around the process of disconnecting a volume from the host | |
| 16:36:18 | hemna | which is the workflow of disconnect_volume(), then terminate_connection() | |
| 16:36:20 | mdbooth | lock *what* around the process of disconnecting a volume | |
| 16:36:31 | imacdonn | hemna: it's not, though | |
| 16:36:34 | mdbooth | And now all drivers are going to have the same serialisation requirements | |
| 16:36:47 | mdbooth | For some drivers, concurrent attach/detach is fine | |
| 16:36:50 | imacdonn | hemna: terminate_connection() is called from a bunch of different places in compute.manager | |
| 16:36:50 | mdbooth | Others not | |
| 16:37:13 | mdbooth | For some, async terminate_connection is fine, no locking required | |
| 16:37:15 | mdbooth | Others not | |
| 16:37:27 | mdbooth | This is a hard interface | |
| 16:39:55 | mdbooth | hemna: On the terminate_connection front, it could reasonably be a callback for eg | |
| 16:40:05 | mdbooth | Then os-brick wouldn't have to know anything about cinder | |
| 16:40:41 | mdbooth | But 'tell the target to kill this lun now' is a generic concept | |
| 16:41:15 | imacdonn | yeah, that's what I'm envisaging | |
| 16:59:06 | hemna | ok so why does this guy not call brick's disconnect_volume ? | |
| 16:59:07 | hemna | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L3616 | |
| 17:00:06 | hemna | This is what a proper disconnect workflow should look like in nova https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2264-L2268 | |
| 17:00:59 | hemna | err | |
| 17:01:01 | hemna | wait...lol | |
| 17:01:09 | hemna | that doesn't call brick either. | |