16:00:04 <ildikov> #startmeeting cinder-nova-api-changes
16:00:04 <openstack> Meeting started Thu Dec  7 16:00:04 2017 UTC and is due to finish in 60 minutes.  The chair is ildikov. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:00:05 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:00:08 <openstack> The meeting name has been set to 'cinder_nova_api_changes'
16:00:11 <ildikov> johnthetubaguy jaypipes e0ne jgriffith hemna mriedem patrickeast smcginnis diablo_rojo xyang1 raj_singh lyarwood jungleboyj stvnoyes
16:00:29 <jungleboyj> @!
16:00:29 <_pewp_> jungleboyj (。･∀･)ﾉ
16:00:48 <mriedem> o/
16:01:10 <ildikov> I don't think we can expect too many more for today
16:01:16 <ildikov> so let's do a quick recap
16:01:24 <ildikov> on this week's happenings
16:01:41 <ildikov> we have progress on the new attach patch in Nova, special and many thanks to mriedem
16:01:57 * jungleboyj claps.
16:01:58 <ildikov> this is how the field looks like right now: https://review.openstack.org/#/q/topic:bp/multi-attach-volume+(status:open+OR+status:merged)
16:02:13 <ildikov> don't get caught up on the topic
16:02:47 <ildikov> mriedem: so the question on my side now is that what's missing to be able to land the new attach patch and who's gonna do it?
16:03:18 <mriedem> i will approve it once tests all pass
16:03:23 <mriedem> however,
16:03:33 <mriedem> https://bugs.launchpad.net/cinder/+bug/1736773
16:03:34 <openstack> Launchpad bug 1736773 in Cinder "attachment-show is including `connection_info` for non-admin callers, it shouldn't" [High,Triaged] - Assigned to John Griffith (john-griffith)
16:03:35 <mriedem> concerns me
16:03:49 <ildikov> in the sense of how we use it in the Nova code?
16:03:54 <ildikov> or in general?
16:03:57 <mriedem> depending on if/how that gets fixed,
16:03:59 <mriedem> it could break nova
16:04:15 <mriedem> as far as i know, nova isn't passing an admin token to get the attachment details
16:04:17 <mriedem> it's using the user token
16:04:21 <mriedem> or the nova service user token
16:04:46 <mriedem> so if a policy rule goes into place to only return connection_info for an admin, that will break our existing code because we won't be able to get the connection_info
16:04:50 <mriedem> as i said in the bug,
16:05:02 <mriedem> i think this is super latent with the os-initialize_connection volume action api
16:05:26 <mriedem> i'm not sure if there are different policy controls in place for those APIs in cinder, i didn't get that far
16:05:34 <mriedem> or if there are any policy controls in place at all
16:05:37 <ildikov> so should we switch to admin for that call now?
16:05:49 <mriedem> well, it will depend
16:06:32 <mriedem> as noted in the bug,
16:06:45 <mriedem> the connection_info dict in the response contains things like details about the target and credentials
16:07:21 <jungleboyj> *Sigh*
16:07:42 <jungleboyj> Changing that is going to break stuff I am working on as well.
16:07:48 <ildikov> so Nova is not supposed to get it at all?
16:07:49 <mriedem> looks like this is a policy check on updating an attachment https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L2075
16:07:57 <mriedem> ildikov: nova must get it
16:08:06 <mriedem> we use it to connect the volume to the host using brick
16:08:35 <mriedem> comparing to os-initialize_connection, there is a policy rule on that too https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L782
16:08:49 <ildikov> ok, so what does it depend on whether or not to use admin context to get those details?
16:08:59 <mriedem> https://github.com/openstack/cinder/blob/master/cinder/policies/volume_actions.py#L164
16:09:27 <mriedem> same as https://github.com/openstack/cinder/blob/master/cinder/policies/attachments.py#L37
16:09:41 <mriedem> but there isn't a policy rule for listing/showing attachment details
16:09:49 <jungleboyj> Would be good to fix that with a policy setting so if there are people who want that exposed we can still get it.
16:10:10 <ildikov> ah, ok, yeah
16:10:16 <mriedem> ildikov: the issue is that i, as a non-admin user, can create a volume and attach it to an instance,
16:10:27 <mriedem> and then get the details, as a non-admin, about the storage connection
16:10:37 <mriedem> including target IP and credentials
16:10:59 <mriedem> as i said, this has always been a problem with os-initialize_connection as far as i can tell
16:11:25 <mriedem> the main difference is there was never a CLI for initializing a connection to get the connection_info back, but the REST API was always there for anyone that knows how to use curl
16:11:39 <mriedem> there is a CLI for listing and showing volume attachment details
16:12:20 <ildikov> ok, put it together now
16:12:54 <ildikov> however it's a problem in general not just from Nova's perspective
16:13:14 <ildikov> so we need to figure out how to handle this
16:13:32 <ildikov> but do we need to hold the new attach patch on this?
16:14:07 <mriedem> idk
16:14:20 <mriedem> the thing is,
16:14:32 <mriedem> let's say cinder adds a policy rule to not show connection_info unless you're an admin by default
16:14:48 <jungleboyj> mriedem:  ++
16:14:58 <mriedem> that would require nova to add config and code for making sure we get an admin token for anytime we need that information,
16:15:02 <mriedem> since the user token might not cut it
16:15:06 <mriedem> and if you backport that change,
16:15:12 <mriedem> it means breaking all existing deployments
16:15:33 <mriedem> ^ if you also apply that policy change to os-initialize_connection
16:15:41 <ildikov> do you mean that for initialize or the new attach api?
16:16:00 <mriedem> i think if you fix this for the new API you also have to fix it for os-initialize_connection
16:16:03 <mriedem> it's the same issue
16:16:11 <ildikov> ok, but for os-initialize-connection that has nothing to do with what we do with the new flow
16:16:22 <mriedem> i realize that
16:16:50 <mriedem> but it doesn't make sense to say, "you can only get connection_info if you're an admin with this one API but not this other API"
16:16:58 <mriedem> e.g.,
16:17:04 <mriedem> i could attach a volume with the new flow,
16:17:11 <mriedem> and then curl to os-initialize_connection to get those details
16:17:17 <ildikov> yeah, I got the point
16:17:43 <ildikov> so the question now whether or not to fix it?
16:17:56 <ildikov> or whether or not fix and backport it?
16:18:14 <mriedem> yeah
16:18:34 <mriedem> if nova.conf is always configured to talk to cinder with a user that has the admin role, then it might be ok
16:19:01 <mriedem> but i don't think we say anywhere that has to be the case
16:19:12 <ildikov> yeah, that would solve the issue
16:19:14 <jungleboyj> mriedem:  That seems like a reasonable limitation to set.
16:20:01 <mriedem> the only other service i know of that nova talks to with an admin token at times is neutron
16:20:28 <mriedem> and that's to do things with the port binding profile
16:20:32 <mriedem> which is like a connection_info for a volume
16:21:11 <ildikov> well, if the issue is that we basically don't want users to access the connection_info just haven't fixed that for whatever reason till now, then I guess that's ok if Cinder becomes another service like Neutron
16:21:25 <jungleboyj> ildikov: ++
16:21:45 <ildikov> it seems that it would be nice to get this fixed at a certain point
16:21:53 <jungleboyj> And if we can fix the connection_info with a policy then it gives users who don't want to use an admin user an option.
16:22:03 <mriedem> devstack sets up the neutron creds in nova.conf with an admin user https://github.com/openstack-dev/devstack/blob/master/lib/neutron-legacy#L372
16:22:11 <ildikov> and I feel the pain, but at least good things come out of implementing the new flow if we fix for instance this one
16:23:11 <mriedem> we don't set credentials in [cinder] in nova.conf in devstack at all
16:23:20 <mriedem> so everything we test against today in CI is using the user token
16:23:34 <ildikov> so it's Cinder code change, Nova code change plus devstack fix
16:24:42 <ildikov> also I need to run in 10 minutes :/
16:24:56 <jungleboyj> Bummer.  More moving pieces.
16:24:59 <ildikov> so questions
16:25:04 <mriedem> well, it requires some thought on how you roll something like this out
16:25:10 <mriedem> if you're going to backport it
16:25:22 <mriedem> i think if you add a policy control for both cinder APIs,
16:25:32 <mriedem> it has to default to admin_or_owner to not break existing code
16:25:54 <mriedem> with clear documentation that if you change that to admin-only, then the [cinder] creds in nova.conf must be an admin user
16:26:17 <jungleboyj> mriedem:  That sounds good.
16:27:07 <ildikov> agreed
16:27:30 <ildikov> so would this all happen before thinking about landing the new attach code?
16:28:09 <mriedem> if the fix in cinder is backward compatible, then no
16:28:40 <jungleboyj> mriedem:  Why is that?
16:28:56 <ildikov> so I guess we need to chat with jgriffith to ensure it is backward compatible
16:29:00 <mriedem> because i would prefer to not merge code that is immediately broken
16:29:30 <mriedem> the release note on the new flow attach code in nova says that this is all basically internal plumbing changes, no impacts to existing stuff,
16:29:38 <mriedem> and should work as long as computes are upgraded and cinder 3.44 is available
16:29:55 <mriedem> i don't want to add a "oh btw you need to run with an admin user now for cinder too btw for this new code to work"
16:30:01 <ildikov> which should still be true with admin_or_owner, right?
16:30:08 <mriedem> "even though this was always a problem"
16:30:19 <mriedem> ildikov: yes admin_or_owner is fine
16:30:20 <ildikov> well, we said we would fix both
16:30:28 <mriedem> the user token is the owner of the instance and volume
16:30:29 <jungleboyj> mriedem:  Fair enough.
16:30:30 <ildikov> so if we break one we break the other one too
16:30:33 <mriedem> otherwise you can't GET the volume
16:30:38 <ildikov> yeah, that's what I meant
16:30:58 <ildikov> just stopped to be sure at anything recently
16:32:14 <mriedem> so i'll update the bug with notes
16:32:15 <mriedem> and thoughts
16:32:23 <mriedem> john is at kubecon?
16:32:30 <ildikov> yes, he is
16:32:34 <mriedem> ok
16:32:46 <ildikov> he's occasionally available, he has a talk I don't know when
16:33:02 <ildikov> so let's hold the code until we had a chat with him about this
16:33:21 <ildikov> we might have a cinderclient soon with the shared_targets microversion
16:33:35 <jungleboyj> ildikov:  Yes, I would like to get that through today.
16:33:40 <ildikov> mriedem: should I bump the microversion in the new attach patch if that gets out in the meantime?
16:33:49 <jungleboyj> I guess I could ninja merge the 3.48 patch.
16:33:50 <ildikov> jungleboyj: nice, thanks!
16:34:11 <ildikov> jungleboyj: yeah, that's just a version bump, nothing else if everything else is in place
16:34:13 <jungleboyj> Would kind-of like jgriffith 's sign off though.
16:34:20 <mriedem> what is 3.48
16:34:20 <mriedem> ?
16:34:29 <mriedem> shared_targets?
16:34:31 <ildikov> yes
16:34:34 <jungleboyj> mriedem: Yep.
16:34:52 <mriedem> meh - at this point i think what we have is ok based on our discussion yesterday
16:35:01 <mriedem> the multiattach code in nova can always check that cinder has 3.48
16:35:10 <mriedem> before it allows attaching a multiattach volume to >1 instance
16:35:11 <ildikov> we just didn't have some previous microversion changes implemented in the client
16:35:21 <mriedem> i would say don't rush the cinder side
16:35:25 <mriedem> we're not blocked
16:35:30 <ildikov> well, it's almost done
16:35:43 <mriedem> except for all the client stuff it sounds like, plus a release, etc
16:35:46 <ildikov> and then we have one less thing to keep in mind, which might be nice every now and then
16:36:10 <ildikov> the stuff is mainly on the gate now
16:36:28 <ildikov> and the client side changes need to land anyway otherwise we mess up like the last time...
16:36:49 <ildikov> ok, let's talk to jgriffith and then we will see where things are and move forward
16:36:54 <jungleboyj> :-)  I will see if jgriffith pops on later.  If he doesn't I will just merge it and propose the release.
16:37:10 <mriedem> don't you guys have other cores?
16:37:16 <mriedem> i know sean is gone too
16:37:17 <mriedem> but
16:37:25 <jungleboyj> Yep ....
16:37:28 <jungleboyj> mriedem:  Don
16:37:34 <jungleboyj> 't get me started .
16:37:36 <mriedem> i'd just rather not rush things
16:37:44 <mriedem> you want to make me core?
16:38:20 <mriedem> joking
16:38:25 <ildikov> ok, we can skip the version bump and just get things tidy in the cinderclient for it's own sake
16:38:25 <mriedem> but i can hear your gears grinding from here
16:38:32 <jungleboyj> mriedem:  That could be arranged.
16:38:42 <ildikov> ok guys, I need to run
16:38:47 <jungleboyj> I wish I could remove some of the other cores to more realistically show the state of things.
16:38:52 <jungleboyj> ildikov:   Ok, thank you.
16:38:56 <ildikov> is there anything else we need to chat/could decide about here?
16:39:00 <mriedem> nope
16:39:08 <jungleboyj> I will continue to curate the client patches.
16:39:11 <ildikov> ok, cool, thank you both
16:39:17 <ildikov> let's keep in touch on the channels
16:39:23 <jungleboyj> ildikov:  Will do.
16:39:23 <ildikov> jungleboyj: thank you
16:39:37 <ildikov> jungleboyj: and you're the PTL, you can do whatever you want ;)
16:39:45 <ildikov> chat later guys!
16:39:53 <ildikov> #endmeeting