17:00:04 <ildikov> #startmeeting cinder-nova-api-changes
17:00:05 <openstack> Meeting started Mon Jun 13 17:00:04 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:06 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
17:00:08 <openstack> The meeting name has been set to 'cinder_nova_api_changes'
17:00:15 <scottda> hi
17:00:33 <ildikov> scottda ildikov DuncanT ameade cFouts johnthetubaguy jaypipes takashin alaski e0ne jgriffith tbarron andrearosa hemna erlon mriedem gouthamr ebalduf patrickeast smcginnis diablo_rojo gsilvis
17:00:38 <ildikov> hi :)
17:00:42 <mriedem> o/
17:00:47 * alaski lurks
17:00:47 * johnthetubaguy lurks
17:00:57 <ildikov> etherpad: #link https://etherpad.openstack.org/p/cinder-nova-api-changes
17:01:02 <patrickeast> hey
17:01:27 <ildikov> sorry for skipping last week
17:03:15 <ildikov> I thought two items for today, which are the refactoring and its impacts that jgriffith is working on and the other would be the migration test, I think scottda has updates on that one
17:03:50 <scottda> I've a migrations-with-BFV patch up: https://review.openstack.org/#/c/326681
17:04:09 <scottda> IT needs a devstack change (devstack-gate) which I need to put up and list as a dependency.
17:04:28 <scottda> But is ready for testing if anyone is interested.
17:04:59 <scottda> Once that is in, we'd want another test with multi-attach and cinder migrate.
17:05:27 <scottda> But we said we wouldn't support multi-attach with BFV, so that'd be slightly different. But not more complicated.
17:05:38 <ildikov> that looks good
17:05:55 <ildikov> I think we will not support booting from a multi-attach volume
17:06:09 <ildikov> but we will support attaching a multi-attach volume at boot time
17:06:29 * amrith lurks
17:06:31 <ildikov> so we have a negative test and a positive as well with the non-bootable one
17:06:57 <ildikov> scottda: those few jobs are failing due to the missing dependency?
17:07:07 <hemna> hey guys, I'm working on the grenade failures with os-brick right now
17:07:29 <scottda> ildikov: Actually, one failure was a requirements issue for something that looked unrelated....
17:07:30 <jgriffith> o/
17:07:36 <jgriffith> sorry I'm a bit tardy
17:08:09 <ildikov> hemna: ok
17:08:13 <scottda> ildikov: The other seemed obscure as well. I did a rebase just to see if it was something fixed in tempest itself.
17:08:50 <jgriffith> ildikov: scottda wait... not support multi-attach BFV?
17:08:51 <ildikov> scottda: ok, got it
17:09:11 <scottda> ildikov: Frankly, I'm learning as I go with regards to getting a Tempest test merged. Anyone with more expertise, let me know if you're available to bother with questions.
17:09:15 <jgriffith> ildikov: scottda I though one of the primary use cases you wanted multi-attach for was to help live-migration of volume backed instances?
17:10:23 <ildikov> jgriffith: we agreed to skip boot from multi-attach volume with mriedem and johnthetubaguy I think during Mitaka
17:10:44 <scottda> jgriffith: At least for the first pass at multi-attach.
17:11:15 <ildikov> scottda: I'm not a big expert either, but I'll try to take a look and be helpful
17:11:16 <mriedem> i honestly don't exactly remember the reason why anymore
17:11:36 <ildikov> mriedem: TBH I think it simply felt odd
17:11:38 <scottda> I think we just didn't like the complexity, and were unsure of the corner cases.
17:12:01 <mriedem> nova won't create a multiattach volume when booting, that would require API changes in nova
17:12:27 <mriedem> i'm not sure what bringing a multiattach volume to attach at boot time would do
17:12:33 <johnthetubaguy> +1
17:12:58 <mriedem> except maybe not landing on a host that could attach that kind of volume, i.e. mitaka node at this point
17:13:08 <ildikov> I think simply attaching it, but not booting from it should be perfectly fine
17:14:51 <ildikov> well, placement is an issue in other cases as well
17:15:03 <johnthetubaguy> so I should check something here
17:15:14 <johnthetubaguy> is that booting from an already created multi-attach
17:15:27 <johnthetubaguy> or creating a multi-attach from an image during the boot process
17:15:29 <johnthetubaguy> or both
17:15:40 <mriedem> "or creating a multi-attach from an image during the boot process" - we're not doing thta
17:15:52 <mriedem> you'd have to tell nova in the BDM dict to create multiattach=True which is more proxy
17:16:02 <ildikov> right
17:16:15 <mriedem> providing an existing multiattach volume to attach at boot time should be fine
17:16:21 <mriedem> it's the same as attaching it to an existing instance
17:16:35 <mriedem> most of this is covered in http://specs.openstack.org/openstack/nova-specs/specs/newton/approved/multi-attach-volume.html#rest-api-impact
17:16:57 <ildikov> and I think I still have the code for failing when we're trying to boot from an existing multi-attach volume
17:17:06 <mriedem> i think we can get around the placement issues if we just block in the nova-api that you can't do anything with multiattach volumes until all of the computes in the deployment are new enough to handle it
17:17:10 <ildikov> source and dest is volume and bootindex is 0
17:17:15 <mriedem> ildikov: yup, that's in the spec
17:18:15 <ildikov> yeap, checking that will help and then placement will only have to check whether we have libvirt or not
17:18:23 <mriedem> well,
17:18:51 <mriedem> unless we extend the compute capabilities filter or add a new scheduler filter, i'm not sure that's going to happen
17:19:06 <mriedem> i have a feeling that non-kvm deployments of openstack will just block this via policy
17:19:17 <mriedem> johnthetubaguy: is that what rax is/would do?
17:19:47 <mriedem> probably on the cinder side so you can't create the multiattach volume to begin with
17:19:59 <ildikov> that would be the easiest
17:20:05 <ildikov> I think
17:20:28 <mriedem> anyway, we're kind of getting off in the weeds on things that have already been discussed and are for the most part in the spec
17:20:45 <ildikov> although we need to get there to have the Cinder changes in place to get to enable multi-attach
17:21:03 <ildikov> jgriffith: can you give us an update on how the refactoring work goes?
17:21:14 <jgriffith> ildikov: sure
17:21:27 <jgriffith> ildikov: was hoping to get feedback on the code
17:21:45 <ildikov> I added the links to the etherpad for the already uploaded patches
17:21:58 <jgriffith> ildikov: I'm working on the detach side of things in Nova now
17:22:36 <jgriffith> attach is working as expected, haven't cleaned out some of the things like calling initialize to get connection info
17:22:52 <jgriffith> but I think that would be good follow up work
17:23:05 <jgriffith> want to focus on the base attach/detach cases for now
17:23:17 <ildikov> you mean you initialize_connection twice on Cinder side with the new code?
17:23:24 <jgriffith> ildikov: NO
17:23:47 <jgriffith> ildikov: I mean there's a few places where Nova uses initialize_connection calls to Cinder to re-establish connection info
17:23:49 <ildikov> yeah, I agree it would be good to have the basic scenarios work before jumping on the more difficult ones
17:23:58 <jgriffith> ildikov: with no intent of performing an attach
17:24:23 <ildikov> jgriffith: ah, ok, I wasn't sure what you meant by missing clean up
17:24:25 <jgriffith> ildikov: ie the volume is already attached, but they want to "refresh" the info
17:25:03 <mriedem> is there going to be a problem with nova calling os-initialize_connection in non-attach cases?
17:25:20 <jgriffith> ildikov: FWIW these changes will just support multi-attach for the most part without Nova really having to be involved (except for the libvirt setting)
17:25:22 <ildikov> jgriffith: right, got it, I think there will be a few cases where we need to figure out what to do based on the instance actions johnthetubaguy described us a few meetings ago
17:25:36 <jgriffith> mriedem: no... but I'd like to eventually make that go away
17:25:44 <mriedem> jgriffith: with a new cinder api to get connection info?
17:25:57 <jgriffith> mriedem: I wrote the code currently in such a way that all of the *old* calls work exactly as they did before
17:26:06 <ildikov> jgriffith: yeah, I expected that to happen as we want to remove the handling of the volume state machine from Nova
17:26:17 <jgriffith> mriedem: https://review.openstack.org/#/c/327408/
17:26:25 <jgriffith> mriedem: I'm just using wrappers of things for now
17:26:51 * johnthetubaguy shakes fist at his internet connection
17:27:05 <jgriffith> mriedem: that way it's relatively seemless, and we can continue progressing all the various implementation details over time
17:27:07 <johnthetubaguy> mriedem: yeah, +1 on both counts, we just block until all compute nodes are upgraded
17:27:27 <jgriffith> mriedem: rather than a single "big bang" wanted a phased approach that just got us over the API hump
17:28:09 <jgriffith> mriedem: note the (outdated) gist of the nova changes that go with that patch
17:28:39 <jgriffith> mriedem: I should have something at least suitable to post as a WIP to Nova shortly and link the cinder, cinderclient patches to it
17:31:13 <ildikov> jgriffith: you already have the snippets up for attach, right?
17:31:16 <mriedem> jgriffith: ok, posted a comment in your change
17:31:21 <jgriffith> ildikov: yes
17:31:25 <mriedem> jgriffith: would nova-api be calling create_attachment now?
17:31:25 <jgriffith> mriedem: looking
17:31:27 <mriedem> rather than the compute?
17:31:45 <mriedem> it'd have to be compute i guess
17:31:47 <mriedem> to pass the connector dict
17:31:53 <ildikov> jgriffith: cool, I planed to play with it a bit in devstack
17:32:19 <jgriffith> mriedem: yeah... so your points are part of what led me to this wrapper approach
17:32:32 <jgriffith> mriedem: https://gist.github.com/j-griffith/ded4a08c488f0d90e90fff53afb2d7f1
17:32:48 <jgriffith> so for now, I only address things in the base attach case
17:33:20 <jgriffith> mriedem: this way I don't change anything like swap_volume or the case with Cells etc
17:34:05 <jgriffith> mriedem: my thought would be start with the base attach/detach cases.  Get that merged, then do follow up patches to start hitting all the individual *special* cases
17:34:20 <jgriffith> mriedem: I mostly want to keep the patch sizes manageable
17:34:33 <mriedem> jgriffith: with https://gist.github.com/j-griffith/ded4a08c488f0d90e90fff53afb2d7f1 we'd still be reserving the volume in nova-api
17:34:35 <jgriffith> mriedem: then deprecate the old calls, and mark them for removal next cycle
17:34:39 <mriedem> would that make create_attachment fail?
17:35:19 <jgriffith> mriedem: nope, it'll still work because I removed that step :)
17:35:22 <ildikov> jgriffith: I would assume if we have more info stored on Cinder side along with the way more simpler API then things should work out for those *special* cases as well, but agreed on adressing them one-by-one
17:35:54 <jgriffith> mriedem: That's something I should clean up a bit thought TBH
17:36:24 <jgriffith> mriedem: I considered removing it from the nova-api and baking it in here, but instead just left it alone and instead "use" it
17:36:46 <ildikov> mriedem: I think the aim is to replace reserve, initialize and attach with the single create_attachment call
17:36:58 <jgriffith> mriedem: I still think there's value in reserve to avoid some possible race conditions
17:37:06 <ildikov> mriedem: and by having only one call kill the race conditions as well
17:37:29 <mriedem> yeah i only bring it up because you can have mitaka computes that aren't using create_attachment
17:37:37 <jgriffith> ildikov: perhaps eventually.  for now I'm just replacing initialize_connection and attach
17:37:41 <jgriffith> mriedem: +1
17:37:50 <jgriffith> mriedem: that's why it ended up that way
17:37:57 <mriedem> but again, nova can check the min compute service version in the deployment and make decisions based on that
17:37:59 <ildikov> jgriffith: ok, then I'm a step ahead it seems :)
17:39:09 <jgriffith> mriedem: I suppose ya'll will want something to deal with the nova-newton, cinder-mitaka... I didn't include that in my gist, but will look at making it happen
17:40:25 <mriedem> yeah, microversion on the cinder side
17:40:34 <mriedem> nova will need to know if cinder is new enough to call create_attachment
17:41:20 <jgriffith> mriedem: ok, honestly I wanted to do this without using those, but no reason not to use them
17:41:32 <jgriffith> mriedem: will just mean I have to finally figure them out and how they really work
17:41:33 <jgriffith> :)
17:42:02 <ildikov> jgriffith: are there any items any of us can help with?
17:42:40 <jgriffith> ildikov: comment on the code that's there, add notes on what you might propose for micro-versions
17:42:53 <jgriffith> ildikov: once I get the nova WIP posted test
17:43:07 <ildikov> jgriffith: I'm not saying I'm friends with microversions, but I'm happy to give it a try :)
17:43:15 <ildikov> jgriffith: ok, cool, will do
17:43:19 <jgriffith> ildikov: and at that point honestly anybody that wants to joint-contribute on the nova side inparticular that would be great
17:43:54 <mriedem> there is obviously a cinderclient change that's needed also
17:44:01 <mriedem> https://gist.github.com/j-griffith/ded4a08c488f0d90e90fff53afb2d7f1#file-gistfile1-txt-L98
17:44:04 <scottda> jgriffith: I'll add some notes for microversions. We are friends.
17:44:10 <ildikov> jgriffith: sounds good, tnx!
17:44:17 <jgriffith> mriedem: posted https://review.openstack.org/#/c/327409/
17:44:46 <ildikov> scottda: :)
17:45:09 <jgriffith> mriedem: FWIW those two reviews and the gist applied to Nova give a working attach change
17:45:28 <jgriffith> mriedem: it doesn't include error-recovery, but I've got that and it will be in the Nova WIP when I get it posted
17:45:37 <jgriffith> and of course no microvresioning etc
17:45:51 <jgriffith> geeshh... *micro versioning*
17:48:24 <mriedem> i'm trying to think through the nova-api changes when we get a multiattach volume
17:48:51 <mriedem> because nova-api is still calling reserve, and the patch in mitaka needed to check if it's a multiattach volume
17:49:00 <ildikov> I would assume check_attach is out of the game as well
17:49:17 <ildikov> or at least it should be
17:49:32 <ildikov> and that checked mutliattach
17:49:45 <mriedem> b/c hemna is removing the calls to os-reserve?
17:49:46 <ildikov> other than that there is the code for the BFV case
17:50:28 <ildikov> mriedem: hemna has a patch up for removing check_attach as those checks are in os-reserve as well
17:50:59 <mriedem> yeah https://review.openstack.org/#/c/315789/
17:51:02 <hemna> I've been absent from this meeting...sorry guys
17:51:10 <ildikov> mriedem: so both Nova and Cinder performs the same checks
17:51:12 <hemna> grenade issues.
17:51:56 <ildikov> hemna: no worries, if you can take a look at jgriffith's patches that would be good, we talked about those mostly
17:52:13 <hemna> ok I'll re-read the meeting after I'm done
17:52:32 <ildikov> mriedem: so if jgriffith did not remove reserve for now it still does its job for checking the volume status
17:53:12 <ildikov> I mean volume state, like 'attached', etc. and it's prepared for multiattach as well
17:53:18 <ildikov> hemna: tnx
17:54:23 <mriedem> ok 5 minutes left
17:55:58 <ildikov> right, so as a summary we need reviews for jgriffith's patches, and also comment on the microversioning to have the simpler API calls for attach and then detach, we can move on to the more difficult cases after figuring out the basics
17:56:30 <ildikov> we also need an eye on hemna's remove check_attach patch as that would be another great cleanup item
17:56:44 <mriedem> there are some comments in that one already
17:56:55 <ildikov> we also talked about the Cinder migrate tests scottda is working on
17:57:28 <ildikov> mriedem: yeap I saw, I'll check on the details later
17:57:51 <ildikov> is there anything for the today's meeting we would need to discuss?
17:59:08 <mriedem> end it
17:59:17 <ildikov> ok, I'll take it as a no :)
17:59:24 <scottda> bye
17:59:25 <ildikov> thanks everyone!!!
17:59:45 <ildikov> #endmeeting