16:00:15 <ildikov> #startmeeting cinder-nova-api-changes
16:00:16 <openstack> Meeting started Thu Nov 30 16: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.
16:00:17 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:00:20 <openstack> The meeting name has been set to 'cinder_nova_api_changes'
16:00:29 <ildikov> johnthetubaguy jaypipes e0ne jgriffith hemna mriedem patrickeast smcginnis diablo_rojo xyang1 raj_singh lyarwood jungleboyj stvnoyes
16:00:50 <jungleboyj> @!
16:00:50 <_pewp_> jungleboyj ( ^_^)／
16:01:07 * johnthetubaguy hides at back of the room
16:01:24 * ildikov sees johnthetubaguy :)
16:01:25 * jungleboyj pulls johnthetubaguy  to the front
16:01:33 <jungleboyj> Nice spot for you here buddy.
16:01:39 <mriedem> o/
16:01:51 * johnthetubaguy doh!
16:02:30 <ildikov> ok, let's start
16:02:42 * smcginnis hides behind johnthetubaguy
16:03:18 <ildikov> so we have the last bit of the new flow enablement in Nova: https://review.openstack.org/#/c/330285/
16:03:30 <ildikov> I was begging for review on the Nova meeting today
16:04:02 <mriedem> need to re-run my tempest patch against that to make sure we're failing if trying to attach the same volume to the same instance
16:04:03 <mriedem> twice
16:04:28 <mriedem> https://review.openstack.org/#/c/515426/
16:04:34 <ildikov> although I think I need to rely on mriedem and johnthetubaguy with that or maybe I can ask jaypipes too
16:05:07 <ildikov> mriedem: right, thanks for the reminder
16:05:46 <johnthetubaguy> ildikov: don't think there were many open questions now, trying to remember the hour or so I spent going through the patch
16:06:28 <ildikov> johnthetubaguy: we must be close now, I did some polishing on the functional tests too by now
16:07:11 <johnthetubaguy> I was worried about left over attachments in a few places, but they didn't look like new bugs (missing unreserve call too)
16:07:29 <johnthetubaguy> otherwise felt close I think
16:07:51 <ildikov> yep, I remember that one, we can fix it in a follow up patch
16:08:09 <ildikov> I would not want to increase the size of this one if we can avoid that
16:09:47 <ildikov> we can figure out the remaining nits on the review
16:10:36 <ildikov> I have the libvirt patch for multi-attach on top of this one, we can get that landed too if we agree on a few nits as it doesn't do any harm but will help enabling multi-attach
16:10:38 <smcginnis> ildikov: Looks like the patch at least needs a rebase. Just waiting to see if there are any other changes?
16:11:05 <smcginnis> The flow patch, not the libvirt one.
16:11:29 <ildikov> smcginnis: we were talking about doing one more version bump once the shared_targets changes land
16:11:34 <mriedem> ildikov: have you run my tempest test for multiattach on your nova multiattach patch? i'm assuming that tempest test is in need of cleaning up now though.
16:11:41 <ildikov> smcginnis: this one: https://review.openstack.org/#/c/520676/
16:12:01 <ildikov> and jgriffith promised a follow-up patch with the API changes and the microversion bump
16:12:05 <smcginnis> ildikov: Ah, OK. Maybe jungleboyj can take a look at that (hint, hint)
16:12:28 * jungleboyj will look.
16:12:44 <ildikov> mriedem: nope, the multi-attach changes need some update now to match the new flow
16:12:52 <smcginnis> Although a couple good questions jgriffith should probably respond to first.
16:13:01 <jgriffith> ildikov: if/when the shared_targets implementation lands yes
16:13:19 <ildikov> smcginnis: yeah, that kept me back from begging for review again :)
16:13:31 <ildikov> jgriffith: plz check the two minor comments on that one :)
16:13:44 <ildikov> jgriffith: so I can annoy people to land it then
16:13:58 <jungleboyj> Ah, the shared targets one.  Cool.
16:14:32 <ildikov> mriedem: the libvirt change only sets the 'shareable' flag, so it doesn't enable multi-attach right away
16:15:47 <ildikov> mriedem: johnthetubaguy: as for multi-attach, jgriffith has a spec on the Cinder side for that: https://review.openstack.org/#/c/523608/
16:16:09 <ildikov> mriedem: johnthetubaguy: plz give it a quick read so we don't miss anything critical
16:17:08 <mriedem> so simply creating a volume with multiattach=True isn't good enough?
16:17:10 <ildikov> jungleboyj: smcginnis: quick question, who could help us with the policy changes in Cinder?
16:17:26 <jungleboyj> The spec looked pretty good yesterday.  I will look through it again now that it is updated.
16:17:35 <jgriffith> mriedem: I'd prefer not to do that if we can avoid it
16:17:58 <smcginnis> ildikov: Policy as far as adding a policy check for this? That's just done in the implementation now. Is that what you're asking?
16:18:06 <jgriffith> mriedem: using the type and capabilities filter makes that a bit more solid
16:18:13 <jungleboyj> ildikov:  Good question.  Need a volunteer there.
16:18:16 <jgriffith> mriedem: and it allows you to update/modify a volume after the fact
16:18:19 * jungleboyj smiles at ildikov  and jgriffith
16:18:34 <jungleboyj> jgriffith:  ++
16:18:38 <ildikov> smcginnis: I think so, I'm one of the least knowledgable people around as far as policy goes :(
16:19:12 <jgriffith> mriedem: there's pros/cons to either; but using types fits a bit more with our existing workflows and requriements so I thought it was a good fit
16:19:18 <smcginnis> jgriffith: Helps with scheduler ability to place it too I think.
16:19:28 <jungleboyj> smcginnis:  Right
16:19:35 <mriedem> that's fine, it's basically a flavor extra spec
16:19:45 <jungleboyj> You could then have multi-attach capable and incapable backends.
16:19:50 <smcginnis> ildikov: Yeah, no worries. With the policy-in-code changes, we just need to add the policy checking in the code for those operations.
16:19:51 <jgriffith> smcginnis: yes for sure; you can make the flag set the capability but that's another story :)
16:19:53 <jungleboyj> The scheduler would get the right onw.
16:20:24 <ildikov> smcginnis: oh, ok, that does not sound too bloody, but we'll see I guess :)
16:21:02 <jgriffith> smcginnis: ildikov I had just assumed that's part of the code, not sure what's being asked WRT to help on that
16:21:21 <jgriffith> ^^ what I mean is part of the job for whomever implements the spec/changes
16:22:03 <jungleboyj> Right.
16:22:18 <ildikov> jgriffith: if that person is me, then I'll need at least someone to ask dumb questions and still get sane answers :)
16:23:13 <smcginnis> ildikov: Yeah, should be enough existing examples that it won't be too bad.
16:23:36 <ildikov> smcginnis: sounds good
16:23:55 <jungleboyj> ildikov:  Right.  There are lots of policy examples.
16:24:24 <johnthetubaguy> jgriffith: do you allow changing of the volume while it is attached?
16:25:24 <jgriffith> johnthetubaguy: so that's an interesting one..... retype does allow that sort of thing if it does not include a migration
16:25:38 <johnthetubaguy> so that would be a problem on the Nova side
16:25:46 <jgriffith> johnthetubaguy: that's probably worth detailing and clearing up in the spec
16:25:57 <jgriffith> johnthetubaguy: how so?
16:26:07 <jgriffith> johnthetubaguy: I mean being a problem on the Nova side?
16:26:20 <johnthetubaguy> we need to tell libvirt to stop its caching when its multi-attach
16:26:29 <johnthetubaguy> we can't do that if its already attached
16:26:42 <jgriffith> johnthetubaguy: ahh, right... on *all* of the attachments right
16:26:54 <ildikov> jgriffith: yes
16:27:27 <smcginnis> johnthetubaguy: No way to change that setting while attached?
16:27:28 <jgriffith> johnthetubaguy: I'll look into that; we may end up back at using a flag on create again then :(
16:27:52 <jgriffith> johnthetubaguy: where did things on the extend attached volume end up on the Nova side?
16:27:55 <johnthetubaguy> its fine if you can update the flag only when there are no attachment records I guess
16:28:00 <smcginnis> Just, from the Cinder side, it's very simple to just retype to enable/disable multiattach this way.
16:28:02 <jgriffith> johnthetubaguy: we could just use the same mechanism there maybe?
16:28:34 <mriedem> "where did things on the extend attached volume end up on the Nova side?" not sure what that means
16:28:39 <johnthetubaguy> so detach can fail, when the volume is in use, so its not easy on the Nova side
16:28:52 <johnthetubaguy> mriedem: I think the answer is we never merged that code?
16:28:58 <jgriffith> mriedem: there was a patch a while back to allow extending an attached volume
16:29:00 <mriedem> it was merged in pike
16:29:12 <jgriffith> mriedem: said operation required a disconnect/reconnect on the Nova side IIRC
16:29:24 <mriedem> https://review.openstack.org/#/c/454322/
16:29:29 <jgriffith> mriedem: thanks
16:30:09 <johnthetubaguy> ah...
16:31:55 <ildikov> hmm, so are we saying now that a volume retype in Cinder would trigger hacking the libvirt domain xml to set the 'shareable' flag while the volume is attached?
16:32:28 <johnthetubaguy> would that work?
16:33:33 <jgriffith> ildikov: it's an option... johnthetubaguy ildikov I don't think it's worth doing at the onset though
16:33:52 <jgriffith> another option is to put login in Cinder to check that condition and disallow it at the API layer
16:33:58 <ildikov> johnthetubaguy: I don't know, I never checked whether or not it is possible
16:33:58 <jgriffith> grrr
16:34:02 <jgriffith> s/login/logic/
16:34:14 <johnthetubaguy> seems a good starting point
16:34:26 <jgriffith> and make that a policy as well so if we change it some day we have that ability
16:34:32 <smcginnis> So we need to block retype if a) it's attached, and 2) the retype include a multiattach capability change?
16:34:40 <jgriffith> smcginnis: correct
16:34:50 <jgriffith> smcginnis: that's what I'm thinking for now at least
16:34:53 <ildikov> jgriffith: sounds like a good start to me
16:34:53 <smcginnis> Bleh. OK, that works.
16:34:58 <jgriffith> smcginnis: it's pretty easy to infer that on the Cinder side
16:35:11 <jgriffith> smcginnis: yeah... not great, but not the worst either
16:35:13 <smcginnis> Yeah, at least it won't involve a too much hackery.
16:35:32 <jungleboyj> It seems like a reasonable start.
16:35:53 <jungleboyj> Would rather limit the function than hack up Nova.
16:36:09 <jungleboyj> It still gives us better all-around function.
16:36:17 <ildikov> I wouldn't be comfortable with the hackery on the Nova side now, so if these restrictions would work in Cinder that made me happy for now
16:38:18 <jgriffith> we used to disallow retype of in-use volumes
16:38:34 <jgriffith> anyway... I'll look at it and see what we can/can't do
16:38:48 <jgriffith> if nothing else fall back to using an option on create
16:39:10 <ildikov> jgriffith: sounds good, thank you
16:40:01 <johnthetubaguy> option on create that can be updated when there are no attachments?
16:40:13 <jgriffith> johnthetubaguy: no
16:40:21 <jungleboyj> jgriffith:  Sounds good.
16:40:26 <jgriffith> johnthetubaguy: if it's an option on create it's static for the life of the volume
16:40:45 <jgriffith> johnthetubaguy: if you want to change then you have to do something like create-from-volume with that option
16:41:00 <johnthetubaguy> OK
16:41:14 <jgriffith> johnthetubaguy: I'm saying that if restricting the retype of an in-use volume looks hacky/ugly then revisit the use of a create flag
16:41:19 <ildikov> jgriffith: johnthetubaguy: we talked about changing the flag if the volume is not attached, I guess we can iterate on that later if we would fall back to the flag
16:41:41 <jgriffith> ?
16:41:48 <smcginnis> That's the nice thing with using volume type. It let's us switch that without forcing the user to do stupid things.
16:42:18 <jgriffith> ildikov: well if you're going to do the same monkey trick with a create flag then there's zero point in going that route as opposed to a type
16:43:04 <ildikov> jgriffith: the request to change the flag or type or whatever we choose came up earlier
16:43:16 <jgriffith> ildikov: what?
16:43:36 <jungleboyj> jgriffith:  Retyping versus having to clone a volume though, far less obvious to the end user as far as how to get the desired result.
16:44:03 <jgriffith> ildikov: if you're talking about using a create flag, and then saying implement something to allow that to be changed then I'm saying that's a horrible idea and I'm back to just doing type/retype
16:44:07 <ildikov> jgriffith: so I think it'll come up again once we have the feature enabled, so better to figure out now what we are willing or not willing to/can or can't do
16:44:42 <jgriffith> and won't consider the flag option, because you're proposing putting in the same logic checks that would need to go into retype which is an accepted/general solution to these sorts of things already
16:44:45 <ildikov> jgriffith: type sounds good if we can block that for attached volumes for now
16:44:57 <jgriffith> ildikov: yeah
16:45:27 <ildikov> jgriffith: then we're on the same page? :)
16:45:40 <jungleboyj> Sounds like it.
16:48:52 <ildikov> jgriffith: anyway, I don't want to argue, just remember having a proposal in Cinder to update the multiattach flag a while back, so if type/retype will be the solution for that with care, I'm all supportive; if we have other ideas that would be fine with me too
16:49:06 <ildikov> jgriffith: what fits into Cinder the best
16:49:43 <ildikov> so to sum it up
16:50:27 <ildikov> I will rebase the new attach patch in Nova and re-run the tests a couple of times to see whether the blocking to attach the same volume to the same instance works fine
16:50:42 <ildikov> then get that patch merged finally if everything works well
16:51:06 <ildikov> in Cinder let's have the shared_targets patch merged and have the microversion bump in place
16:51:35 <ildikov> update the Cinder spec with the type/retype restrictions we've talked about just now
16:51:52 <ildikov> and that will cover a weekly work I think easily
16:51:57 <ildikov> did I miss anything?
16:53:49 <jungleboyj> ildikov: That sounds good to me.
16:54:10 <ildikov> jungleboyj: cool :)
16:54:35 <ildikov> mriedem: johnthetubaguy: jgriffith: plz let me know if I missed any critical item from the list above
16:54:51 <ildikov> anything else to discuss today?
16:54:57 <jgriffith> ildikov: sounds like you got it
16:55:24 <ildikov> jgriffith: cool, thanks
16:57:04 <ildikov> ok, then thanks everyone for joining today!
16:57:18 <ildikov> let's get some progress by the meeting next week!! :)
16:57:34 <ildikov> have a good rest of the day everyone!
16:57:50 <ildikov> BTW, I know it's KubeCon next week
16:58:06 <ildikov> will we still have the majority of people available?
16:58:12 <jungleboyj> I will be around.
16:58:21 <jungleboyj> Wanted to go but couldn't work it out.
16:58:28 <ildikov> johnthetubaguy: mriedem: jgriffith: smcginnis: ^^?
16:58:39 <ildikov> jungleboyj: sadness, next time I guess
16:59:01 <jungleboyj> ildikov:  Indeed.  I am sure jgriffith  and smcginnis will be good reps.
16:59:04 <smcginnis> I'll be at Kubecon, so probably won't be able to attend.
16:59:40 <ildikov> ok, will chat with the Nova guys then I guess :)
16:59:53 <ildikov> thanks again
17:00:07 <ildikov> let's move the further chats to the channels
17:00:14 <jungleboyj> Sounds good.
17:00:14 <ildikov> #endmeeting