17:00:22 <ildikov> #startmeeting cinder-nova-api-changes
17:00:29 <openstack> Meeting started Thu Jun  2 17:00:22 2016 UTC and is due to finish in 60 minutes.  The chair is ildikov. Information about MeetBot at http://wiki.debian.org/MeetBot.
17:00:30 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
17:00:32 <openstack> The meeting name has been set to 'cinder_nova_api_changes'
17:00:40 <ildikov> scottda DuncanT ameade cFouts johnthetubaguy jaypipes takashin alaski e0ne jgriffith tbarron andrearosa hemna erlon mriedem gouthamr ebalduf patrickeast smcginnis diablo_rojo gsilvis
17:00:45 <scottda> hi
17:00:55 <mriedem> o/
17:00:57 <jgriffith> o/
17:01:10 <patrickeast> hey
17:01:35 <andrearosa> o/
17:01:50 <ildikov> hi everyone
17:01:55 <hemna> hey
17:02:33 <ildikov> we have our etherpad #link https://etherpad.openstack.org/p/cinder-nova-api-changes more or less up to date
17:02:34 <hemna> shameless plug....I'm still looking for a job. :P
17:03:17 <hemna> so I'm able to get BFV 'working'
17:03:22 <hemna> with reserve_volume
17:03:26 <ildikov> hemna should I add it to the etherpad as well?
17:03:43 <ildikov> :)
17:03:53 <ildikov> hemna: that's good news!
17:03:56 <hemna> but I'm having an issue reporting the an exception back up the stack when the volume is already in use.
17:03:59 <hemna> I'm working on that
17:04:12 <hemna> InvalidInput(u'Invalid input received: Invalid volume: Volume status must be available to reserve.
17:04:15 <ildikov> hemna: did you remove the second check_attach?
17:04:32 <hemna> not sure why I'm getting recursive messages as the exception message at this point
17:04:39 <hemna> but at least I have the http response code correct
17:04:48 <hemna> so it's not a nova api change (other than a message change)
17:04:56 <hemna> ildikov, kinda
17:05:06 <hemna> I just blindly removed the check_attach call
17:05:07 <hemna> :P
17:05:08 <mriedem> hemna: that won't work
17:05:08 <jgriffith> hemna: is there a bug for what you're working on?
17:05:12 <hemna> mriedem, ok
17:05:21 <mriedem> hemna: when you get to the compute in BFV
17:05:25 <mriedem> it's a cast by then
17:05:29 <mriedem> so you've lost the api response to the user
17:05:41 <ildikov> jgriffith: there are a few race condition type of bug reports, this should help in those
17:05:42 <hemna> ?
17:05:45 <mriedem> so you'd basically fail and the instance would go into error state
17:05:45 <hemna> I'm not following
17:05:55 <hemna> the API response isn't different to the user as it is now
17:06:01 <hemna> other than the text of the message
17:06:06 <jgriffith> ildikov: well, I'm looking specifically to understand what is being discussed without taking up meeting time :)
17:06:16 <mriedem> i guess i'm confused on where you're calling reserve_volume in that flow
17:06:21 <mriedem> hemna: from the API or from the compute?
17:06:26 <hemna> mriedem, API
17:06:29 <ildikov> hemna: you have the reserve call, where the first check_attach was in validate_bdm?
17:06:30 <hemna> I'll post up the patch today
17:06:33 <mriedem> hemna: oh, in that case, nevermind
17:06:43 <hemna> I'm replacing the existing check_attach call in the API
17:06:47 <hemna> with reserve_volume
17:06:58 <mriedem> there are some wrappers on the reserve method in the nova.volume.cinder api method
17:07:00 <mriedem> to handle errors
17:07:04 <ildikov> jgriffith: with hemna we're aiming at removing the check_attach calls from Nova as a first step on our journey to refactor
17:07:05 <mriedem> maybe something missing in those error decorators
17:07:15 <hemna> and detecting the case when the volume is 'in-use' and raising the same exception as it does today, just with a different message.
17:07:29 <hemna> I can put up a gist
17:07:40 <hemna> it's total hack code at this point
17:08:28 <ildikov> if you replaced that API check_attach that should be the right direction
17:08:29 <jgriffith> ildikov: ok... just curious as to why
17:08:33 <hemna> https://gist.github.com/WaltHP/12d86a9edf7c21b50d2aff443056dfc6
17:08:40 <hemna> that's on top of my existing patch
17:08:54 <ildikov> jgriffith: the check_attach call is unnecessary for Nova, it's kind of a left over from old times
17:09:16 <ildikov> jgriffith: but it's a change that can be handled in a reasonable timeframe, therefore it seems worth starting with
17:09:26 <hemna> jgriffith, the existing nova/volume/cinder.py check_attach looks at the internal state of the cinder volume
17:09:28 <jgriffith> ildikov: okie-dokie
17:09:30 <hemna> trying to eliminate that
17:09:42 <hemna> and just rely on Cinder to do the check that already exists in os-reserve
17:09:45 <hemna> it's the same check
17:09:56 <jgriffith> ildikov: do we have an agenda today or is it open-discussion?
17:10:04 <ildikov> jgriffith: so basically we're trying the baby steps approach to have some progress, while discussing more radical changes, like the new API calls you're working on
17:10:15 <hemna> ok I'm done if we want to move on
17:10:20 <hemna> I just wanted to give a status
17:10:47 <mriedem> for the multiattach spec,
17:10:51 <mriedem> johnthetubaguy:  is +2 on it,
17:11:00 <mriedem> i got back to the world <48 hours ago
17:11:02 <ildikov> jgriffith: the agenda is to discuss the ongoing items, like the one hemna brought up, bring up your work item, plus get the multi-attach spec in Nova accepted by mriedem
17:11:19 <mriedem> i will get through the spec today
17:11:27 <jgriffith> link 2 spec?
17:11:35 <mriedem> https://review.openstack.org/#/c/304681/
17:11:43 <jgriffith> mriedem: gracias
17:12:27 <ildikov> hemna: cool, tnx, it looks good
17:13:16 <ildikov> mriedem: the spec is aiming at capturing what we need to deal with and points at the etherpad as well
17:13:49 <ildikov> mriedem: I tried not to add too many details about the dependencies there
17:14:33 <ildikov> if nothing for the spec, I think we can move to what jgriffith is working
17:16:12 <jgriffith> ildikov: ok, I guess I'll just chat about some things
17:16:18 <ildikov> here is the link to the review: https://review.openstack.org/#/c/323571/
17:16:25 <jgriffith> I have 3 different approaches going on currently:
17:16:51 <jgriffith> 1. Just massage the existing flow and calls:  https://review.openstack.org/#/c/320721
17:16:54 <jgriffith> Ugly hacky stuff
17:17:35 <jgriffith> 2. Introduce some new calls:  https://review.openstack.org/#/c/323571/ but leverage all the old stuff (helps with unit tests and back compat)
17:17:51 <jgriffith> 3. Write new stuff:  https://github.com/j-griffith/cinder/commit/4feabd244e755dd02f930c1efa2c4d59f89b0c2e
17:18:29 <jgriffith> 3 is my favorite :)  I'm also working on Nova changes as POC to go with it.  Basically want to have a full working solution with multi-attach using option 3 in another week
17:19:12 <jgriffith> I can work on all, none or one of these
17:20:00 <jgriffith> obviously they're not complete, nor functional as they are right now.  Like the copy/paste of _get_connection_count etc
17:20:04 <ildikov> 2 contains parts from 3, right?
17:20:35 <ildikov> or at least the concept of the new calls is going to that direction, IIUC
17:20:39 <jgriffith> ildikov: yes... they're all related
17:20:57 <ildikov> ok, cool
17:21:17 <jgriffith> ildikov: but the more time I spend looking at the attach/detach code in Cinder, the more I realize it's become a bit of a train wreck
17:21:22 * hemna is a fan of 3
17:21:34 <ildikov> I like that we're trying to simplify the interaction between Cinder and Nova as much as possible so having one call as opposed to three or more
17:21:37 <scottda> jgriffith: Will you be pushing #3 up to gerrit ?
17:21:44 <jgriffith> and taking the opportunity to do something like 3 (while it may be scary to some) eliminates the issues folks have with race-conditions etc
17:21:51 <jgriffith> scottda: nope, I refuse :)
17:21:55 <jgriffith> scottda: of course
17:21:56 <scottda> haha
17:22:03 <hemna> :)
17:22:13 <jgriffith> scottda: but it's got a ways to go before being even WIP status in Gerrit IMO
17:22:19 <mriedem> from a nova perspective, they are all bound by microversions so not sure i have an opinion
17:22:20 <scottda> cool
17:22:30 <jgriffith> and I didn't want to bombard everybody with yet another gerrit patch yet
17:22:34 <ildikov> jgriffith: sure, that I completely get, I'm wondering about the amount of changes in NOva and the timeframe we would need to introduce them
17:22:40 <mriedem> and really would start as only calling those if nova is given a multiattach volume (i think)
17:23:00 <jgriffith> mriedem: so that's a possibility if preferred
17:23:17 <jgriffith> mriedem: I do plan to provide a nova patch to go with it FWIW
17:23:40 <jgriffith> mriedem: although it will surely not be up to standards with unit tests and some of the versioning stuff
17:23:52 <jgriffith> mriedem: I at least wanted to take a swag at a proof of concept
17:24:04 <mriedem> sure
17:24:05 <mriedem> that's fine
17:24:38 <jgriffith> mriedem: my question to you though would be... out of the choices, what has a chance of landing in N
17:24:51 <mriedem> well those 3 are all cinder changes
17:25:17 <ildikov> with smaller or larger Nova impacts
17:25:17 <jgriffith> mriedem: hehe.. yeah, but they will all require something on the Nova side to utilize them
17:25:33 <mriedem> so #1 is existing APIs but new behavior, right?
17:25:34 <jgriffith> mriedem: in other words, I want to finish and release multi-attach in N and get past all of this
17:25:37 <ildikov> #3 is the rethinking of the attach concept
17:25:38 <patrickeast> jgriffith: whats the scale difference as far as nova changes required between 1, 2 and 3?
17:26:20 <jgriffith> mriedem: pretty much, yes... but it's super ugly in terms of "optional" args/params (I think it's more trouble than it's worth)
17:26:26 <mriedem> so, a thing that might suck on the nova side is if volume['multiattach'], and we have to call a different API - however, that can all be abstracted
17:26:40 <jgriffith> mriedem: I'd suggest we don't do that if possible
17:26:42 <mriedem> yeah
17:26:59 <mriedem> so i'm not opposed to new apis that are better
17:26:59 <jgriffith> mriedem: I've been looking at just replacing the nova calls with the new calls in option-3
17:27:09 <ildikov> mriedem: in my understanding these new API calls are not for multiattach
17:27:18 <jgriffith> patrickeast: they're in ascending order in terms of amount of change in Nova
17:27:26 <mriedem> e.g. cinder provides new api in microversion 2.6 which is the preferred way to do this, and consider attach/detach deprecated at that point
17:27:29 <mriedem> so clients would start moving
17:27:42 <jgriffith> mriedem: ok... that's certainly fair
17:27:49 <patrickeast> jgriffith: yea, but like... from 2 to 3 are we talking incremental amount of differences... or like 10x?
17:27:49 <ildikov> mriedem: I mean they would be prepared to handle multiattach as well, but it's about to remove the reserve, initialize_connection, attach chain in my understanding
17:27:56 <jgriffith> mriedem: I was more concerned that we were just already too late in the cycle for this sort of thing
17:28:37 <mriedem> my preference is to take the time to do the right thing for the long-term, whatever that is
17:28:43 <jgriffith> mriedem: and YES, exactly my plan would be to add the deprecation marker to reserve, initialize_connection and attach etc
17:28:45 <patrickeast> mriedem: +1
17:28:48 <mriedem> if that's using new better cleaner APIs in cinder, i think we do that regarldess of schedule
17:28:57 <jgriffith> mriedem: ok, that's the best answer I could hope fo
17:28:59 <jgriffith> for
17:29:02 <mriedem> because i don't want to shoehorn #1 and introduce more tech debt
17:29:16 <hemna> I think the long term thing is option #3
17:29:21 <jgriffith> mriedem: yes, that's the sort of thing that created the mess we have now to begin with
17:29:45 <hemna> I'd like to remove the check_attach stuff short term and rely on reserve_volume
17:29:48 <jgriffith> It's unfortunate I didn't recognize some of this sooner or as it was happening
17:29:59 <hemna> that should make it easier to migrate to #3, since it's all cinder side checks
17:30:01 <ildikov> mriedem: the regardless of schedule means being able to merge changes in Nova regardless of schedule?
17:30:26 <ildikov> mriedem: also totally agree on the long term thinking
17:30:30 <jgriffith> ildikov: hehe.. I think it means, "do the right thing even if it takes another cycle"
17:30:40 <hemna> jgriffith, +1
17:30:41 <mriedem> what jgriffith said
17:30:52 <ildikov> jgriffith: if it's only one...
17:30:58 <hemna> ildikov, it's never one
17:31:18 <ildikov> do we have any incremental choice here?
17:31:34 <jgriffith> ildikov: I tihnk incremental in this case is bad
17:31:44 <mriedem> +1
17:31:54 <mriedem> the problem with incremental is you bake in the badness, and then it's hard to move
17:31:58 <jgriffith> ildikov: I also think that #3 is incremental.. because to start it's mostly wrapping existing cinder calls
17:32:02 <mriedem> not only technically, but because people don't want to do it
17:32:10 <jgriffith> mriedem: +1
17:33:05 <ildikov> sure, I'm not saying I'm against the good things, I'm just sometimes invited to customer meetings... :S
17:34:31 <hemna> ok so anyway, what is the plan then?
17:34:45 <hemna> I vote for #3
17:35:02 <jgriffith> I'd propose I finish up POC's of #3 for nova and cinder and post them within the next week
17:35:03 <ildikov> should we do a vote?
17:35:05 <jgriffith> does that work?
17:35:13 <jgriffith> ildikov: oh.. yeah... we can vote :)
17:35:32 <ildikov> jgriffith: what is the aim from the POC?
17:35:53 <ildikov> jgriffith: is it about whether this works or more about how hard to get it work?
17:35:56 <jgriffith> ildikov: ummm... Proof Of Concept?
17:36:22 <jgriffith> ildikov: yes, see if people vomit all over it, or if it's acceptable and we all pitch in to finish/merge it
17:36:23 <mriedem> point is to see what the changes look like and see if there are red flags
17:36:27 <mriedem> that make it a non-starter
17:36:30 <ildikov> jgriffith: I know the term :)
17:36:35 <jgriffith> mriedem: +1
17:36:54 <jgriffith> ildikov: sorry... mriedem 's answer was better
17:37:51 <ildikov> ok, let's see the PoC then
17:38:22 <mriedem> fwiw if the nova spec is generic enough to know we're playing with new things here,
17:38:22 <ildikov> jgriffith: is there any work item where either of us could join?
17:38:31 <mriedem> i'm fine with getting that in today - just need the time to look at it
17:38:46 <mriedem> ildikov: testing probably
17:39:18 <ildikov> mriedem: the plan was to have it that generic, so I referred to having new Cinder API microversion as we cannot avoid that
17:39:26 <jgriffith> ildikov: Give me til towards end of next week to get patches up.  Then yes, testing
17:39:32 <ildikov> mriedem: the rest is supposed to be on us
17:39:56 <ildikov> jgriffith: ok
17:41:26 <hemna> coolio
17:41:57 <ildikov> should we continue with trying to remove check_attach, etc?
17:42:51 <ildikov> just to maintain a plan B if #3 would not work out for any reason
17:43:17 <mriedem> removing check_attach still applies for those that aren't at the microversion required to use the new stuff
17:43:20 <mriedem> so seems worthwhile
17:43:24 <hemna> ildikov, I think the removal of the check_attach stuff is a bug fix IMHO.
17:43:33 <ildikov> ok, cool
17:43:34 <hemna> BFV currently doesn't even call reserve_volume
17:43:41 <hemna> :(
17:44:03 <ildikov> yeah, don't even remind me, I checked it zillion times as I was sure I'm not able to debug properly...
17:46:13 <ildikov> ok, do we have anything else for today to discuss?
17:47:31 <ildikov> so as a quick summary we have the check_attach removal and the #3 prototyping activity ongoing
17:48:29 <ildikov> there was also an item about testing Cinder migrate
17:48:39 <mriedem> yeah are there any updates on that testing?
17:48:52 <ildikov> although for the above two that does not seem to be a direct requirement
17:49:25 <mriedem> umm
17:49:30 <mriedem> swap_volume uses check_attach right?
17:49:50 <ildikov> mriedem: that checks everything before check_attach TBH
17:49:58 <mriedem> so, it would be good to have some kind of integration test coverage before changing all that, but it could be tested manually too
17:50:11 <hemna> yah I'd like to have that testing in place
17:50:17 <mriedem> ok, i do seem to remember a bunch of checks in the compute api for swap_volume
17:50:26 <ildikov> scottda: do you have any news about testing?
17:50:44 <scottda> Sorry had to run afk...
17:50:53 <scottda> I'm making progress.
17:51:07 <ildikov> mriedem: I tried to refactor that when I touched check_attach for multi-attach, I partially failed due to the order of checks there...
17:51:30 <scottda> I need to add plumbing to have cinder tests do Nova create and volume attach.
17:51:51 <scottda> Hopefully that's not took painful.
17:52:07 <mriedem> scottda: it should be a scenario test
17:52:10 <mriedem> not a tempest api test
17:52:14 <scottda> S/took/too
17:52:22 <mriedem> scenario tests extend a base manager that does a lot of that kind of stuff for you
17:52:34 <scottda> mriedem: OK. I'll have a look.
17:54:31 <mriedem> hemna: ildikov: btw, i think i see where BFV with source=volume calls check_attach, but we can talk after the meeting
17:54:59 <ildikov> mriedem: it calls it two times if I;m not mistaken
17:55:01 <mriedem> oh nvm, it calls check_attach but not reserve
17:55:04 <hemna> do_check_attach defaults to true
17:55:12 <hemna> mriedem, correct
17:55:16 <hemna> that's what I want to change
17:55:19 <ildikov> mriedem: yeah, no reserve
17:55:32 <hemna> and in the process eventually remove check_attach completely.
17:55:35 <hemna> if possible.
17:56:34 <ildikov> it should be, having two separate modules maintaining one single state machine is really not ideal
17:56:42 <ildikov> just to state the obvious :)
17:57:24 <ildikov> ok, we're almost out of time
17:57:53 <ildikov> we have the next meeting on Monday, I think it would be good to briefly sync up and get back to the normal schedule
17:58:38 <ildikov> any other last minute items for the meeting?
17:59:11 <mriedem> nope
17:59:20 <ildikov> ok, then thanks everyone, we had a good chat
17:59:35 <scottda> Thanks
17:59:44 <ildikov> and talk to you on Monday!
18:00:01 <ildikov> or earlier on either channel to address implementation details :)
18:00:15 <ildikov> #endmeeting