16:00:02 <thingee> #startmeeting cinder
16:00:03 <openstack> Meeting started Wed Aug 12 16:00:02 2015 UTC and is due to finish in 60 minutes.  The chair is thingee. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:00:04 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:00:06 <openstack> The meeting name has been set to 'cinder'
16:00:08 <smcginnis> o/
16:00:10 <jungleboyj> fireball: has multiple personalities.
16:00:11 <ericksonsantos> o/
16:00:13 <jungleboyj> o/
16:00:13 <thangp> o/
16:00:14 <dulek> Hello!
16:00:14 <scottda> hi
16:00:17 <geguileo> Hi!
16:00:20 <eharney> hi
16:00:20 <jordanP> o/
16:00:21 <patrickeast> hi
16:00:21 <Guest72293> hello :)
16:00:21 <davechen> Hi,
16:00:24 <Swanson> hello
16:00:26 <tbarron> hi
16:00:31 <cebruns> Hi
16:00:42 <xyang0> hi
16:00:45 <jgregor> Hello
16:00:53 <thingee> #topic how translate specific exceptions
16:01:22 <kmartin> hello
16:01:35 <thingee> uhhh mriedem?
16:02:22 <thingee> ok move on
16:02:24 <xyang0> mriedem: not here?
16:02:30 <xyang0> https://review.openstack.org/#/c/209150/
16:02:34 <jungleboyj> thingee: I told mriedem the time.  I will ping him.
16:02:42 <xyang0> thingee: that is the patch he proposed
16:02:45 <thingee> #topic modify volume image metadata
16:02:48 <hemna_> morning
16:02:54 <thingee> #link https://review.openstack.org/#/c/211201/3
16:02:59 <davechen> Hi,
16:03:16 <mriedem1> error handling in the client
16:03:20 <thingee> So I started look at some of the work davechen is doing here and it's quite a bit of complexity for us to have role protected metadata
16:03:32 <thingee> mriedem1: we'll circle back
16:03:40 <mriedem1> ok
16:03:49 <davechen> thingee: as far as I  know, only glance has this feature.
16:04:05 <davechen> thingee: and we need this based on two reason.
16:04:18 <hemna_> I'm on a call for work, so I'll lurk here as best I can.
16:04:25 <erlon> hi
16:04:33 <cpallares> o/
16:04:40 <davechen> the first reason is, Glance Image Properties that are copied are a combination of mutable and immutable values, But that distinction is lost when they are copied into Cinder.
16:05:13 <davechen> the second reason is, the Glance has RBAC to properties, tt could be the case that we want to update a property in Cinder that is protected in Glance.
16:05:33 <davechen> have a glimpse of this webpage, http://docs.openstack.org/developer/glance/property-protections.html
16:05:57 <davechen> there are some desc about the props pretection in glance.
16:06:27 <thingee> so to put in small amount of words, this is too much complexity for more than I care to add into cinder
16:07:05 <thingee> I don't think cinder should even care how this works
16:07:09 <davechen> thingee: so, this is not needed per your perspective?
16:07:40 <thingee> based on the reasons you gave in the review, I'm trying to think of a time someone was trying to solve this problem
16:07:52 <thingee> license key
16:07:59 <thingee> kernel id
16:08:03 <thingee> ramdisk id
16:08:03 <davechen> I think there have been some discussion about this.
16:08:07 <thingee> where?
16:08:08 <davechen> yeah...
16:08:21 <davechen> https://openstack.nimeyo.com/16488/openstack-cinder-glance-update-volume-image-metadata-proposal
16:08:35 <davechen> mailing list, but i think it's a long discussion.
16:08:42 <thingee> I open this to meeting because I want to know from folks what they think too
16:08:56 <thingee> And really I think this is overthought for just modifying metadata.
16:09:30 <davechen> and I think Duncan is in the glance's meeting to talk with glance team about this.
16:10:03 <davechen> http://eavesdrop.openstack.org/meetings/glance/2014/glance.2014-06-26-20.03.log.html
16:10:28 <davechen> so, anyone has a interesting could read the that.
16:11:00 <deepakcs> o/
16:11:07 <davechen> thingee: I am okay with it, if this is too heavy, we can just drop it.
16:11:42 <davechen> but not sure what other's thoughts?
16:11:59 <davechen> winston-d^^
16:12:07 <davechen> DuncanT ^^
16:12:08 <winston-d> davechen: sorry, i was joining late, dropping what?
16:12:23 <davechen> winston-d: props pretection.
16:12:26 <winston-d> role/policy base metadata update?
16:12:54 <davechen> yup, this maybe too complexity per Mike's perspetive.
16:13:29 <guitarzan> davechen: this is as opposed to always requiring admin to update image meta?
16:13:30 <davechen> there once a disccusion around this, and thought this was needed to pretect some props as what glance did.
16:13:52 <jgriffith> Personally I think we might need to do some metadata cleanup/fixing before we tackle this.. but perhaps in M
16:13:54 <davechen> guitarzan: no, it's based on user's role.
16:14:09 <guitarzan> davechen: so a normal user can update some image meta in glance?
16:14:11 <davechen> guitarzan: not always just admin can do this stuff.
16:14:14 <winston-d> DuncanT wanted this feature
16:14:38 <davechen> sure, if the props is not the confidential props, why we cannot read/update it.
16:14:40 <guitarzan> davechen: are kernel and ramdisk the only editable ones?
16:14:41 <winston-d> guitarzan: i guess so, if it's a instance snapshot or private image
16:14:54 <jgriffith> FWIW if the numbers in this analysis are accurate: https://bugs.launchpad.net/cinder/+bug/1483165  I'm a bit concerned
16:14:54 <openstack> Launchpad bug 1483165 in Cinder "The performance of volume index is low" [Undecided,In progress] - Assigned to wangxiyuan (wangxiyuan)
16:15:02 <uvirtbot> Launchpad bug 1483165 in cinder "The performance of volume index is low" [Undecided,In progress] https://launchpad.net/bugs/1483165
16:15:03 <uvirtbot> Launchpad bug 1483165 in cinder "The performance of volume index is low" [Undecided,In progress]
16:15:19 <smcginnis> New bot?
16:15:34 <davechen> winston-d: not just DuncanT but glance's team as well.
16:15:41 <jgriffith> smcginnis: it's like a begin/end code statement :)
16:16:15 <smcginnis> ;)
16:16:56 <davechen> thingee: what do you think? Mike
16:17:23 <davechen> thingee: drop that patch?
16:17:35 <thingee> I'm not seeing a huge reason to bring it in honestly. I think it's crazy all these different things in openstack doing role based policies though.
16:17:42 <thingee> I can't even keep track
16:17:50 <thingee> I'd hate to be a deployer.
16:17:58 <davechen> thingee: :)
16:18:35 <davechen> thingee: okay, if we need this, we can do it.
16:19:21 <davechen> thingee: If these stuff is not such important, we can just let it be.
16:19:25 <winston-d> should 'admin_or_owner" coever 99% use cases?
16:19:26 <jgriffith> thingee: +1
16:19:43 <thingee> winston-d: that's where I'm getting it at.
16:19:46 <thingee> what's wrong with what we have now
16:19:50 <guitarzan> winston-d: I doubt it, because most of them the owner can't update
16:20:09 <davechen> winston-d: this policy is too simply I think.
16:20:25 <winston-d> guitarzan: then change policy i guess?
16:20:42 <davechen> winston-d: If admin_or_owner cover 99%, there is no need to be oslo policy to be existing.
16:21:09 <guitarzan> winston-d: I'm not sure what to do about the 4 keys that they can change
16:21:13 <guitarzan> or whatever that number is
16:21:14 <winston-d> davechen: we are talking about cinder context
16:21:25 <winston-d> davechen: not overall openstack use cases
16:21:39 <davechen> winston-d: okay.
16:21:56 * thingee is confused
16:22:17 <winston-d> is this a blocking issue for certain use cases?
16:22:43 <thingee> no
16:23:14 <thingee> alright lets move on
16:23:24 <davechen> okay, thanks.
16:23:28 <thingee> #topic how to translate specific exceptions
16:23:30 <thingee> mriedem1: hi
16:24:12 <mriedem1> https://review.openstack.org/#/c/209150/
16:24:21 <mriedem1> ust wondering what to do there
16:26:57 <guitarzan> mriedem1: this might be a silly question, but why do we fail a detach when unattached anyway?
16:27:42 <mriedem1> guitarzan: because the attach failed on boot
16:27:49 * jungleboyj can hear everyone's brains processing.
16:28:11 <guitarzan> mriedem1: I'm not sure that answers my question
16:28:16 <guitarzan> but I might just not understand
16:28:34 <guitarzan> detaching after an attach failed is a cleanup operation, no?
16:28:36 <winston-d> mriedem1: and nova doesn't know about the attaching failure so it thought novulme is attached?
16:28:53 <guitarzan> how does nova not know about an attach failure?
16:30:40 <guitarzan> I don't want to get off track, but I don't understand why we couldn't just say "ok, you detached"
16:30:54 <guitarzan> anyway, just my 2 cents if we want to continue on the conversation
16:31:03 <smcginnis> guitarzan: That makes sense to me.
16:31:05 <mriedem1> sorry, multitasking,
16:31:08 <mriedem1> so there is the bug report
16:31:17 <mriedem1> i'd suggest looking at that for the root issue
16:31:36 <mriedem1> my question is how we should handle translating errors from cinder-api to specific exceptions i nthe client so nova can key off those
16:31:47 <mriedem1> rather than nova having to parse the error message from cinder, which could change
16:32:00 <guitarzan> sure, I'm just saying that cinder could just not fail the detach...
16:32:00 <mriedem1> we have a similar model with neutronclient
16:32:09 <guitarzan> and nova would continue happily on with its cleanup
16:32:13 <mriedem1> guitarzan: cinder fails the detach b/c the attach didn't happen
16:32:19 <scottda> https://bugs.launchpad.net/nova/+bug/1476806
16:32:19 <openstack> Launchpad bug 1476806 in OpenStack Compute (nova) "Unable to delete instance with attached volumes which failed to boot" [High,In progress] - Assigned to Maxim Nestratov (mnestratov)
16:32:21 <uvirtbot> Launchpad bug 1476806 in nova "Unable to delete instance with attached volumes which failed to boot" [High,In progress]
16:32:22 <uvirtbot> Launchpad bug 1476806 in nova "Unable to delete instance with attached volumes which failed to boot" [High,In progress] https://launchpad.net/bugs/1476806
16:32:25 <mriedem1> there are no attachments so the detach returns a 400
16:32:32 <mriedem1> nova wants to handle that and ignore
16:32:32 <guitarzan> mriedem1: that's what I'm saying we could fix
16:32:38 <guitarzan> *we* could ignore it
16:32:45 <smcginnis> Sub-codes again?
16:32:59 <mriedem1> there is an API working group patch up about specific codes for error responses
16:33:04 <smcginnis> We talked about this at the kilo midcycle.
16:33:06 <mriedem1> i assume that's a ways out from landing
16:33:37 <jgriffith> mriedem1: if it gets a detach and the volume is not attached it should just say "ok"
16:33:47 <jgriffith> mriedem1: it shouldn't be a failure to begin with in that case
16:34:09 <mriedem1> yeah, idk, i heard things about races yesterday where the db and actual block storage backend aren't in synhc
16:34:10 <mriedem1> *sync
16:34:14 <jgriffith> mriedem1: but FWIW I do like the idea of some standardized responses to nova so it's not mess parsing
16:34:28 <jungleboyj> jgriffith: ++
16:34:34 <jgriffith> mriedem1: meh... I don't know how real that is, but regardless
16:34:39 <mriedem1> sure
16:34:47 <jungleboyj> mriedem1: So, this is moving the message parsing into cinderclient instead.
16:34:51 <guitarzan> mriedem1: well, bdms being out of sync does happen, but this just doesn't seem like a failure case
16:34:52 <mriedem1> yes
16:34:54 <mriedem1> jungleboyj: yes
16:34:58 <jgriffith> mriedem1: there's lots of fud around here about races and consistency that isn't as prevalent as one might think
16:35:00 <jungleboyj> mriedem1: What improvement are we going to get from that?
16:35:08 <mriedem1> jungleboyj: that it lives closer to cinder
16:35:08 <xyang0> https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L865
16:35:18 <xyang0> we threw exception here in the manager
16:35:33 <guitarzan> xyang0: that should just return instead of raise :)
16:35:35 <mriedem1> jungleboyj: and it's the same model we (nova) has with neutronclient errors that are translated from neutron api in the client
16:35:36 <jungleboyj> mriedem1: ++  I thought that was the answer but wanted to make sure.
16:35:37 <mriedem1> that nova handles
16:35:46 <guitarzan> xyang0: and it should just be a  warn
16:35:47 <jgriffith> jungleboyj: it's way better than trying to guess/parse strings in exception messages
16:36:09 <mriedem1> granted, this is a thing nova will want to backport to kilo,
16:36:18 <mriedem1> because with this bug you can't delete an instance w/o removing the bdms in the db
16:36:20 <mriedem1> which blows
16:36:31 <jungleboyj> jgriffith: ++  Agreed.
16:36:38 <mriedem1> i don't know how keen cinder people would be on backporting that change from 400 to 200
16:36:39 <winston-d> mriedem1: yeah, hated it
16:37:04 <winston-d> mriedem1: i mean have to manually clean up bdm
16:37:13 <hemna_> ok back
16:37:26 <mriedem1> winston-d: yeah, any surgery directly in the nova db isn't fun
16:37:27 <hemna_> so I agree with jgriffith if a detach call comes in and the volume is already detached, just say ok.
16:38:10 <xyang0> can anyone look at this: https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L865
16:38:17 <xyang0> okay if we just return success?
16:38:35 <mriedem1> xyang0: this is actually what nova is hitting in that bug https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L885
16:38:54 <xyang0> mriedem1: right.  so we can fix it in cinder
16:39:01 <hemna_> xyang0, yah
16:39:09 <xyang0> mriedem1: return success instead of raise
16:39:10 <hemna_> xyang0, want me to put up a patch to change that?
16:39:13 <mriedem1> this is an API change, but from error to success which might be ok
16:39:13 <scottda> +1 to fix in the manager
16:39:28 <xyang0> hemna_: sure
16:39:33 <hemna_> xyang0, ok I'll do it.
16:39:37 <smcginnis> +1
16:39:54 <geguileo> I don't like changing it to success
16:40:01 <jungleboyj> Yeah, we shouldn't raise.  There was nothing to detach.
16:40:15 <geguileo> There was nothing and you asked me to detach, so error
16:40:19 <winston-d> geguileo: 400 to 200, badrequest to accepted
16:40:31 <xyang0> geguileo: we also do that in drivers.  if you try to detach a volume that is already detached, log a msg and move on
16:40:31 <geguileo> It's like trying to delete a file in your filesystem that doesn't exist
16:40:35 <geguileo> Don't you get an error?
16:40:37 <winston-d> not necessarily sucess
16:40:43 <hemna_> it's not much different from what we do with delete volume that doesn't exist.
16:40:47 <hemna_> drivers just say...ok
16:40:51 <xyang0> hemna_: +1
16:40:54 <thingee> geguileo: this is no different from other cinder things "I was not able to delete this volume because it didn't exist, so success"
16:41:01 <thingee> I'm fine with this approach
16:41:07 <scottda> Truth is, this is an API change and so it might break people's scripts
16:41:07 <geguileo> thingee: I understand it
16:41:15 <jungleboyj> hemna_: That makes sense.
16:41:17 <geguileo> thingee: But I still believe we should report an error
16:41:22 <xyang0> you can log a warning
16:41:28 <geguileo> thingee: And Nova should look at the type of error
16:41:30 <thingee> #topic update on volume migration
16:41:34 <jungleboyj> geguileo: ++  But not raise the exception.
16:41:45 <thingee> jungleboyj: ^
16:41:50 <thingee> vincent isn't around
16:41:53 <jungleboyj> Argh!
16:42:14 <thingee> do you have an update?
16:42:21 <thingee> for vincent or should I move on?
16:42:36 <winston-d> jungleboyj: I have one comment about https://review.openstack.org/#/c/186312/
16:42:50 <jungleboyj> thingee: I don't have anything specific.  Sorry, missed that he had put that out there.
16:43:04 <jungleboyj> If we can get eyes on the patches and work on getting things merged so we get more run time that would be great.
16:43:12 * jungleboyj needs to go review the patches as well.
16:43:14 <deepakcs> geguileo: ++
16:43:15 <jgriffith> geguileo: why should we put the volume in error state in this case?
16:43:34 <jgriffith> geguileo: it doesn't work that way... "nova look at the type of error"
16:43:36 <jungleboyj> winston-d: Did you put it in the review?
16:43:43 <winston-d> which is within this change, attach/detach or whatever actions that are not allowed when volume is marked as 'maintaince',
16:43:44 <jgriffith> geguileo: the volume gets put in error-detaching and your'e "stuck"
16:43:54 <thingee> #topic call for reviews on object patches
16:44:02 <dulek> hi!
16:44:04 <winston-d> this change  should stop these operations.
16:44:10 <geguileo> jgriffith: But we can return an error and not set it to error detaching
16:44:25 <thingee> dulek: thangp hi
16:44:25 <winston-d> jungleboyj: I mentioned this a few times in previous reviews.
16:44:28 <thangp> hi!
16:44:34 <dulek> We just want to report that work on RPC compatibility is blocked by object patches.
16:44:35 <winston-d> but it seems vincent hasn't addressed it.
16:44:44 <jungleboyj> winston-d: Ok, I will look through that and highlight it to Vincent.
16:44:49 <jungleboyj> winston-d: Thanks.
16:44:49 <dulek> We recently merged ConsistencyGroup object.
16:44:51 <winston-d> jungleboyj: thx
16:44:57 <jgriffith> geguileo: I suppose I don't care... just don't see any real value in it, because what's Nova going to do?  What are your expectations here?
16:45:02 <thingee> #link https://review.openstack.org/#/c/177054/
16:45:05 <jgriffith> It's a warning IMHO
16:45:07 <dulek> There are only 3 to go: Volume, Service and CGSnapshot
16:45:11 <thingee> #link https://review.openstack.org/#/c/201404/
16:45:12 <jungleboyj> jgriffith: ++
16:45:18 <thingee> #link https://review.openstack.org/#/c/160417/
16:45:18 <guitarzan> geguileo: it would just add more code to nova to do the same thing it's already going to do
16:45:23 <thingee> jgriffith: +1
16:45:30 <geguileo> jgriffith: My expectations are giving the right response
16:45:33 <dulek> So we want to call for reviews of them.
16:45:37 <thingee> #link https://review.openstack.org/#/c/195027/
16:45:46 <dulek> Without them thangp's work on making rolling upgrades *actually* work is blocked.
16:45:47 <thingee> geguileo: I'm with jgriffith here, it's a warning.
16:45:48 <geguileo> guitarzan: Sure, but it would be the right response
16:45:59 <thingee> #link https://review.openstack.org/#/c/205622/
16:46:00 <jgriffith> geguileo: LOL.. and I'm saying if you asked to detach a volume and it's detached that is the correct resposne :)
16:46:02 <guitarzan> geguileo: nah, the right thing to do is just smile and nod when nova tries to detach something already detached
16:46:03 <jgriffith> but anyway
16:46:04 <geguileo> You guys agree, so I'm cool with it  :-)
16:46:04 <jgriffith> moving on
16:46:09 <thingee> #link https://review.openstack.org/#/c/209701/
16:46:19 <thingee> jgriffith: we already moved on :)
16:46:20 <dulek> (thingee's posting links to reviews)
16:46:24 <dulek> (and my messages are sinking in the background noise I think)
16:46:32 <vilobhmm> thingee, jgriffith : https://review.openstack.org/#/c/205369/ - nested quota driver needs review
16:46:52 <geguileo> dulek: Yep, sorry for making that much noise
16:47:06 <xyang0> dulek: we need updated versionedobjects for cgsnapshot object to work.  that is not release yet, right?
16:47:08 <vilobhmm> its all +1 since a week or so …if someone from core team can have a look it would be helpful
16:47:15 <vilobhmm> thingee , jgrifith : ^^
16:47:15 <thingee> #info people should review finishing the object patches and rpc versioning patches
16:47:27 <dulek> xyang0: Correct, but we can still work on other patches.
16:47:31 <winston-d> dulek: no, they are still on the screen
16:47:35 <thangp> xyang0: yes, but we could put a workaround in place until then?
16:47:53 <dulek> So again - work on upgrades is going forward, but patches are very big and hard to review.
16:48:10 <xyang0> thangp: that will break all the drivers if that is not merged.  when do you expect versionedobjects will be updated?
16:48:20 <dulek> We need them objects merged to progress.
16:48:28 <winston-d> dulek: can't be broken down to smaller ones?
16:48:34 <thangp> xyang0: not sure
16:48:44 <thangp> xyang0: i can ask doug
16:48:44 <xyang0> thangp: what is the workaround?  install versionedobjects manually from git?
16:48:50 <xyang0> thangp: ok
16:49:00 <dulek> winston-d: It's quite hard to use objects in one place and not in the others.
16:49:01 <thingee> thanks dulek and thangp
16:49:04 <thangp> xyang0: no, just unset the consistencygroup
16:49:22 <thangp> so cgsnapshot.consistencygroup is not populated
16:49:29 <thangp> so it could be sent over rpc
16:49:41 <xyang0> thangp: can you add the workaround in the review?  I couldn't run that patch with this problem
16:49:48 <thangp> xyang0: sure
16:49:58 <dulek> xyang0: I'll make sure this is done in CGSnapshot patch.
16:50:08 <xyang0> dulek: ok, thanks
16:50:23 <winston-d> dulek: that i agree
16:50:37 <dulek> That's all from our side I think
16:50:49 <thingee> #topic open discussion
16:51:23 <ericksonsantos> it would be nice if we have reviews in nested quota patches as well
16:51:36 <vilobhmm> thingee : looks like the ping got lost in the objects discussion before but  https://review.openstack.org/#/c/205369/ - nested quota driver needs review
16:51:36 <ericksonsantos> https://review.openstack.org/#/c/206171
16:51:39 <ericksonsantos> and https://review.openstack.org/#/c/205369/
16:51:51 <vilobhmm> thingee, jgriffith : ^^
16:52:30 <thingee> #endmeeting