17:00:15 <ildikov> #startmeeting cinder-nova-api-changes
17:00:16 <openstack> Meeting started Mon Jan 16 17:00:15 2017 UTC and is due to finish in 60 minutes.  The chair is ildikov. Information about MeetBot at http://wiki.debian.org/MeetBot.
17:00:17 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
17:00:20 <openstack> The meeting name has been set to 'cinder_nova_api_changes'
17:00:23 <ildikov> scottda DuncanT ameade cFouts johnthetubaguy jaypipes takashin alaski e0ne jgriffith tbarron andrearosa hemna erlon mriedem gouthamr ebalduf patrickeast smcginnis diablo_rojo gsilvis  xyang1 raj_singh lyarwood
17:00:56 <mriedem> o/
17:01:22 <smcginnis> o/
17:01:37 <ildikov> hi :)
17:02:07 <ildikov> let's wait a bit hoping we will have more people around
17:03:12 <ildikov> ok, lets dive in
17:03:18 <ildikov> we have a few patches up for review
17:03:40 <ildikov> in the server side spec there's a comment from patrickeast regarding terminate_connection and ordering
17:03:44 <ildikov> #link https://review.openstack.org/#/c/414632/11/cinder/volume/manager.py
17:04:01 <ildikov> line 4542
17:04:21 <ildikov> mriedem: did we agree on anything on the Nova side about what the ordering of things is when we detach?
17:04:42 <mriedem> umm
17:04:48 <mriedem> as in today or in the future?
17:04:56 <mriedem> i thought we had like one detach call now?
17:04:59 <mriedem> in the future
17:05:17 <ildikov> not in yet and there is one attachment-delete call
17:05:36 <mriedem> from what i remember of johnthetubaguy's spec, we detach with cinder api and then get something back, and pass that to os-brick's disconnect_volume
17:05:47 <ildikov> although my understanding is that Nova will still call out to os-brick for instance
17:05:49 <mriedem> and the issue was if disconnect on the host fails
17:06:06 <hemna> mornin
17:06:16 <ildikov> hemna: morning :)
17:06:19 <mriedem> since we don't have like a terminate_connection - disconnect - os-detach (ack) flow anymore
17:06:37 <smcginnis> jgriffith: Are you around?
17:07:31 * diablo_rojo_phon sneaks in the back of the room
17:09:00 <ildikov> mriedem: ok, I need to re-read that part
17:09:27 * jgriffith shows up late
17:09:56 <ildikov> jgriffith: we're talking about patrickeast's comment and detach/disconnect in general
17:10:34 <ildikov> jgriffith: I mean the one in line 4542 here: https://review.openstack.org/#/c/414632/11/cinder/volume/manager.py
17:11:24 <jgriffith> looking
17:11:51 <jgriffith> oh, yeah; so I can change that
17:11:58 <hemna> yah patrick is correct
17:12:01 <jgriffith> but I have a fundamental problem with the FC stuff at this ponit
17:12:02 <jgriffith> point
17:12:16 <smcginnis> jgriffith: Haven't you always? ;)
17:12:23 <jgriffith> Patrick may be correct, but the FZM stuff is obviously completely f'd up
17:12:23 <hemna> the FCZM code will remove the zone if the target map is passed back in terminate_connection
17:12:48 <jgriffith> it uses it's own interpretation of things and ignores every other driver in the code base
17:13:04 <jgriffith> it's frankly super annoying
17:13:14 <jgriffith> but anyway
17:13:17 <hemna> fczm code doesn't care about non FC drivers, and only looks for the FC related information
17:13:29 <jgriffith> hemna yes, but an interface is an interface!
17:13:38 <hemna> we are changing the terminate_connection behavior, so we need to adjust things accordingly
17:13:56 <jgriffith> You can't just choose to inherit from a base class and change it however you feel in this case
17:14:19 <jgriffith> hemna I'm not aruging this here, I'll work on addressign the FZM stuff
17:14:21 <hemna> some FC drivers don't inherit from the FC base class
17:14:23 <jgriffith> nuf said
17:14:36 <hemna> I've tried many times to get folks to adopt it
17:14:54 <hemna> but there are several FC drivers that are combined with iSCSI drivers
17:15:10 <hemna> instead of having 2 drivers iSCSI and FC, they combine them both into 1 driver
17:15:11 <hemna> :(
17:15:22 <hemna> so there is that too
17:15:33 <jgriffith> hemna yeah and frankly that's a good thing, again I don't want to air this out in this meeting
17:15:41 <jgriffith> not the appropriate time or place
17:15:49 <hemna> ok, just sharing information.
17:15:53 <hemna> since it's relevant
17:17:41 <ildikov> jgriffith: is the plan for attachment_delete to return a True/False to indicate whether there's a shared connection or not?
17:17:54 <jgriffith> ildikov that *was* the plan yes
17:18:11 <jgriffith> ildikov that had been the discussed proposal for a while now, but that's not going to work with the FZM stuff
17:18:20 <jgriffith> at least, not the way it is currently
17:18:47 <ildikov> do we have alternatives?
17:18:54 <jgriffith> not really
17:19:08 <jgriffith> I mean, no real alternatives other than fixing the mess
17:19:27 <jgriffith> and making it work for both.  Ideally making things consistent again
17:19:39 <hemna> wait
17:19:40 <hemna> so
17:19:41 <ildikov> so the current plan is to fix the mess and stick to the above plan?
17:19:49 <hemna> the decorator is on terminate_connection in the driver
17:20:18 <hemna> that code has already been executed by the time _connection_terminate returns
17:20:43 <patrickeast> jgriffith: I kinda like the idea of either changing the fczm code to be smarter with the wrapper function return value or a new driver API that can just like list shared connections or something
17:20:47 <hemna> the drivers are only supposed to return the target map if there are no more connections from the backend to the nova host
17:21:06 <hemna> so I'm not sure anything is wrong here
17:21:30 <hemna> the zoning has already been done by the time that return call is made
17:21:45 <patrickeast> hemna: yep, the fczm can just return a book correctly based on the map it got
17:21:50 <hemna> and drivers are already tracking if there are connections left or not.
17:21:57 <patrickeast> Bool*
17:22:19 <jgriffith> patrickeast I liked "book" better :)
17:22:29 <patrickeast> Lol
17:22:53 <hemna> the zoning code just returns what it got from the driver
17:22:57 <hemna> so I don't see a problem here
17:24:46 <hemna> I don't see a conflict
17:24:50 <hemna> or I'm just slow
17:27:21 <patrickeast> It's backwards is all, the fczm needs to return true when it didn't get a request to tear down zones since the volume connection is still in use on that initiator.... I think
17:27:59 <patrickeast> So like empty map means return true, map with stuff to clean means return false
17:28:31 <patrickeast> Assuming my sleepy logic checks out
17:28:49 <jgriffith> patrickeast yeah, so the "absence" of anything currently means "nothing shared, nothing to do"
17:28:56 <patrickeast> Yea
17:29:01 <jgriffith> patrickeast from the new codes perspective that is
17:29:23 <hemna> the fczm just returns what the driver does
17:29:43 <hemna> so, the driver can simply return it, because it already knows there are no more connections or not.
17:29:52 <hemna> the FCZM's job is to add or remove zones only.
17:29:58 <jgriffith> hemna but you have a return in your decorator as well
17:30:13 <hemna> which is a pass through from the driver
17:30:42 <patrickeast> Right, my proposal is that we make it a little more than a pass though
17:30:56 <patrickeast> So that it complies with the new api
17:30:57 <hemna> to do what?
17:31:20 <patrickeast> To return true or false
17:31:37 <patrickeast> Like the new terminate connection should
17:31:44 <jgriffith> patrickeast +1
17:31:56 <smcginnis> So iscsi drivers will return true/false but FC drivers will return a dict?
17:32:19 <hemna> we are changing every driver to return something different now in terminate_connection ?
17:32:21 <hemna> ?!
17:32:23 <patrickeast> smcginnis: yea, they need to know that the fczm monkeys with things
17:32:47 <smcginnis> patrickeast: Not a big fan of it, but if we can get that to work, I guess I'm fine.
17:32:59 <jgriffith> hemna the idea is to have a consistent interface, which would be kinda nice IMO
17:33:08 <ildikov> jgriffith: +1
17:33:12 <smcginnis> There's already a presumed assumption that FC driver developers know to use the decorator and what to return for the decorator.
17:33:29 <smcginnis> jgriffith: +1 for consistent interface.
17:33:37 <jgriffith> part of why all of this work and things like multi-attach have been difficult is lack of consistency in interfaces
17:33:44 <jgriffith> everybody did "their own thing"
17:33:46 <patrickeast> They could all just return maps of initiators similar to the fczm
17:33:56 <jgriffith> patrickeast and that's fine as well IMO
17:34:16 <jgriffith> patrickeast just so long as it's consistent and we know what it means
17:34:22 <smcginnis> patrickeast: I kind of like that just because then it is also consistent from the driver implementation side.
17:34:42 <patrickeast> Ehh kinda
17:34:57 <jgriffith> the point for me is I would like to get rid of all the snow-flake implementations of attach/detach
17:35:09 <jgriffith> that was the whole point of reworking these to begin with
17:35:38 <jgriffith> I don't care what you do in the driver of course, but the interfaces and what they are perceived to do should be consistent
17:35:47 <hemna> so, we could also change the way drivers interact with the fczm
17:35:56 <hemna> and have the drivers call the fczm remove code manually
17:35:58 <patrickeast> Everyone has their own idea of hosts and initiators, it might be similar for FC where the spec sorta forces it, but on iscsi and other protocols it might get more weird
17:36:00 <hemna> instead of as a decorator
17:36:16 <patrickeast> hemna: +1
17:36:17 <hemna> that would make the driver return from terminate_connection consistent
17:36:27 <smcginnis> My preference is always return an initiator map, or have an explicit call for the FC management.
17:36:41 <jgriffith> personally I would like to see the decorator 86'd
17:36:47 <smcginnis> It is almost universal in new driver reviews that we have to point out FC drivers need a decorator and what they need to return for it
17:36:52 * jgriffith is not a fan
17:37:12 <patrickeast> I learn towards removing the decorator if we aren't going to modify it transparently
17:37:21 <hemna> this is one of the reasons why I wanted FC drivers to extend the FC base class
17:37:32 <xyang1> some FC drivers don't use the zone manager
17:37:32 <hemna> then we could modify the base FC class and everyone gets the changes for free
17:37:46 <hemna> xyang1, yah and those FC drivers don't work :)
17:37:52 <hemna> in a real openstack deployment
17:38:16 <xyang1> hemna: they work using their own zone mgmt
17:38:29 <smcginnis> Well... we actually use open zoning quite a bit since the array handles isolating the target and initiator.
17:38:47 <smcginnis> So fczm just adds complexity.
17:38:49 <jgriffith> smcginnis +1 same here
17:38:52 <hemna> ok do you want me to look at a follow up patch?
17:38:59 <jgriffith> but I'm still resisting actually supporting it anyway
17:39:04 <jgriffith> :)
17:39:10 <smcginnis> jgriffith: :)
17:39:22 <hemna> well some arrays don't have fabric management
17:39:35 <jgriffith> hemna honestly I'd prefer we have an answer and include it in the proposed patch
17:39:39 <smcginnis> hemna: Right, some I think absolutely need it.
17:39:51 <jgriffith> there have been WAY too many cases where we say "follow up" and it never happens, or doesn't work
17:39:54 <hemna> I don't think it would be hard to adjust things
17:40:00 <jgriffith> leading to more and more 1/2 in 1/2 out impls
17:40:09 <hemna> ok do what you want
17:40:09 <smcginnis> SO what are our options? All return initiator map or explicit FCZM calls in the driver?
17:40:12 <hemna> I was just offering help
17:40:40 <hemna> I'd prefer explicit fczm calls in initiialize_connection and terminate_connection.
17:41:53 <smcginnis> hemna: I kind of lean that way too. Then we can have a cleaner interface and more explicit code in the drivers.
17:42:07 <smcginnis> patrickeast: Was that what you were saying too?
17:42:42 <patrickeast> Yep
17:43:16 <patrickeast> Thats my preference if we are going to make changes to all the FC drivers
17:43:30 <smcginnis> jgriffith: Work for you? I just wonder how we're going to get all the drivers updated, but I guess we will need to either way.
17:43:36 <jgriffith> sure
17:45:47 <patrickeast> IMO just fiddling with decorator return values isn't so bad for an interim solution though, wouldn't require additional driver patches to work with the new APIs
17:46:16 <patrickeast> But if the concern is that it's too confusing I'm OK with not doing it
17:46:26 <ildikov> I just wanted to ask what's the roadmap for this what you all seem to agree on?
17:47:01 <ildikov> just to clarify this for less knowledgable people on the driver side like me :)
17:47:18 <patrickeast> To get all of them updated?
17:47:44 <ildikov> to get to the next step with the new attach/detach API
17:48:10 <ildikov> if everything needs to be updated before anything happens with that than yes
17:49:10 <ildikov> sorry for killing the joy with administrational questions :/
17:49:22 <patrickeast> Lol
17:49:54 <jgriffith> patrickeast I'm fine with fiddling with the decorator to make everything consistent
17:50:14 <jgriffith> patrickeast and if later we want to dump the decorator and force drivers to impl the method that's fine
17:50:28 <jgriffith> patrickeast my only requirement is consistency
17:50:39 <jgriffith> don't care how that's done right now
17:50:55 <patrickeast> jgriffith: sounds good to me
17:51:08 <jgriffith> I was just confused on what if any initiators *needed* that return info
17:51:20 <jgriffith> if they don't need it anyway, was confused on why it was there :)
17:51:20 <mriedem> so any updates on getting nova to use cinder v3 in the last <10 min?
17:51:55 <ildikov> mriedem: did you see the latest patch scottda uploaded last week?
17:52:34 <ildikov> mriedem: that solves the endpoint not found error
17:53:16 <ildikov> mriedem: although I'm not sure what's the preferred way of setting the api micorversion for the cinderclient for instance
17:53:29 <mriedem> ildikov: do you mean this? https://review.openstack.org/#/c/420201/
17:54:09 <ildikov> mriedem: yeap, sorry didn't have the link handy
17:54:11 <mriedem> re: the microversion to use, i commented on that here https://review.openstack.org/#/c/385682/3/nova/volume/cinder.py@100
17:54:19 <mriedem> nova definitely doesn't want to use the latest available
17:54:32 <mriedem> nova needs to say, i want 3.x, is that available?
17:54:50 <mriedem> if nova needs 3.24 and it's not available, then we can't do whatever we needed 3.24 for
17:55:12 <mriedem> the client has to opt into whatever version it's going to use so it can handle backward compat issues
17:55:43 <ildikov> mriedem: did you see the comment about the config option here: https://review.openstack.org/#/c/420201/2/nova/volume/cinder.py ?
17:55:53 <ildikov> line 98
17:56:41 <mriedem> we don't want a config option
17:57:02 <ildikov> that was my thought too
17:57:25 <ildikov> but wasn't sure how it is supposed to work in general
17:57:43 <mriedem> nova is going to hardcode the microversion it needs
17:57:52 <mriedem> if cinder can provide it, we use it, else we fallback to the old flow
17:58:31 <ildikov> ok, that sounds good to me
17:59:30 <mriedem> btw http://logs.openstack.org/01/420201/2/check/gate-tempest-dsvm-neutron-full-ubuntu-xenial/e66fd9a/logs/screen-n-api.txt.gz?level=TRACE
17:59:45 <mriedem> so i guess he's around the endpoint issue which is good
18:00:16 <ildikov> I think the get_version patch is not merged yet in the client
18:00:29 <mriedem> we're at time btw
18:00:49 <ildikov> mriedem: https://review.openstack.org/#/c/420119/
18:01:03 <ildikov> mriedem: yep, we are, tnx
18:01:16 <ildikov> anything urgent for this meeting?
18:02:09 <ildikov> ok thanks everyone, let's catch up on the Cinder channel if there's anything
18:02:12 <smcginnis> ildikov: Thanks.
18:02:28 <ildikov> otherwise chat next week!
18:02:33 <ildikov> #endmeeting