13:00:21 <alex_xu> #startmeeting nova api
13:00:22 <openstack> Meeting started Wed Jan 25 13:00:21 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:00:23 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
13:00:25 <openstack> The meeting name has been set to 'nova_api'
13:00:33 <alex_xu> who is here today?
13:00:40 <Kevin_Zheng> o/
13:00:52 <gmann> o/
13:01:44 <alex_xu> looks like just three of us
13:01:54 <gmann> yea
13:02:10 <Kevin_Zheng> https://review.openstack.org/#/c/421760/
13:02:27 <Kevin_Zheng> please also review he doc patch if have time?
13:02:31 <gmann> or let's wait for few min for johnthetubaguy  sdague
13:02:33 <alex_xu> Kevin_Zheng: yea
13:02:56 <Kevin_Zheng> I may update again tommorow
13:02:58 <gmann> Kevin_Zheng: sure, ll check in morning tomorrow
13:03:10 <johnthetubaguy> oops, hello
13:03:51 <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:57 <alex_xu> ^ the last three patches
13:04:16 <alex_xu> johnthetubaguy: the policy one is ready, hope you can take a look at it
13:05:59 <johnthetubaguy> so the logging, we probably don't want to do all that logging on every API request
13:06:27 <johnthetubaguy> maybe we can check that in the API controller constructor? would that work?
13:06:58 <alex_xu> emm...yea, that is a little annoying
13:07:00 <gmann> johnthetubaguy:  you mean for old rule?
13:07:09 <johnthetubaguy> yeah
13:07:31 <alex_xu> in the controller constructor sounds doable
13:08:12 <gmann> but that will be only when all_tenants
13:08:31 <johnthetubaguy> its really logging to the operators about their config
13:08:41 <johnthetubaguy> its not really anything to do with a particular API request as such
13:08:56 <alex_xu> we have another warning at https://github.com/openstack/nova/blob/master/nova/policy.py#L83
13:09:02 <gmann> ah i see your point
13:09:47 <alex_xu> that warn on every policy config change
13:09:50 <johnthetubaguy> alex_xu: ah, well remembered, we should probably copy that pattern
13:09:59 <johnthetubaguy> I had forgotten about that one
13:10:34 <gmann> +1
13:11:04 <alex_xu> we can put that warning in the policy.init() also
13:11:41 <johnthetubaguy> I was kinda expecting to have the defaults deal with falling back to the old rules
13:11:44 <johnthetubaguy> did that not work?
13:11:44 <gmann> with is_rule_override check it can be more aligned when old rules are overridden
13:12:38 <johnthetubaguy> all_tenants_visible = rule:detail:get_all_tenants and rule:index:get_all_tenants (or something like that)
13:13:08 <johnthetubaguy> I guess thats been done in code, just wondering what pattern we should choose for these things
13:13:45 <gmann> johnthetubaguy: but if operator use to have policy.json from the sample and override the interesting one then it might be issue
13:13:54 <gmann> but not sure if they do this way
13:14:18 <johnthetubaguy> thats fine, they overrode it, so it does the right thing
13:14:45 <alex_xu> johnthetubaguy: that doesn't work for new deployer. due to the old one always return 403 when fail
13:15:05 <johnthetubaguy> why? all the rules have the same default?
13:15:30 <johnthetubaguy> oh, wait, I see your point now
13:15:33 <alex_xu> the new behaviour expect no 403 when fail.
13:15:36 <johnthetubaguy> you are trying to keep the 403 behaviour
13:15:54 <johnthetubaguy> hmm... I wasn't expecting us doing that
13:16:11 <gmann> i think no 403 and give at least their own servers
13:16:46 <johnthetubaguy> doesn't that change need a microversion, with the new rule we 403 before it, and after it everyone leaves it soft
13:17:31 <johnthetubaguy> I guess its dropping an error, so maybe thats OK
13:17:37 <alex_xu> emm...whether it return 403 still depend on policy config
13:18:46 <alex_xu> that isn't something resolved by microversion. it should be resovled by policy discovery
13:20:33 <johnthetubaguy> lets step back
13:21:26 <johnthetubaguy> right now, all_tenants sends 403 if an admins (or whatever policy says)
13:21:52 <alex_xu> yup
13:21:53 <johnthetubaguy> after this, we don't send 403... unless the admin has overridden the old policy rule, which we want to delete next cycle
13:22:18 <gmann> yea
13:22:21 <johnthetubaguy> I think my comment about working with the old policy has confused things here
13:22:54 <johnthetubaguy> what I mean is, you can only view other tenants servers when you pass a certainly policy rule
13:23:34 <johnthetubaguy> we are basically renaming that rule
13:23:49 <johnthetubaguy> so across upgrade, we need to respect settings on the old rules for now
13:23:54 <johnthetubaguy> I think we are all agreed with that aim
13:24:05 <alex_xu> yes
13:24:06 <gmann> yes
13:24:09 <johnthetubaguy> cool
13:24:12 <Kevin_Zheng> yeah
13:24:21 <johnthetubaguy> so you could consider that one patch
13:24:28 <johnthetubaguy> now there is a second patch in there
13:25:27 <johnthetubaguy> when you pass all_tenants=True, the API should return as many instance as you are allowed to see given your token, rather than doing 403 if you fail a policy check to list instances from all tenants in the system
13:25:52 <johnthetubaguy> part of me wonders if that needs a microversion
13:26:19 <johnthetubaguy> it feels strange to me for the API to flip between the old behaviour and new behaviour based on the policy file, given we want to kill the old behaviour next cycle
13:26:19 <gmann> but that is with new policy rule.
13:26:51 <johnthetubaguy> no, we want the new behaviour
13:27:01 <johnthetubaguy> we could keep the old policy rules
13:27:40 <johnthetubaguy> but with the new behaviour, the old policy rule names don't really make any sense
13:28:06 <johnthetubaguy> I am not every good at describing my concern here I guess
13:28:38 <alex_xu> so you want to rename it first
13:28:46 <alex_xu> then microversion with behaviour change
13:30:10 <johnthetubaguy> actually my preferece is the other way around
13:30:12 <cdent> ah microversions... :)
13:31:16 <johnthetubaguy> whether we want a microversion or not is a different debate, it feels like we are changing the API to me, but I could be looking at this wrongly
13:31:26 <gmann> even that is something depends on policy setting and each cloud can have different behavior based on their policy setting
13:32:14 <gmann> but yea if cloud with same policy setting then, API can behave differently
13:33:10 <johnthetubaguy> I don't think we want different behaviours based on config
13:34:34 <johnthetubaguy> we do want admins to be able to upgrade without accidentally changing who is able to list all the instances on the system, because they have modified their policy rules, and we didn't warn them about the change
13:35:27 <johnthetubaguy> we can have different behaviours based on different microversions though, that seems just fine
13:37:03 <alex_xu> so microversion with new behaviour. but new rule just copy the rule of old one
13:38:15 <gmann> but when policy rules are modified, anyways behavior can be changed when admin upgrade (currently also)
13:38:48 <johnthetubaguy> gmann: thats fairly terrible, we should stop that
13:39:14 <johnthetubaguy> now we need oslo.policy to help us more, eventually, but we just are not there yet
13:39:42 <johnthetubaguy> my concern is based on this
13:39:42 <johnthetubaguy> https://governance.openstack.org/tc/reference/tags/assert_supports-upgrade.html#requirements
13:39:53 <johnthetubaguy> and because I believe policy = configuration
13:42:08 <alex_xu> johnthetubaguy: yes, we just implement that. so you concern on microversion?
13:42:17 <alex_xu> sorry, i'm a little lost...
13:42:21 <gmann> humm
13:42:46 <Kevin_Zheng> old rule can work on the current patch, I think
13:44:23 <johnthetubaguy> alex_xu: that was just a comment about how we have failed to do this properly in the past
13:44:24 <gmann> johnthetubaguy: can new rule be considered as new config option kinnda ?
13:44:38 <johnthetubaguy> so it is a new config, effectively
13:44:48 <johnthetubaguy> I really don't like us changing the API behaviour based on a config
13:45:04 <johnthetubaguy> the odd bit, is we plan on removing the old behaviour and old config next cycle
13:45:17 <gmann> yea that's true
13:45:23 <alex_xu> yea
13:45:26 <johnthetubaguy> that just seems odd to me, struggling to describe why
13:45:40 <gmann> now i got your point
13:45:47 <johnthetubaguy> for me, we use microversions to create API behaviour changes
13:45:57 <johnthetubaguy> and advertise them correctly, etc
13:46:22 <gmann> agree on this now.
13:46:39 <Kevin_Zheng> ok
13:46:40 <gmann> otherwise it can be interop issue
13:47:17 <johnthetubaguy> now I think its easier to think about this in two steps
13:47:52 <johnthetubaguy> one change adds a microversion to make all_tenants no longer trigger 403, the old policy rules just decide if you show servers from all tenants, when you request all_tenants=True
13:48:24 <johnthetubaguy> the second change can rename the policy rules, with some backwards compatibility, because the old rule names don't really make sense with the new behaviour
13:49:25 <alex_xu> so whether we return 403 in old microversion?
13:49:52 <johnthetubaguy> in the old microversion, yes, the new policy rule decides if its 403 or allowed
13:50:04 <johnthetubaguy> just like the old ones did before it
13:50:18 <gmann> but with old rule support right
13:50:52 <johnthetubaguy> the rename of the rule has to be backwards compatible, yes
13:51:41 <johnthetubaguy> whatever those rules say, it gets used in the old microversion to return 403, and in the new microversion its a silent ignore or list instances across all tenants on the system
13:51:59 <gmann> yea
13:52:14 <johnthetubaguy> now, there is a debate of whether its worth a microversion or not, but I don't think the policy settings should decide the behaviour
13:52:51 <johnthetubaguy> if there is no microversion, I think everyone should have the new behaviour
13:53:44 <Kevin_Zheng> maybe a microversion bump when remove the old rule and change the behaviour for new rule?
13:53:57 <gmann> but still that change behavior if doing for everyne
13:54:18 <johnthetubaguy> yeah, as long as its the same behaviour for everyone, that could work for me
13:54:45 <gmann> i mean app moving from upgareded cloud to old cloud can break
13:55:00 <gmann> no 403 -> 403
13:55:14 <alex_xu> 5 mins left
13:55:27 <johnthetubaguy> really its only people depending on getting 403 that would break
13:55:53 <johnthetubaguy> but yeah, a break is possible, unless its microversioned
13:56:21 <gmann> yea it can break interoperability
13:56:44 <alex_xu> yea, althought the 403 is generic error people processed
13:57:25 <johnthetubaguy> you could use that 403 to check if you had an admin token, duno why you would, but you could
13:57:54 <gmann> also app, on upgraded cloud where no 403 if decide to move to old non-upgraded cloud where 403
13:57:55 <alex_xu> emm...yes
13:58:37 <gmann> i mean we changing error to success which is issue. error-> good error only ok
13:58:38 <alex_xu> one news, I will start the vacation from tomorrow. Kevin_Zheng will be there for tomorrow?
13:58:56 <Kevin_Zheng> yeah
13:59:00 <johnthetubaguy> alex_xu: have a good break
13:59:00 <alex_xu> probably I will help on the patch tonight. but i'm not sure I have network tomorrow evening
13:59:04 <gmann> alex_xu:  i can take up those updates, i am in for next week
13:59:13 <alex_xu> johnthetubaguy: gmann thanks
13:59:49 <Kevin_Zheng> could you also update the doc one? already did some work
13:59:49 <gmann> so we should go with microversion on behavior change ? as johnthetubaguy mentioned on 2 changes
13:59:52 <alex_xu> ok, we should back to nova channel, it is time to close the meeting
13:59:57 <gmann> Kevin_Zheng: sure ll do
14:00:09 <alex_xu> #endmeeting