13:01:05 <alex_xu> #startmeeting nova api
13:01:06 <openstack> Meeting started Wed Jan 18 13:01:05 2017 UTC and is due to finish in 60 minutes.  The chair is alex_xu. Information about MeetBot at http://wiki.debian.org/MeetBot.
13:01:08 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
13:01:10 <openstack> The meeting name has been set to 'nova_api'
13:01:18 <alex_xu> who is here today?
13:01:26 <Kevin_Zheng> o/
13:01:33 <gmann> o/
13:01:42 <johnthetubaguy> o/
13:02:01 <cdent> o/
13:02:15 <alex_xu> let us start the meeting
13:02:19 <alex_xu> #topic open
13:02:31 <alex_xu> #link https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:master+topic:bp/add-whitelist-for-server-list-filter-sort-parameters
13:03:15 <alex_xu> Kevin_Zheng and I work on those patches, and I think they are close
13:03:58 <alex_xu> the last patch is reno and api-ref
13:04:50 <gmann> nice
13:04:54 <alex_xu> anyone have more concern about it?
13:05:00 <johnthetubaguy> my memory is bad
13:05:10 <johnthetubaguy> just recovering from last two days in bed
13:05:15 <gmann> about policy deprecation ? should we log and add in sample file too ?
13:05:35 <johnthetubaguy> why do we filter the search params?
13:05:36 <gmann> i mean how user will get to know other than reno
13:06:22 <johnthetubaguy> gmann: thats something we should look at next cycle, policy deprecation, renames, and docs
13:06:36 <johnthetubaguy> but reno at a minimum I feel
13:06:55 <gmann> johnthetubaguy:  ok
13:07:45 <johnthetubaguy> anyone remember where the search param ignore list came from?
13:08:19 <alex_xu> johnthetubaguy: http://specs.openstack.org/openstack/nova-specs/specs/ocata/approved/add-whitelist-for-server-list-filter-sort-parameters.html#proposed-change
13:08:31 <alex_xu> the begin of propose section
13:08:41 <Kevin_Zheng> everything will be ignored except joined table ones and internal keys
13:08:52 <Kevin_Zheng> start with _
13:09:06 <Kevin_Zheng> 400 will raise for those
13:09:14 <johnthetubaguy> so we were matching the filters I guess
13:10:02 <johnthetubaguy> good point, the current change is a very "breaking" one, in the raising 400 sense
13:10:22 <alex_xu> johnthetubaguy: you mean the sort keys? it is for not breaking the user. we disable some sort keys without microversion and return 400. that will break the user directly
13:10:48 <alex_xu> yea
13:11:22 <Kevin_Zheng> Yeah just saw your comment What I said was filter
13:11:55 <johnthetubaguy> alex_xu: not sure, I am trying to remember what happened before the sort key change
13:12:56 <johnthetubaguy> so my real feedback here is I am +2 the first one, I am still working through the other changes
13:13:17 <johnthetubaguy> only came back online this morning after signing off on friday :(
13:13:44 <alex_xu> i see, the whole series have too much detail :)
13:13:53 <gmann> 1 question
13:13:56 <gmann> should we add updated_at in  filter also as we do for sorting ?
13:14:02 <alex_xu> johnthetubaguy: just ping me or Kevin_Zheng when you need more info
13:14:17 <alex_xu> gmann: change-since is equal to updated_at
13:14:48 <johnthetubaguy> will do
13:14:57 <gmann> oh yea, thanks
13:15:15 <alex_xu> np
13:15:34 <johnthetubaguy> oh, so the policy thing...
13:15:51 <johnthetubaguy> we could make the new rule default to one of the old rules?
13:16:03 <johnthetubaguy> so its more backwards compatible
13:16:42 <alex_xu> johnthetubaguy: the old rule is open for anyone now
13:17:04 <Kevin_Zheng> and new one is soft
13:17:14 <johnthetubaguy> so think about the deployer
13:17:14 <alex_xu> for we can remove it in next release
13:17:22 <johnthetubaguy> they have modifed policy
13:17:30 <johnthetubaguy> and set the index:all_tenants rule
13:17:44 <johnthetubaguy> they upgrade, and now we ignore their rule change, and use some new policy rule
13:17:52 <johnthetubaguy> this breaks our upgrade promise around configuration
13:18:22 <alex_xu> if the old rule is more strict, then nothing break. it due to we check the old rule first
13:18:25 <johnthetubaguy> if the new rule defaults to the old rule, and we log a warning message if the old rule is different to the default, then it would be better
13:18:55 <alex_xu> if the old rule is more relax, yes, the new rule will strict that again
13:19:00 <johnthetubaguy> right
13:19:12 <johnthetubaguy> which breaks them
13:19:37 <johnthetubaguy> we need some patterns around how we change policy
13:19:48 <alex_xu> johnthetubaguy: but we need change the old rule back to admin-only
13:19:52 <gmann> yea nice point
13:20:01 <johnthetubaguy> we clearly are not there yet, as we need help from oslo.policy I feel
13:20:02 <johnthetubaguy> right, we would
13:20:16 <johnthetubaguy> but we can log a warning for users that have changed the default, warning them to update their policy file
13:20:53 <alex_xu> emm...this is bad for new deployment user
13:21:02 <johnthetubaguy> why?
13:21:25 <gmann> but warning will prevent new deployment  to use old rules
13:21:27 <johnthetubaguy> if they modify the older rule, we log a message telling them its bad, and they should modify the new policy file
13:21:35 <gmann> yea
13:23:01 <alex_xu> oops, I miss-understood, log a warning for changed the default is good
13:23:35 <johnthetubaguy> now the default is in code, we can do that kind of thing
13:23:43 <johnthetubaguy> longer term olso.policy should do all that for us
13:23:50 <johnthetubaguy> but for now, its manual I think
13:23:59 <gmann> johnthetubaguy: is 1 cycle enough for user to switch on policy file? i mean people may upgrade after 2-3 cycle may be
13:25:09 <johnthetubaguy> gmann: we only do one for config, so that seems fine for policy
13:25:33 <johnthetubaguy> gmann: we have never supported N -> N+2 or N+3 upgrades
13:25:47 <gmann> yea
13:27:23 <johnthetubaguy> on the regex parameter front, there is a curious bug that has a review up
13:27:55 <johnthetubaguy> https://review.openstack.org/#/c/392305/
13:28:19 <johnthetubaguy> basically
13:28:36 <johnthetubaguy> its possible to set a field like name to a value that isn't a valid regex
13:28:45 <johnthetubaguy> when you filter with the exact name, you get an error today
13:29:02 <johnthetubaguy> but really, you would expect us to fall back to an exact match, and return that server
13:29:33 <johnthetubaguy> I am curious what you think
13:29:44 <johnthetubaguy> https://launchpad.net/bugs/1523668
13:29:44 <openstack> Launchpad bug 1523668 in OpenStack Compute (nova) "valid instance name before proceed" [Medium,In progress] - Assigned to Sujitha (sujitha-neti)
13:30:01 <johnthetubaguy> more context above
13:30:46 <cdent> weird
13:30:57 <alex_xu> interesting
13:31:06 <johnthetubaguy> its honestly not something I had thought about, but its super interesting
13:31:28 <johnthetubaguy> totally and edge case, but folks are hitting this in production it seems
13:31:43 <alex_xu> is it ok to fix that without microversion
13:32:18 <gmann> for me, we should allow to return if accepting that while creating
13:32:19 <johnthetubaguy> its error / no results -> a result, so its purely a question of discoverability of the fix
13:32:54 <gmann> yea, without microversion it is risky
13:34:05 <alex_xu> what people think about regex matching?
13:34:47 <alex_xu> we really have a lot of parameters are pattern matching https://review.openstack.org/#/c/420494/6/nova/api/openstack/compute/schemas/servers.py
13:34:47 <johnthetubaguy> I am wondering about IPv6 addresses and regex
13:34:53 <johnthetubaguy> that may not go well
13:35:24 <johnthetubaguy> its tempting to do a microversion that restricts the regex to a very small number
13:35:33 <johnthetubaguy> but the ship has sailed I guess
13:35:53 <johnthetubaguy> with search light it becomes easier I guess
13:36:00 <johnthetubaguy> s/ //
13:37:00 <alex_xu> at least I think https://review.openstack.org/#/q/topic:bp/add-whitelist-for-server-list-filter-sort-parameters just keep the API behaviour same as for now. Then think about that bug separately
13:37:50 <alex_xu> Kevin_Zheng: just found 'created_at' work with regex? the type is datetime I think
13:38:40 <Kevin_Zheng> I did test and it works
13:38:55 <gmann> and what it gave? empty
13:39:11 <Kevin_Zheng> but didn't dig too deep
13:39:33 <Kevin_Zheng> for corect number like 2016
13:39:44 <Kevin_Zheng> we will have result
13:39:54 <Kevin_Zheng> for string like abc
13:40:04 <Kevin_Zheng> empty list returned
13:40:11 <johnthetubaguy> so today we only validate name right?
13:41:04 <johnthetubaguy> I assume you get 500s for invalid regex for the other params I guess?
13:41:05 <alex_xu> johnthetubaguy: in api layer, yes. the db will raise exception for invalid regex, which lead to 500
13:41:26 <johnthetubaguy> OK, so this is just removing 500s we know about, which is reasonable.
13:41:35 <alex_xu> yea
13:41:41 <gmann> yea
13:42:18 <gmann> we have coordinate between this bug fix and BP changes  - https://review.openstack.org/#/c/392305/8/nova/api/openstack/compute/servers.py
13:42:41 <gmann> because if we merge that bug fix we have to do same in schema side also where we do regex for name
13:44:57 <johnthetubaguy> so I am +2 that regex check now
13:45:04 <johnthetubaguy> I think it makes sense to kill the 500s
13:45:11 <johnthetubaguy> we can sort out the rest of things after that
13:45:17 <alex_xu> ++
13:45:22 <gmann> yea
13:45:54 <johnthetubaguy> I am just uncertain about the sort params, and that black list that we now ignore
13:47:14 <alex_xu> I remember we discuss that in the meeting, we all agree to ignore is best choice for not breaking user
13:47:15 <johnthetubaguy> I am just not sure why we are rejecting things, I guess before we hit 500 error in the DB layer?
13:47:29 <johnthetubaguy> for totally unknown things
13:47:34 <johnthetubaguy> like foo_bar
13:47:47 <alex_xu> foo_bar return 400 in sort_key
13:47:57 <johnthetubaguy> after our change it does
13:48:01 <johnthetubaguy> what happens today?
13:48:08 <alex_xu> it return 400 in today also
13:48:36 <johnthetubaguy> how, I am curious?
13:48:46 <gmann> but say on 'locked' what is today result
13:49:33 <alex_xu> johnthetubaguy: due to oslo.db handle that https://github.com/openstack/oslo.db/blob/master/oslo_db/sqlalchemy/utils.py#L190
13:50:01 <johnthetubaguy> alex_xu: I thought that gave us a 500 error?
13:50:09 <johnthetubaguy> I guess we handle that
13:50:49 <alex_xu> johnthetubaguy: if we input joined_table in filters, we will get 500, due to that handle in nova, and we just use 'getattr'
13:51:18 <johnthetubaguy> its the sort I am focusing in
13:52:05 <johnthetubaguy> ah.... https://github.com/openstack/nova/blob/a5a94cefb58512217f0042ffaa153a9c3af033f0/nova/db/sqlalchemy/api.py#L2247
13:53:13 <johnthetubaguy> anyways, that answers one set of worries, thanks
13:53:40 <johnthetubaguy> gmann I guess "locked" today just sorts true/false all the instances?
13:54:20 <gmann> and we will ignore now. is it fine?
13:56:08 <johnthetubaguy> I am not sure I like that
13:56:16 <johnthetubaguy> because "locked" is in the filters list
13:56:31 <johnthetubaguy> alex_xu: is it worth a rebase, so https://review.openstack.org/#/c/415142 is last in the series here?
13:56:47 <johnthetubaguy> that seems the controversial one now.
13:57:14 <gmann> not 'locked' its 'locked_by' in filter actually
13:57:55 <johnthetubaguy> hmm, good point
13:58:00 <johnthetubaguy> oops
13:58:03 <alex_xu> yea
13:58:24 <alex_xu> too much confuse in the filter and sort keys world
13:58:38 <johnthetubaguy> +1 for it being a messy
13:58:44 <johnthetubaguy> s/messy/mess/
13:58:47 <gmann> not 'locked' its 'locked_by' in filter actually :)
13:58:51 <gmann> opps
13:58:57 <alex_xu> remind: 2 mins left
13:58:59 <gmann> too many keys to handle :)
13:59:48 <alex_xu> so for now we need check the policy deprecation
14:00:11 <alex_xu> johnthetubaguy: and ping me and Kevin_Zheng in nova channel for any question
14:00:15 <alex_xu> #endmeeting