17:00:16 <ildikov> #startmeeting cinder-nova-api-changes
17:00:16 <openstack> Meeting started Mon Dec  5 17:00:16 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:17 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
17:00:19 <openstack> The meeting name has been set to 'cinder_nova_api_changes'
17:00:26 <mriedem> o/
17:00:27 <ildikov> scottda ildikov 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:38 <scottda> hi
17:01:13 <ildikov> mriedem: scottda: hi :)
17:01:37 <ildikov> let's wait a bit longer to see who's around for today's meeting
17:02:34 <DuncanT> Hi
17:02:54 <DuncanT> I'm here, though I've had no real input on this for ages
17:03:12 <ildikov> DuncanT: no worries, let me give a very short summary
17:04:01 <ildikov> last week we decided to go with CRUD operations for the new Cinder API in the sense of having attachment_create, attachment_update, attachment_get and attachment_remove
17:04:28 <ildikov> attachment_create reserves the volume and creates a volume attachment
17:04:39 <ildikov> if the connector is not specified in the call it stops there
17:04:52 <ildikov> otherwise finishes the attachment process
17:05:14 <DuncanT> So that does the state change (attaching or attached) too
17:05:15 <ildikov> Nova can call it from the API for instance and call attachment_update when has the connector
17:05:16 <DuncanT> ?
17:05:40 <ildikov> DuncanT: it should handle state changes
17:06:17 <mriedem> i've finally reviewed johnthetubaguy's spec again https://review.openstack.org/#/c/373203/
17:06:21 <mriedem> 47 comments :)
17:06:30 <mriedem> i have some concerns in there
17:06:30 <ildikov> DuncanT: we haven't agreed on the exact state machine, I hope to have that when jgriffith updates the code based on what we agreed at last week
17:06:36 <DuncanT> ildikov: So for the instance migration case, we create two attachments for the same volume?
17:06:43 <mriedem> DuncanT: yes
17:06:50 <ildikov> DuncanT: if you mean live migrate, then yes
17:06:55 <mriedem> same instance, same volume, different hosts per attachment
17:06:56 <DuncanT> That makes sense. Thanks.
17:07:28 <ildikov> mriedem: does any of your concerns affect the high levels we agreed last week?
17:07:39 <mriedem> oh that's right, johnthetubaguy is out this week, wedding + honeymoon
17:07:47 * smcginnis sneaks in
17:07:48 <mriedem> ildikov: honestly i didn't catch the meeting last week
17:08:11 <mriedem> ildikov: some of my main concerns in john's spec are (1) nova setting the vol attachment status to 'error'
17:08:15 <ildikov> mriedem: then with what I summarized at the beginning of this one?
17:08:28 <mriedem> CRUD ops in cinder api for attachments is fine
17:08:37 <ildikov> mriedem: ok, we haven't touched on those "details"
17:08:39 <mriedem> my concerns in the nova spec are really around interactions
17:08:59 <mriedem> and some smaller things like i don't think nova needs a config option to tell it to do the new flow
17:09:10 <mriedem> but my main issue is nova setting the vol attachment status to error
17:09:49 <ildikov> I think that comes from the direction of creating new attachment in almost ever use case Nova has
17:10:26 <ildikov> if we can update the attachment, then changing the state to error does not make much sense
17:10:36 <mriedem> well, the spec is also a bit contradictory in places about either deleting the attachment on failure, or setting it to error status
17:10:55 <smcginnis> mriedem: Yeah, I think on failure probably better to just delete and retry. Or not.
17:11:04 <mriedem> imo nova shouldn't touch the state on an external resource,
17:11:09 <mriedem> if nova wants to set the instance to error state, so be it
17:11:44 <ildikov> I don't think either that Nova should change the attachment sate to error
17:12:02 <mriedem> the shelve stuff in there isn't fully fleshed out, but it's late in the spec and i'm assuming john ran out of steam
17:12:18 <ildikov> sounds like too many things to clean up after a while, but it's unclear that when and by whom
17:12:19 <mriedem> i'm trying to walk a fine line before john (both johns) rage quitting
17:12:29 <mriedem> s/before/between/
17:12:48 <mriedem> anyway, john garbutt is out this week
17:12:51 <mriedem> my comments are in the spec
17:12:58 <mriedem> i don't really have anything else to share today
17:13:32 <smcginnis> So the nova side issues have an impact on the cinder side ones?
17:13:47 <ildikov> mriedem: we can move forward with the Cinder parts this week as if my understanding is correct your comments are more about how Nova is using the Cinder API
17:13:55 <ildikov> mriedem: is this assumption correct?
17:13:59 <smcginnis> I think we mostly have agreement at a high level. Just wondering if we can proceed on the cinder side and work out the nova bits as we go.
17:14:20 <smcginnis> ildikov: Thinking the same. ;)
17:14:30 <mriedem> i haven't looked at the cinder spec,
17:14:43 <mriedem> the nova spec calls out some assumptions about how cinder handles state transitions for the attachment
17:14:50 <mriedem> but we could change that over time with microversions if needed
17:14:54 <mriedem> as nova starts to use it
17:15:01 <ildikov> mriedem: it's short and high level mainly captures CRUD
17:15:33 <mriedem> there are some things in the nova spec about upgrading old attachments to the new state model...that might be worth looking into
17:15:46 <mriedem> like if cinder will have issues with nova creating a new attachment record for an already attached volume
17:15:55 <ildikov> mriedem: I think if we go on and implement that under a next API microversion in Cinder then it should be a pretty good first step, but I'll look into the state changes part
17:16:11 <ildikov> although I think it's mainly Cinder's business what the volume state is...
17:16:12 <mriedem> i.e. nova has the bdm.connection_info, i'm assuming nova would pass that in on attachment create/update, but what would cinder do with that?
17:16:25 <smcginnis> ildikov: +1
17:17:16 <ildikov> mriedem: you mean Nova gets back the connection_info fro Cinder and stores it, right?
17:17:22 <mriedem> no
17:17:31 <mriedem> i'm talking about migrating existing attachments to the new model
17:17:39 <mriedem> e.g. we have a server with a volume that was attached in kilo
17:17:45 <mriedem> it doesn't have any attachment record in cinder
17:17:53 <mriedem> nova has the bdm with the connection_info
17:18:12 <ildikov> ah, ok
17:18:13 <mriedem> garbutt's spec says that to upgrade nova will be creating new style attachments for existing bdms
17:18:37 <mriedem> so the volume is already attached, we just need to get it into the new state of things so that a later detach, using the new flow, will work
17:18:48 <ildikov> I think we were talking about an upgrade script kind of thing earlier
17:19:02 <mriedem> i *think* that just means nova creates the attachment record in cinder and passes the connection_info, connector, host and server id
17:19:33 <mriedem> but with the volume already being in-use, and probably multiattach=False, will cinder puke on that?
17:19:48 <ildikov> that's the same instance so it will not
17:19:56 <mriedem> in other words, cinder will need some kind of 'oh this is for an existing attachment thing, let's not change state and just store it for our records' kind of logic
17:20:08 <hemna> why would nova pass the cinder connection_info during detach?
17:20:14 <mriedem> hemna: this isn't detach
17:20:25 * hemna is confused
17:20:39 <mriedem> hemna: did you show up late?
17:20:47 <hemna> yah I did sorry
17:20:51 <ildikov> although it's also the same host, I'm not sure what we agreed on that, most probably nothing in details
17:20:53 <hemna> trying to read the scrollback
17:21:12 <mriedem> i don't think this is a major hurdle,
17:21:26 <hemna> oh migrating existing attachments
17:21:34 <mriedem> just saying, there are cases like this that are probably not fully thought through yet on the cinder side, which we might have to microversion as nova starts using the new apis
17:21:37 <mriedem> hemna: yes
17:21:44 <ildikov> hemna: we are talking about the situation, when we are upgrading the system so we have volume attachments that were created with the old Cinder, so they don't have all the data, etc.
17:21:58 <mriedem> yeah, nova has the data
17:21:58 <hemna> so is cinder supposed to save the connection_info in the attachments table for each attach?
17:22:02 <mriedem> and wants to give it to cinder
17:22:06 <mriedem> yes
17:22:15 <ildikov> hemna: also in Nova we need to add the attachment_id and additional stuff to the BDM that we don't store today to get to a consistent environment
17:22:29 <hemna> ah yah
17:22:41 <mriedem> the old connection_info won't have the 'shared_host_connection' flag either...
17:22:46 <hemna> do we have a way to get that attachment_id in the API during a migration?
17:23:02 <mriedem> so when we go to detach the old style connection we'd be lacking those details..unless we refresh that at some point
17:23:28 <hemna> the shared_host_connection comes from the cinder driver though
17:23:29 <mriedem> hemna: when upgrading, nova creates the attachment in cinder, gets the attachment id back and stores it in the nova bdm
17:23:34 <mriedem> hemna: i know,
17:23:39 <hemna> unless Cinder calls the driver to ask for it, we can't get it
17:23:42 <mriedem> but it's not in the kilo era attachment
17:23:58 <mriedem> that's why i'm thinking cinder might need to refresh it or something
17:24:51 <mriedem> i guess if os-initialize_connection is idempotent today, this should probably be ok
17:24:58 <hemna> so I presume that the nova migration process should trigger the fetching of that
17:25:14 <hemna> yup
17:25:28 <mriedem> so,
17:25:35 <hemna> the volume manager would have to call initialize_connection to ask for it
17:25:50 <hemna> and would also get the rest of the connection_info at that point too
17:25:57 <hemna> even though Nova has a copy of it in the bdm
17:26:03 <mriedem> john's spec says that when nova calls update_attachment, it passes the os-brick connector, and gets the connection_info back - i guess for our upgrade scenario, nova can just be creating the attachment and updating it with the connector, and then brick can refresh the connection at that point
17:26:23 <mriedem> then cinder doesn't know if nova is actually attaching a new volume or just record keeping an old legacy attachment
17:26:48 <hemna> that *should* be ok
17:27:02 <hemna> FC is easy
17:27:08 <hemna> iSCSI does issues some rescans, etc
17:27:20 <hemna> but tht should be ok too, for an existing attachment
17:28:15 <ildikov> with the new Cinder API it sounds like an attachment_create with specifying all the info to it and delete the old one if the new attachment is created successfully
17:28:39 <mriedem> delete what old one?
17:28:46 <mriedem> there is no old one if the volume attachment is from kilo
17:29:17 <ildikov> then what was the "creating" part of your description above?
17:30:06 <mriedem> not sure i follow
17:30:27 <mriedem> i'm talking about a nova periodic task or something that's migrating old vol attachment data to the new world
17:30:39 <mriedem> those old vol attachments won't have a literal 'attachment' record yet
17:30:47 <mriedem> b/c when those were created, the attachment API didn't exist
17:31:06 <hemna> there will be an existing attachment record in cinder already for those
17:31:07 <mriedem> so nova's periodic task or some migration script will need to create those attachments for existing volume bdms
17:31:09 <ildikov> but the attachments are saved in the DB today too
17:31:13 <ildikov> with an attachment_id
17:31:20 <mriedem> hmmm
17:31:25 <ildikov> I think it might even be a separate table, I would need to check that
17:31:33 <hemna> volume_attachment
17:31:36 <mriedem> it won't have the connector
17:31:38 <hemna> I created that
17:31:41 <ildikov> hemna: yeap, that's the one
17:31:43 <mriedem> or the host?
17:31:43 <hemna> correct
17:31:48 <hemna> it won't have the connector
17:31:51 <mriedem> i'm not sure what needs to be consolidated there
17:31:53 <hemna> or the host
17:31:59 <ildikov> mriedem: yeap, with less info as you say, but it exists
17:32:11 <mriedem> but if it already exists, then i guess nova just needs to get the attachments for a given volume/server/host and then update them with the connector
17:32:32 <mriedem> and then store that attachment id in the bdm
17:32:34 <hemna> yah
17:32:44 <hemna> the attachment will have the instance_uuid in there
17:34:08 <mriedem> hemna: will it?
17:34:16 <mriedem> i thought that was something new getting stored in like mitaka
17:34:16 <hemna> yup
17:34:23 <mriedem> even going back to juno attachments?
17:34:31 <mriedem> or essex
17:34:32 <mriedem> :)
17:34:38 <hemna> I think that was stored in the volume table
17:34:42 <mriedem> maybe cinder didn't exist until folsom :L)
17:34:48 <hemna> prior to my adding the volume_attachment table
17:34:50 <mriedem> yeah, so point being...
17:34:55 <mriedem> data migration ftw
17:34:58 <hemna> I think we still stored the instance_uuid
17:35:03 <mriedem> and people with pets they haven't touched in 4 years
17:35:53 <ildikov> what about creating a new attachment record and do cleanup for the old data?
17:36:38 <hemna> https://github.com/openstack/cinder/blob/grizzly-eol/cinder/db/sqlalchemy/migrate_repo/versions/001_cinder_init.py#L179
17:36:42 <mriedem> not sure
17:36:44 <hemna> :)
17:37:10 <mriedem> hemna: yeah, but was that copied into the volume_attachments table when it was added?
17:37:15 <mriedem> i.e. was that data migrated?
17:37:20 <hemna> yes it was migrated
17:37:23 <hemna> I wrote that migration
17:37:28 <mriedem> ok
17:37:43 <hemna> existing attachments got copied into the volume_attachment table with the instance_uuid
17:37:44 <mriedem> alright, so maybe we just have nova call update_attachment and pass in the connector,
17:37:45 <mriedem> and that's it
17:37:51 <mriedem> cinder returns the attachment_id and nova stores it in the bdm
17:37:55 <mriedem> and we consider that done
17:37:56 <hemna> that should work
17:38:12 <mriedem> i mean, nova's migration is really just, get me all BDMs for instances on this host with a volume_id set but no attachment_id value
17:38:21 <mriedem> once we update those with the attachment_id, we don't migrate them again
17:38:21 <hemna> yup
17:38:34 <hemna> that should be good
17:38:40 <mriedem> then we drop that after ocata-eol or whatever
17:38:49 <mriedem> pike-eol at this point probably..
17:38:50 <mriedem> but anyway
17:39:18 <hemna> +1
17:39:23 <ildikov> sounds good to me
17:39:52 <ildikov> I hope to get the Cinder API landed soon and have the code up for Nova by the end of Ocata the latest
17:40:49 <mriedem> i can go back and update garbutt's spec and my comments/questions about upgrades with this info
17:41:02 <mriedem> i.e. nova shouldn't need to create a new attachment
17:41:17 <mriedem> do we know that griffith plans on using the existing volume_attachments table?
17:41:33 <ildikov> mriedem: that would be great to have that updated
17:41:37 <hemna> so...
17:41:51 <hemna> for whatever reason he created some key/value pair table for attachments
17:41:57 <hemna> I never understood the need for it
17:42:04 <ildikov> I think he planned to use a new one, but then we touched on that briefly that we might have that as a next step rather than a starting point
17:42:25 <hemna> since the volume_attachment table exists and just needs the new columns to support the shared flag, connector and connection_info
17:42:45 <ildikov> I would guess part of the issue is the info we store is different per driver, etc.
17:42:46 <hemna> everything would be there in that normalized table
17:42:48 <hemna> but whatevs
17:42:50 <mriedem> ok, so you guys are going to have to sort that out
17:43:24 <ildikov> mriedem: on my TODO list, it's added as a TODO in the Cinder spec too to ensure we have an eye on that
17:43:25 <mriedem> "I would guess part of the issue is the info we store is different per driver, etc." so store an 'extras' column with a json blob of driver cruft
17:43:25 <hemna> it shouldn't affect the Nova <---> Cinder API interactions
17:43:39 <hemna> it just makes the implementation in Cinder more difficult for no reason.
17:43:41 <mriedem> hemna: right, as long as it doesn't leak out of the API i think we're fine
17:43:42 <ildikov> hemna: +1
17:44:05 <ildikov> mriedem: if it does then we did something wrong IMHO
17:44:10 <hemna> yah
17:45:34 <hemna> https://review.openstack.org/#/c/387712/6/cinder/db/sqlalchemy/migrate_repo/versions/087_add_attachment_specs.py
17:45:35 <hemna> fwiw
17:46:37 <smcginnis> ildikov: Is the Cinder spec up to date with the latest?
17:46:54 <ildikov> smcginnis: yes
17:47:02 <smcginnis> ildikov: Great, thanks for doing that.
17:47:41 <ildikov> smcginnis: I'm still waiting for some input from jgriffith, but the outcome of the last meeting is captured in the spec
17:48:22 <smcginnis> Sounds like we need some of his time. That would be good to have him check off on it and add any other details from in his head.
17:48:23 <ildikov> smcginnis: I added a ref to the Nova spec as it's detailed regarding the interaction, so it contains examples regarding usage
17:48:31 <smcginnis> ildikov: +1
17:48:56 <ildikov> smcginnis: he's traveling today AFAIK, but I'll ping him later this week
17:49:18 <smcginnis> ildikov: Yeah, I think he's on a plane right now or in route. Hopefully he has some time later in the week.
17:49:39 <ildikov> smcginnis: do you have a rough guess on until when we should have the code ready to have it merged in Ocata?
17:50:35 <smcginnis> ildikov: I'm fine up until O-3 as long as we have some confidence in the code. At least in that it won't break anything existing and we can start iterating with microversions if tweaks are needed once Nova starts trying to use it.
17:51:35 <hemna> are multinode tests working?  re: live migration
17:51:42 <ildikov> smcginnis: I believe it's simple enough to serve the purpose and as it's completely new we should have less issues with it
17:51:55 <smcginnis> ildikov: I'm hoping so.
17:52:01 <mriedem> hemna: which multinode tests? nova or cinder?
17:52:02 <smcginnis> hemna: I know we have multinode running.
17:52:03 <mriedem> or both
17:52:07 <smcginnis> hemna: For cinder.
17:52:14 <ildikov> scottda: are you available to help out with microversions?
17:52:15 <mriedem> we had to disable the volume-backed live migration tests for lvm in the gate
17:52:15 <hemna> ildikov, well the API is new and not used yet, but his patch does change the volume manager quite a bit
17:52:18 <smcginnis> hemna: But can't remember what exactly it's covering at this point.
17:52:18 <mriedem> b/c the failure rate was too high
17:52:27 <hemna> :(
17:52:32 <scottda> ildikov: Yes, always
17:52:54 <ildikov> scottda: coolio, tnx for confirming!
17:52:56 <mriedem> volume-backed ceph live migration might be more stable
17:53:01 <scottda> ildikov: I've already given jgriffith Some POC code to help him moving forward
17:53:40 <hemna> attaches will work or they won't
17:53:41 <ildikov> scottda: sounds great, tnx
17:53:50 <hemna> it's live migration that has always been a PITA IMHO
17:54:49 <hemna> anyway, I think we need to get his patch out of merge conflict
17:54:54 <hemna> and get to testin git
17:54:57 <hemna> testing it
17:56:02 <ildikov> hemna: +1
17:56:23 <smcginnis> 3 minutes. Anything else today?
17:56:45 <hemna> not I
17:56:46 <ildikov> mriedem: BTW, do you think you can check this one too: https://review.openstack.org/#/c/335358/ ? :)
17:56:49 <mriedem> this was the volume-backed live migration bug anyway https://bugs.launchpad.net/nova/+bug/1524898
17:56:49 <openstack> Launchpad bug 1524898 in OpenStack Compute (nova) "Volume based live migration aborted unexpectedly" [High,Confirmed]
17:57:17 <mriedem> ildikov: i don't know when
17:57:46 <ildikov> mriedem: I think it would pretty much make our lives easier with moving to the new Cinder API if we got this cleanup in
17:58:22 <ildikov> mriedem: if you have an opinion about removing the do_check_attach flag that could give me some work
17:58:35 <ildikov> mriedem: I mean what hemna asked on the last patch set
17:59:02 <mriedem> i have to swap all of that context back in, which is going to take time, and have lots of things going on right now
17:59:07 <mriedem> so i'll get to it when i can
17:59:38 <ildikov> mriedem: ok
17:59:52 <ildikov> mriedem: please try to do it before Christmas
18:00:08 * ildikov likes Christmas gifts :)
18:00:11 <smcginnis> mriedem: A christmas present for ildikov
18:00:22 <ildikov> smcginnis: +1 :)
18:00:27 <mriedem> humbug
18:00:33 <mriedem> i don't get any gifts
18:00:59 <ildikov> mriedem: I will stop annoying you with the reminders
18:01:15 <ildikov> mriedem: and will not remind you to anything else this year
18:01:36 <ildikov> mriedem: I think we can almost say that it would be a present from me to you ;)
18:02:15 <smcginnis> We're over time..
18:02:17 <mriedem> on that note, can we wrap up so i can get some lunch? :)
18:02:34 <ildikov> yeap, we can
18:02:40 <ildikov> tnx everyone :)
18:02:44 <smcginnis> Thanks
18:02:50 <ildikov> #endmeeting