13:00:04 <alex_xu> #startmeeting nova api
13:00:06 <openstack> Meeting started Wed Nov  9 13:00:04 2016 UTC and is due to finish in 60 minutes.  The chair is alex_xu. Information about MeetBot at http://wiki.debian.org/MeetBot.
13:00:07 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
13:00:10 <openstack> The meeting name has been set to 'nova_api'
13:00:16 <alex_xu> who is here today?
13:00:18 <cdent> o/
13:00:19 <diana_clarke> o/
13:00:24 <johnthetubaguy> o/
13:00:31 <bauzas> \o
13:01:01 <alex_xu> ok, let's start the meeting
13:01:13 <alex_xu> #topic action from previous meeting
13:01:21 <jichen> o/
13:01:44 <alex_xu> there is action for Kevin to bring up a spec for fix the server list query parameters
13:01:56 <alex_xu> #link https://review.openstack.org/393205
13:02:08 <alex_xu> this is the spec. and Kevin can't join the meeting today
13:02:40 <alex_xu> I found actually if I input the joined table in the sort_key, I will get 400 returned
13:02:55 <johnthetubaguy> any bits worth discussing here?
13:02:55 <gmann_> o/
13:03:08 <alex_xu> This behaviour is changed after this patch https://review.openstack.org/#/c/143632/6
13:03:12 <johnthetubaguy> ooh, interesting...
13:03:48 <alex_xu> But there still have some strange thing, like I can input sort_key=__mapper__, then got 500
13:04:13 <alex_xu> also I can input the joined table in the filter, like GET /servers?info_cache=xyz, then got 500
13:04:25 <johnthetubaguy> yeah, so not quite as bad position as we thought we were, so thats nice
13:04:47 <gmann_> so we want 400 in all cases right
13:05:06 <johnthetubaguy> yup
13:05:15 <alex_xu> but I can get 500 when the nova-api startup, then I sort by info_cache
13:05:23 <johnthetubaguy> alex_xu: that list in the spec looks quite large still, I guess we don't have an index on all of those?
13:05:31 <alex_xu> and I only can get 500 for sort_key=info_cache, which I didn't the reason
13:05:53 <alex_xu> johnthetubaguy: yes
13:06:18 <alex_xu> johnthetubaguy: the spec is only talk about the sort key. I thought the spec should talk about the filter and sort key.
13:06:45 <gmann_> yea it looks lot of things, for example anyone do sort with display_description ?
13:06:59 <johnthetubaguy> yeah, need filter and sort key doing
13:07:01 <alex_xu> do we want to stop sort on the column without index in this spec, or just stop the joinedtable filter and sort?
13:07:34 <johnthetubaguy> I suspect we want to add indexes needed, but only allow indexed ones to be filtered/sorted
13:07:47 <johnthetubaguy> but I would want to know what jaypipes thinks
13:08:01 <johnthetubaguy> might be filtered on non-indexed is not as bad, I am unsure
13:08:25 <johnthetubaguy> FWIW, I am OK having a big list for the admin users
13:08:45 <alex_xu> yea, for admin user, that sounds ok
13:09:02 * gmann facing some network issue
13:09:33 <alex_xu> how about let me catch jay, or send email to ask about that
13:11:40 <alex_xu> johnthetubaguy: that sounds ok?
13:11:58 <johnthetubaguy> yeah, thats fine, I will try ping him if I see him
13:12:11 <alex_xu> johnthetubaguy: cool, thanks
13:12:33 <alex_xu> then I will talk to Kevin to update the spec
13:13:13 <alex_xu> #topic Priority tasks
13:13:19 <alex_xu> emm...I didn't catch other thing for the last week
13:14:03 <johnthetubaguy> try undo cmd, if you want to head back to that
13:14:04 <alex_xu> johnthetubaguy: anything more you think worth to update?
13:14:17 <johnthetubaguy> what was our other action from last week?
13:14:36 <alex_xu> johnthetubaguy: that is the only one action
13:15:27 <alex_xu> sorry, I jump to another topic suddenly
13:16:16 <johnthetubaguy> no worries
13:16:25 <johnthetubaguy> priority wise, we made a list last week
13:16:41 <johnthetubaguy> basically that spec we just discussed, and its dependencies, I think?
13:16:51 <gmann> we need to update the same in https://etherpad.openstack.org/p/ocata-nova-priorities-tracking
13:16:57 <alex_xu> yea
13:17:06 <alex_xu> gmann: thanks
13:17:19 <alex_xu> #link https://review.openstack.org/388518
13:17:43 <alex_xu> I updated the base one
13:17:50 <alex_xu> looking for more feedback
13:18:35 <gmann> alex_xu: nice, ll check tomorrow, actually not getting much time for review due to office movement
13:19:02 <alex_xu> gmann: thanks1
13:19:11 <alex_xu> s/1/!/
13:19:52 <johnthetubaguy> I totally need to follow up on that one
13:20:04 <alex_xu> johnthetubaguy: thanks
13:20:14 <johnthetubaguy> does adding key pair as an example make sense?
13:20:17 <alex_xu> next week is spec freeze, just left one week for us
13:20:30 <alex_xu> johnthetubaguy: yes, agree with you, I added it into the spec
13:21:06 <johnthetubaguy> so you add the new microversion for additional_properties=True
13:21:17 <johnthetubaguy> I wonder if we should wait and add that later?
13:21:43 <johnthetubaguy> oops = False I mean
13:22:00 <johnthetubaguy> I totally miss read the spec
13:22:08 <johnthetubaguy> I think thats what I expecting, doing something later
13:22:11 <gmann> ignoring those as of now and then error with version bump
13:22:22 <johnthetubaguy> do we want to strip out the invalid stuff from what we return to the code?
13:22:29 <johnthetubaguy> like we do for v2.0 API
13:23:00 <alex_xu> for keypair, I guess there isn't invalid stuff work on v2.0 API
13:23:11 <johnthetubaguy> ?foo=asdfas is invalid right?
13:23:29 <johnthetubaguy> I mean the code doesn't read foo today, but it feels good to strip that all out
13:23:38 <gmann> johnthetubaguy: but strip out may lead to confusion by returning other item too ?
13:23:46 <alex_xu> yes, both v2.0 and v2.1 ignore the foo
13:23:57 <johnthetubaguy> I am really thinking about the follow up spec
13:24:08 <johnthetubaguy> when we pass the list of items stright into the DB layer
13:24:11 <gmann> i am sure some tempest tests might be filtering based on some
13:24:15 <johnthetubaguy> if we strip out all the bad bits, it should help
13:24:21 <alex_xu> johnthetubaguy: +1 for follow up spec
13:24:46 <johnthetubaguy> so it feels like the framework should always do the striping of additional properties, so we don't get confused about it
13:25:01 <johnthetubaguy> gmann: sorry, don't yet your question about the confusion?
13:25:31 <johnthetubaguy> I am thinking, when you have the decorator, it ensures you only get expected params in the query string you get back
13:25:44 <gmann> johnthetubaguy: i mean striping out those will still create impression that requested filter is taken care ?
13:26:04 <johnthetubaguy> not sure what you mean
13:26:17 <johnthetubaguy> longer term, we would reject the request with 400 errors
13:26:27 <johnthetubaguy> but doing that breaks existing requests, so we can't really do that
13:26:45 <gmann> like if i filter with some black-list filter key but i still get whole list items
13:26:45 <johnthetubaguy> ... now, if all the "bad" params cause 500 or 400 already, we should probably keep them as 400
13:27:00 <gmann> yea 400 for long term looks nice
13:27:12 <johnthetubaguy> I think we all agree on the future
13:27:19 <johnthetubaguy> the problem is what do we do now
13:27:21 <gmann> yea, it break existing one
13:27:48 <johnthetubaguy> add sanity to existing keys, protect the system from bad inputs, but also don't break existing API requests
13:28:07 <johnthetubaguy> seems like the same thing we had with 2.0 vs 2.1 to me, where we did the silent strip out of the extra params
13:28:35 <gmann> yea at least it would not break those completely, agree
13:28:58 <alex_xu> emm... I see the point now
13:29:18 <gmann> is there any way we can also tell that queried param is not taken care?
13:29:25 <johnthetubaguy> now for keypairs, it makes no difference, but I think it would be good to add that already
13:29:36 <johnthetubaguy> gmann: not really, right now we just silently ignore stuff anyways
13:29:53 <alex_xu> another case, if we add new parameter in the future, but in the old version API, the additional properties is allowed, that may lead to new parameter leak in the old api.
13:30:14 <johnthetubaguy> yep, thats the same problem we have today, basically
13:30:17 <gmann> but for v2.0 only, v2.1 return error but this case v2.1 also ignore
13:30:35 <johnthetubaguy> no, v2.0 ignores params you send in that are bad
13:30:39 <alex_xu> except also check the api request version in the python code
13:31:13 <johnthetubaguy> microversion is still checked here
13:31:30 <gmann> yea v2.0 does, i mean request with v2.1 does not ignore those instead return 400
13:31:33 <johnthetubaguy> we have additionalproerties = true mean, strip them out, but don't error out on bad keys
13:31:48 <johnthetubaguy> so there are bad values, and unknown properties
13:32:00 <alex_xu> johnthetubaguy: that sounds we create non-standard jsons-schema again
13:32:10 <johnthetubaguy> same as v2.0, I am proposing we reject bad values still, but we just ignore unknown properties
13:32:24 <gmann> hummm
13:32:42 <johnthetubaguy> alex_xu: well ish, true = true, false equals strip
13:32:50 <johnthetubaguy> so the key bit is, requests that used to work, keep working
13:33:10 <alex_xu> false equal strip sounds good
13:33:11 <johnthetubaguy> bad requests may fail in a new way, but whatever
13:33:40 <johnthetubaguy> we still *allow* the properties, just to keep the code "clean" we don't allow them into the main api code
13:34:03 <johnthetubaguy> stops us messing up, basically
13:34:12 <alex_xu> yea
13:34:20 <gmann> that's true
13:34:52 <alex_xu> I will update the spec for that
13:35:06 <alex_xu> and the code
13:35:17 <gmann> and next spec to version bump return 400 for unknown case too
13:35:31 <gmann> may be in P ? or in Ocata only?
13:35:37 <alex_xu> gmann: that will be next release
13:35:42 <gmann> ok
13:35:42 <alex_xu> or the future
13:36:02 <johnthetubaguy> yeah, next cycle or layer
13:36:20 <johnthetubaguy> we will need to go through every API and fix up the validation first, I think
13:36:28 <johnthetubaguy> thats all for next cycle
13:36:34 <alex_xu> yea
13:36:35 <gmann> +1
13:37:07 <johnthetubaguy> we just focus on the framework (using keypairs as a test), then fix up servers for the cells dependency
13:37:12 <johnthetubaguy> so one extra thing...
13:37:21 <johnthetubaguy> I think we have other params in the /servers APIs
13:37:30 <johnthetubaguy> we should really validate all the params at one
13:37:32 <johnthetubaguy> once
13:38:42 <alex_xu> johnthetubaguy: what means validate at once?
13:39:23 <johnthetubaguy> so the paging parameters
13:39:30 <johnthetubaguy> I think we need to validate those a the same time
13:39:46 <johnthetubaguy> as we will have a single JSON schema for all inputs to the /servers API
13:41:17 <alex_xu> yea, for the parameters which are used in multiple apis
13:42:57 <johnthetubaguy> well, more just that spec needs to cover all the API, but yes
13:43:10 <alex_xu> yea
13:43:14 <johnthetubaguy> I added my comments in the review anyways
13:43:36 <alex_xu> johnthetubaguy: thanks!
13:43:40 <gmann> johnthetubaguy: too fast :)
13:43:51 <johnthetubaguy> did we want to look through the list of API specs, once was go into Open, see if there are any really important ones hiding in there?
13:44:09 <alex_xu> johnthetubaguy: sure, we can
13:44:36 <alex_xu> i guess diana_clarke is here for his spec
13:44:54 <alex_xu> #topic Open
13:45:45 <johnthetubaguy> so #link https://review.openstack.org/#/c/386771
13:45:48 <gmann> simple-tenant-usage pagination one ?
13:45:50 <gmann> yea
13:45:52 <johnthetubaguy> loads of replies I only just spotted
13:46:47 <johnthetubaguy> so the paging is quite funky on purely instance_uuid
13:46:53 <johnthetubaguy> but all the alternatives are way worse
13:46:58 <johnthetubaguy> so it feels like the correct choice
13:48:10 <alex_xu> i'm thinking about if the dashbard want to page in the web page, each page only show 5 tenant usage, with current propose, the dashboard still need go through all the pages to get first 5 tenant usage
13:48:28 <alex_xu> that sounds bad
13:49:31 <johnthetubaguy> so horizon could get all the pages, then show something nice, if it wanted to
13:50:47 <alex_xu> ok, they can do that, just a little slow for get all the pages
13:51:20 <johnthetubaguy> so I have two nits on the spec now:
13:51:34 <johnthetubaguy> 1) the "now" value should be included in the links
13:52:00 <johnthetubaguy> 2) this claims to solve the horizon use case, but it really solves the admin script not taking down the whole cloud case
13:52:08 <johnthetubaguy> from a protection point of view, this makes sense
13:53:27 <johnthetubaguy> so if we did paging on the tenant_id, we would still get the DDos when tenants have lots of instances
13:53:52 <johnthetubaguy> doing paging on tenant_id and instance_uuid, just doesn't seem practical
13:54:07 <alex_xu> johnthetubaguy: yea, but to be clear, i didn't mean to paging on the tenant_id, i mean sort by tenant_id first, then instance_id
13:54:22 <johnthetubaguy> ah, I was just thinking about that...
13:55:12 <alex_xu> it give the chance, the dashboard only need to go through the first few pages to get the first 5 tenants usage
13:55:53 <johnthetubaguy> so I just changed to -1 saying we should do that
13:56:07 <johnthetubaguy> the downside is you might have one tenant go over three pages
13:56:21 <johnthetubaguy> but thats less confusing than always mixing everything up
13:56:59 <alex_xu> johnthetubaguy: I have full solution at line 240
13:57:51 <alex_xu> to note, it need to add one more field 'paged=true/false' in the api, to tell the user whether this tenant usage paging is ending or not.
13:58:21 <alex_xu> 2 mins left
13:58:41 <gmann> alex_xu: but that can be known with no link available like other paging ?
13:58:46 <johnthetubaguy> hmm... that seems quite complicated, but I guess its better for horizon
13:59:04 <johnthetubaguy> gmann: you don't know if its the continuation of the same tenant or not, on the next page
13:59:15 <alex_xu> gmann: sorry I didn't get you
13:59:18 <gmann> ah, multiple tenant there
13:59:35 <alex_xu> johnthetubaguy: gmann let us back to the openstack-nova, we run out of time at here
13:59:53 <gmann> alex_xu: for single mapping we can do but with multiple tenant we need some other bit to tell
13:59:55 <alex_xu> #endmeeting