12:59:57 <alex_xu> #startmeeting nova api
12:59:58 <openstack> Meeting started Wed Nov 16 12:59:57 2016 UTC and is due to finish in 60 minutes.  The chair is alex_xu. Information about MeetBot at http://wiki.debian.org/MeetBot.
12:59:59 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
13:00:01 <openstack> The meeting name has been set to 'nova_api'
13:00:09 <alex_xu> who is here today?
13:00:14 <gmann_> o/
13:00:20 <Kevin_Zheng> o/
13:00:48 <cdent> o/
13:00:52 <alex_xu> let us wait one more minute for johnthetubaguy get a coffee :)
13:01:22 <johnthetubaguy> ah, I meant don't wait will be back in a sec
13:01:31 <johnthetubaguy> 0/
13:01:38 <jichen> o/
13:01:39 <alex_xu> hehe
13:01:52 <alex_xu> let me whether we have action from previous
13:02:21 <alex_xu> no action from previous
13:02:29 <alex_xu> #topic priority task
13:02:34 <gmann_> :)
13:02:45 <alex_xu> #link https://review.openstack.org/388518
13:03:30 <alex_xu> ^ thanks cdent point it out actually we can validate single value with max length of list. So I changed the spec, it makes the json-schema simpler
13:03:45 <alex_xu> avoid to use 'oneOf'
13:04:05 <alex_xu> but we need use 'array' for all the parameters, but it still looks simper than 'oneOf'
13:04:27 <alex_xu> #link https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@96
13:04:39 <alex_xu> ^ you guys can find out the example from the link
13:04:40 <johnthetubaguy> yeah, just been looking at that, very cunning
13:05:11 <johnthetubaguy> so in the conversion of the query string params, if its not a list, we add it as an item of one in a list?
13:05:21 <alex_xu> johnthetubaguy: yes
13:05:36 <alex_xu> at line 87, there is example
13:05:42 <johnthetubaguy> alex_xu: did you mean to delete line 118?
13:05:55 <johnthetubaguy> oh, I see, keep the utils
13:05:57 <alex_xu> johnthetubaguy: no, still want to keep them
13:06:00 <alex_xu> yea
13:06:22 <alex_xu> I will update the poc later
13:06:46 <gmann_> alex_xu: you mean that solve the multiple occurrence of same param thing ?
13:06:51 <gmann_> or something else
13:07:10 <alex_xu> gmann_: yea
13:07:29 <alex_xu> gmann_: for the case name=abc&name=def
13:07:52 <gmann_> alex_xu: but i thought oneOf was only needed for sort_key only
13:08:16 <alex_xu> gmann_: with new way, we needn't oneOf anymore
13:09:27 <alex_xu> gmann_: for multiple value, it is array. for single value, it is array limit maxItems=1
13:09:29 <gmann_> alex_xu: but still it would not restrict name=abd&name=def right?
13:09:52 <gmann_> alex_xu: ohk, you will parse as list, got it
13:10:00 <alex_xu> gmann_: yea, we won't for now.
13:10:09 <alex_xu> so the second spec
13:10:25 <johnthetubaguy> one sec
13:10:26 <alex_xu> #link https://review.openstack.org/393205
13:10:28 <alex_xu> oops
13:10:30 <alex_xu> johnthetubaguy: go ahead
13:10:42 <johnthetubaguy> there was a note about allowing multiple values for a single one
13:10:49 <johnthetubaguy> like foo=bar,foo=baz
13:10:59 <johnthetubaguy> today we just pick one of them, or something?
13:11:02 <gmann_> alex_xu: so we will be parsing all query params into list and then schema can handle
13:11:16 <alex_xu> gmann_: yes
13:11:33 <alex_xu> johnthetubaguy: yes
13:11:38 <gmann_> johnthetubaguy: yea json schema just pick one but that same case for request body validation also
13:11:40 <johnthetubaguy> I was thinking about this bit: https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@193
13:11:44 <alex_xu> we just pick oen of them
13:12:02 <johnthetubaguy> oh, do you have an example of that?
13:12:07 <alex_xu> yea, that thanks to gmann_ point it out
13:12:41 <alex_xu> for example, change-since accept a datefomart
13:12:53 <gmann_> johnthetubaguy: if you run create server tests with 2 name there 1 invalid, still pass
13:13:17 <johnthetubaguy> so you could have multiples in the body
13:13:26 <johnthetubaguy> I am thinking about keypairs I guess
13:13:29 <johnthetubaguy> like this one:
13:13:30 <johnthetubaguy> https://git.openstack.org/cgit/openstack/nova/tree/nova/api/openstack/compute/keypairs.py#n122
13:13:50 <johnthetubaguy> it seems that would only deal if there was a single type parameter
13:14:06 <johnthetubaguy> oops
13:14:09 <johnthetubaguy> thats the body
13:14:40 <alex_xu> johnthetubaguy: there are multiple value the user put in the url. Then all the values pass into the nova api python code.
13:14:58 <alex_xu> The python code only will pick one of them(probably the last one I guess), then validate it
13:15:08 <johnthetubaguy> I thinking we have ?user_id=2,user_id=3
13:15:09 <gmann_> yea
13:15:20 <johnthetubaguy> is it the routing code that hides that?
13:15:31 <alex_xu> maybe other value are invalid, only the last one is valid, the request still can pass the validation
13:15:37 <gmann_> and json schema pick either which is valid
13:15:57 <johnthetubaguy> so once we have strict validation, that would be invalid, it has to pass if it works today
13:16:01 <johnthetubaguy> so agreed with the approch
13:16:13 <alex_xu> But with the new proposal, the json-schema will validate all the input value even the python code only pick the last one
13:16:36 <gmann_> johnthetubaguy: alex_xu i simply ran functional tests with 2 name(1 valid) and API did not complain. json schema picked valid one and ignore invalid one
13:16:41 <johnthetubaguy> yes, that seems correct, longer term
13:16:53 <gmann_> alex_xu: 1 sec
13:17:13 <johnthetubaguy> I am worried more about the query string params, I thought name was a body param?
13:17:15 <gmann_> alex_xu: i thought it will pick both and make list where json schema will 400 as maxItem is 1
13:17:29 <johnthetubaguy> for json, I believe the last one in the document wins
13:17:40 <johnthetubaguy> although its a shame we don't call that invalid
13:17:57 <alex_xu> johnthetubaguy: it isn't longer term, the json-schema will validate all the values for any request
13:18:13 <johnthetubaguy> sorry, I am crossing the streams/wires here
13:18:23 <gmann_> johnthetubaguy: even it can pick first one in case second one is invalid
13:18:45 <johnthetubaguy> I am trying to see how the current code accepts multiple and ignores one today, for the keypairs API
13:19:01 <johnthetubaguy> just the query strings for now
13:20:31 <alex_xu> emm...sorry, I lost the context also :(
13:20:38 <gmann_> alex_xu: last query- so in this spec, if user out multiple value for example for name (name=abc&name-def) it will 400 right.
13:20:49 <gmann_> out->put
13:21:10 <gmann_> as we are making list for each param and passing to json schema
13:21:27 <alex_xu> gmann_: do you mean the spec https://review.openstack.org/393205, it won't, just ignore one of them
13:21:43 <johnthetubaguy> so lets back up
13:21:54 <johnthetubaguy> I think we should focus on the real api, keypairs
13:22:01 <johnthetubaguy> thats what the spec is modifying
13:22:08 <alex_xu> yea
13:22:13 <gmann_> alex_xu: no this one  - https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@193
13:22:17 <johnthetubaguy> so I am trying to work out what happens today with the user param
13:23:29 <johnthetubaguy> I guess this bit:
13:23:30 <johnthetubaguy> https://git.openstack.org/cgit/openstack/nova/tree/nova/api/openstack/compute/keypairs.py#n185
13:23:40 <johnthetubaguy> I was going blind but I see it now
13:23:58 <alex_xu> gmann_: that spec only describe the new mechanism, whether we return 400, depend the schema
13:24:44 <gmann_> alex_xu: yea
13:24:52 <alex_xu> johnthetubaguy: it only take one of them
13:25:03 <gmann_> johnthetubaguy: yea
13:27:13 <alex_xu> johnthetubaguy: so....
13:27:48 <johnthetubaguy> in the distant future, when additional_properties = False
13:27:48 <johnthetubaguy> we would want user_id=1&user_id=2 to fail
13:27:48 <johnthetubaguy> right now we just pick the first one
13:27:48 <johnthetubaguy> it feels like we need to preserve that
13:27:48 <johnthetubaguy> for the moment
13:27:48 <johnthetubaguy> the spec says that, I believe?
13:27:48 <johnthetubaguy> I was unclear...
13:27:48 <johnthetubaguy> need to keep the same API semantics, so we will not be as strict as we could be about duplicates
13:27:49 <johnthetubaguy> now, we have a few options for the validation
13:27:49 <johnthetubaguy> 1) reject the request with duplicates, long term, we probably want that
13:27:49 <johnthetubaguy> 2) only validate the element the code will touch
13:28:14 <alex_xu> oops, I have network delay
13:28:36 <cdent> I thought the point of putting this stuff under a microversio was so that we _don't_ need to preserve the old ambiguous behavior?
13:29:15 <gmann_> but duplicates were just being not taken care, should give 400
13:29:21 <gmann_> which is broken as of now
13:29:47 <gmann_> as that was a good example of bad usage
13:30:22 <johnthetubaguy> cdent: so step one is don't break what we have, just protect us against bad stuff, step 2 is make it better
13:30:31 <johnthetubaguy> at least thats what I have in my head
13:31:16 <johnthetubaguy> basically like 2.0 vs 2.1 and the body validation
13:31:19 <alex_xu> emm...+1 for don't break what we have now
13:31:45 <johnthetubaguy> so you said the validation only takes one of them?
13:31:57 <johnthetubaguy> I thought it would get passed the full list of params
13:32:29 <alex_xu> johnthetubaguy: with current proposal, it would get passed the full list of params
13:32:38 <johnthetubaguy> yep, I am fine with that
13:32:57 <gmann_> alex_xu: and 400 for duplicate case
13:33:12 <johnthetubaguy> well, that breaks the existing requests, that would need a new microversion
13:33:27 <alex_xu> yea
13:33:38 <gmann_> johnthetubaguy: even that usage was not right
13:34:10 * alex_xu should put a real example in the REST API impact section
13:34:15 <gmann_> alex_xu: but this will return 400  -https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@102
13:34:22 <gmann_> alex_xu: in current spec
13:35:13 <gmann_> so if duplicate value then json object from query param will be like 'name': ['abc', def],
13:35:23 <johnthetubaguy> which we should totally do at some point, but is not proposed in the current spec (its mentioned as future work)
13:35:28 <alex_xu> gmann_: maybe I didn't clear about that. It is only an example to show how the new mechanism works, it didn't talk about the real would
13:35:56 <johnthetubaguy> alex_xu: +1, I think your proposals is good, if I understand it now, but an example would really help
13:36:21 <gmann_> yea that was my assumption but examle seems retricted those
13:37:00 <gmann_> alex_xu: because maxItems in example makes 400 for duplicate
13:37:05 <alex_xu> gmann_: I would try to add note clarify that
13:37:28 <gmann_> alex_xu: Thanks
13:37:41 <alex_xu> gmann_: emm....forget that one, this spec is about the real world https://review.openstack.org/393205 :)
13:37:58 <gmann_> :)
13:38:54 * johnthetubaguy hmm, I think I am seeing network delays somwhere now
13:39:02 <alex_xu> I will upda the spec after the meeting, one note for the example, and put a real world example for the thing in the rest api impact section
13:39:10 <alex_xu> s/upda/update/
13:39:54 <alex_xu> so, are we good for the base spec one? let us talk about the second one?
13:41:07 <alex_xu> I guess i'm waiting for the network delays now :)
13:41:16 <johnthetubaguy> yeah, lets talk second one
13:41:27 <alex_xu> #link https://review.openstack.org/393205
13:41:31 <gmann__> yea, sorry got disconnected
13:41:57 <gmann__> for me this looks fine, just version bump needs to be more clear
13:41:59 <alex_xu> gmann__: no worries, you didn't miss anything :)
13:42:07 <gmann__> oh :)
13:42:29 <gmann__> alex_xu: johnthetubaguy so that is version bump plan what i stated in comment ?
13:42:38 <alex_xu> gmann__: so...no version bump at here
13:43:07 <gmann__> alex_xu: yes, and in future we bump when we will reject invalid filter
13:43:21 <johnthetubaguy> yeah, the spec says there is a version bump, I thought we were going to not do that for the moment
13:43:23 <alex_xu> gmann__: yea
13:43:40 <alex_xu> oops, just found that
13:43:50 <alex_xu> that is wrong
13:43:50 <gmann__> yea just ignore for invalid filter and 400 for invalid sort_key
13:44:08 <johnthetubaguy> so lets go back to why
13:44:17 <johnthetubaguy> existing requests that worked in the past should be accepted
13:44:27 <gmann__> +1
13:44:37 <johnthetubaguy> bad requests that were dangerous need to be made "safe"
13:44:46 <johnthetubaguy> so the 500 errors can go to 400 errors
13:45:15 <johnthetubaguy> the ignore unknown keys seems to be the best approach
13:45:24 <johnthetubaguy> and fail with invalid values for know keys
13:45:41 <gmann__> johnthetubaguy: for sort_key also ?
13:45:48 <johnthetubaguy> should be the same for both I think
13:46:01 <johnthetubaguy> they are both "dangerous" in some sense
13:46:07 <gmann__> johnthetubaguy: i mean if any other key than whitelist for sort_key
13:46:14 <gmann__> johnthetubaguy: yea
13:46:32 <gmann__> totally agree with approach.
13:46:41 <johnthetubaguy> sorry, I get what you mean now
13:46:46 <johnthetubaguy> the filters are all different keys
13:46:56 <johnthetubaguy> but the sort thing is a value
13:47:09 <gmann__> yea
13:47:12 <johnthetubaguy> so it will trigger 400 errors
13:47:20 <johnthetubaguy> thats annoying, but its consistent
13:47:40 <johnthetubaguy> we support multiple sort keys?
13:48:02 <gmann__> yea invalid value gives 400
13:48:03 <alex_xu> yes
13:48:13 <alex_xu> yes for multiple sort keys
13:48:30 <gmann__> L88 says sort key not sort_key's value :)
13:48:49 <alex_xu> "so the 500 errors can go to 400 errors": for the filters, it go to ignore, not 400
13:49:20 <alex_xu> or you want that go to 400?
13:49:56 <johnthetubaguy> so it gets tricky
13:50:22 <johnthetubaguy> we could have a *these are bad* list, and reject those, but ignore unknown values, but that seems like a mess
13:50:44 <gmann__> ah, yea
13:50:44 <johnthetubaguy> I think I am OK with filters we don't know, just getting the standard ignore unknown key treatment for now
13:51:35 <alex_xu> if user use that bad key, that means the bad request will break their client code?
13:52:25 <johnthetubaguy> right, hence the ignore / strip approach
13:52:39 <gmann__> alex_xu: but current code ignore that right? for admin (as we have only non-admin list in code)
13:53:26 <johnthetubaguy> operator feedback on the restriction is probably a good idea, but I am not sure what we do if they don't like it
13:53:38 <alex_xu> gmann__: I thought the "bad" point to the joined table and column without index
13:53:52 <gmann__> for me too, ignore/strip approach is more better way to go
13:54:34 <johnthetubaguy> going back to that, we seem to be back to having a massive list in the spec
13:56:20 <alex_xu> ok, so we continue shrik the list
13:56:37 <alex_xu> 4 mins left for us
13:56:39 <johnthetubaguy> well, we need it to match what we have indexed
13:56:44 <johnthetubaguy> I believe thats the aim
13:56:55 <johnthetubaguy> that might involve adding an index or two, if we really want to keep some
13:57:13 <alex_xu> ok
13:57:35 <gmann__> humm
13:58:02 <alex_xu> Kevin_Zheng: is the currently all the proposal filter/sort includes index?
13:58:10 <Kevin_Zheng> no
13:58:16 <johnthetubaguy> I am just checking, we are missing loads of them
13:58:21 <johnthetubaguy> https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L196
13:58:43 <johnthetubaguy> note that users filter for deleted=false, hence the compound index
13:59:21 <Kevin_Zheng> some useful fields missing indexes
13:59:30 <Kevin_Zheng> like user_id
13:59:52 <Kevin_Zheng> pretty common to filter by user
13:59:57 <alex_xu> it's time to close, we should move back to #openstack-nova
14:00:07 <alex_xu> #endmeeting