16:00:37 #startmeeting cinder 16:00:37 Meeting started Wed Mar 6 16:00:37 2019 UTC and is due to finish in 60 minutes. The chair is jungleboyj. Information about MeetBot at http://wiki.debian.org/MeetBot. 16:00:38 Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 16:00:40 The meeting name has been set to 'cinder' 16:00:45 Hi 16:00:48 hi 16:00:52 hi o/ 16:00:55 hi 16:00:59 Hello 16:01:01 o/ 16:01:14 hi 16:01:29 o/ 16:01:53 hi 16:01:56 @! 16:01:56 <_pewp_> jungleboyj ( ・ω・)ノ 16:02:14 smcginnis: You should come join me in Scottsdale before the next Snowmageddon. 16:02:41 Hello 16:02:50 That would be nice. 16:03:09 :-) Next year you can come join our guys' week for Spring Training. :-) 16:03:11 o/ 16:03:29 smcginnis: Wondering if I am going to have issues getting home Sunday though. Ugh. 16:03:57 Ok. Lets get started. 16:04:15 courtesy ping: jungleboyj diablo_rojo, diablo_rojo_phon, rajinir tbarron xyang xyang1 e0ne gouthamr thingee erlon tpsilva ganso patrickeast tommylikehu eharney geguileo smcginnis lhx_ lhx__ aspiers jgriffith moshele hwalsh felipemonteiro lpetrut lseki _alastor_ whoami-rajat yikun rosmaita enriquetaso hemna _hemna 16:04:37 hi 16:04:39 #topic announcements 16:04:51 PTL Nominations are now open. 16:05:02 hi 16:05:09 I have been PTL for a while now. 16:05:32 I am willing to continue but don't want to hold anyone up from running if they are interested in doing so. 16:05:57 jungleboyj: thank you for your hard work! 16:06:10 We are kind of at that point where Sean was, when I ran, that I am happy to continue but also think change is good for the team. 16:06:40 My time commitments keep getting to be more stretched and I don't want anyone to hesitate if they feel like they could do better. :-) 16:06:45 e0ne: Thank you! 16:06:52 It is a team effort. 16:07:07 what e0ne said 16:07:50 <_pewp_> hemna (;-_-)ノ 16:07:50 rosmaita: Thanks. 16:08:03 hemna: Welcome. 16:08:14 Does anyone have questions about that? 16:08:49 If anyone is interested or has questions feel free to reach out to me. 16:09:25 My other announcement is we are now at Milestone-3 for Stein. 16:09:38 #link https://releases.openstack.org/stein/schedule.html 16:10:08 Now is the time to transition from development to more testing. No more merging new features. 16:10:45 do we have anything for FFE? 16:10:57 We should try to get bugs addressed and merged. 16:11:05 e0ne: Not that I have seen. 16:11:56 Anyone have comments or questions there? 16:12:18 i have a patch up for a requirements change, does that have to merge this week? 16:12:29 https://review.openstack.org/#/c/641037/ 16:12:39 rosmaita: I don't believe that has to merge but sooner is better. Let me look here. 16:12:45 eharney has one also: https://review.openstack.org/#/c/641065/ 16:12:51 i didn't notice any others 16:13:10 the schedule says req freeze at m-3 16:13:12 rosmaita: Oh, this is redrobot requirements freeze. 16:13:14 So yes. 16:13:32 *requirements freeze 16:13:43 We can take a look at those and get them merged. 16:13:49 ok, cool 16:13:52 That taskflow one might be good to get in quickly to watch for side effects. 16:14:01 smcginnis: ++ 16:14:01 smcginnis: +2/A 16:14:10 e0ne: ++ 16:14:34 great, thanks everyone 16:14:36 Anything else that people want to discuss around M-3? 16:15:34 Ok. Cool. Moving on. 16:15:56 General filtering API modification 16:16:03 #topic General filtering API modification 16:16:06 whoami-rajat: 16:16:09 Hi 16:16:27 @! 16:16:27 <_pewp_> jungleboyj |。・ω・|ノ 16:16:39 So i've been digging around the general filtering API and it's bugs regarding non-admin users 16:16:48 whoami-rajat: Cool. 16:17:06 i would like to get some feedback on my findings. 16:17:19 Ok. 16:17:33 previously we used to pop out the invalid filters with a debug msg 16:17:44 after the filtering API we error out on them 16:18:21 since the filters mentioned in the resource_filters.json file are just a fraction of what all filters we support like is_public, all_tenants ... 16:18:42 we're facing a lot of unexpected errors for non-admins 16:18:54 all_tenants is a special case compared to most of these other filters IMO 16:19:15 eharney: cause all_tenants is common to multiple commands, 16:19:35 is_public is also an issue related to volume types 16:19:53 We should probably go back to existing behavior of just logging it. But I can also see why we wouldn't want to ignore something a client has requested to filter on. 16:20:25 Don't want to spam the logs. 16:21:09 well, it makes sense to distinguish between filters that are valid vs ones that will never work, as well as returning permission denied when appropriate (denied by policy etc) 16:21:28 eharney: Agreed. 16:21:40 whoami-rajat: Why are there so many errors for non-admins? 16:22:02 so i'm pretty sure the answer isn't to stop throwing errors 16:22:04 eharney: also an interesting case that may fail commands using all_tenants the revert-to-snapshot was failing because we first search for snapshots and all_tenants is automatically appended in the search_opts, so it should have generated NotFound error but generating BadRequest due to filtering API. 16:22:27 jungleboyj: would like to get my patch in for the new driver options discovery 16:22:32 it's been sitting a few days 16:22:50 jungleboyj: because while adding the support for general filtering, all filters weren't covered. 16:22:52 hemna: Hold on. 16:23:00 that could be fixed in the client, or we could handle all_tenants in a way that the client expects/expected 16:23:26 but all_tenants is more about who has privileges to see what than resource filtering, afaict 16:23:47 jungleboyj: since the filters are only for non-admins (admins are allowed for mostly all filters), this caused failure in multiple commands after 3.31 mv 16:24:41 Ok. 16:24:52 eharney: yes, popping out invalid filters also doesn't look a good approach as if client sends it intentionally we shouldn't process the request 16:24:56 we are seeing this problem as well 16:25:28 eharney: one solution could be to add all valid filters in the json but that would cause a lot of duplicacy 16:25:33 whoami-rajat: i would agree for filters in general, because we designed the server to reject invalid filters. but, the client was clearly designed before that to assume that all_tenants will work, and i think it's reasonable to treat that one separately 16:25:53 do we have a bug for this in lauchpad 16:25:54 ? 16:26:12 Sounds like this is a bug that should be tracked. 16:26:39 yeah, but it's not just a small bug, it's an area we need to actually document the design of because nobody is sure how this is supposed to work 16:26:59 another good reason for a bug :) 16:27:08 eharney: i tried to handle the case for all_tenants but then it error out for is_public, i think there may occur multiple other scenarios as well 16:27:26 :-) Just to take a step back. This is a big bug but it is a nuisance right, not a major issues. 16:27:28 hemna: i found multiple different bugs for different commands, 16:27:57 but it's a filter permissions issue no? 16:27:58 it's a couple of different bugs, the server is wrong, the client is also doing things a bit wrong 16:28:01 no 16:28:07 we have a demo account that can't list volumes 16:28:19 :-( 16:28:30 when filters are added to that list command issued 16:28:43 like....status=available 16:28:55 I think the idea to implement it was to avoid multiple checks for each filter and stack them at one place. 16:29:20 but we forgot to cover all filters possible and only added the major ones 16:29:49 forgot to cover what exactly? isn't the list of filters configurable? 16:30:05 whoami-rajat: How hard would it be to fix this by adding the filters to the json file as a bandaid and then work on the long term fix after that? 16:31:13 eharney: yes, 16:31:42 jungleboyj: depends on every resource. i can start with it if that is the intended approach. 16:31:51 adding all_tenants to the default filter list is wrong IMO 16:32:01 hehe 16:32:03 yah 16:32:14 eharney: That sounds like a special case. Right? 16:32:44 We should at least handle the non-admin filter cases by default. 16:32:47 eharney: probably needs to be added in every resource, 16:32:53 a workaround would be nice for short term, but this seems quite broken in general 16:33:00 whoami-rajat: what does? 16:33:04 hemna: Agreed. 16:33:19 eharney: all_tenants 16:33:23 * jungleboyj sadly doesn't understand this code path so well. 16:33:39 i don't think it has to be added to every resource 16:34:00 what i suggested a couple weeks ago was that we special case all_tenants and verify that it is handled as expected at a layer further into the stack, which it should be 16:34:39 all_tenants are the things that all_tenants should be able to filter upon? 16:34:45 no 16:34:49 Ugh. 16:34:57 it's an arg that says "show me resources owned by other tenants" 16:34:59 eharney: IIUC you're suggesting we should pass it through the filtering and then handle this particular as a special case? 16:35:14 and it is specifically checked all over our code with checks like "if is_admin and all_tenants..." 16:35:17 that's admin like privs 16:35:19 because it's not a normal filter 16:35:27 it's just caught by the generic filtering code 16:35:31 because it wasn't written right 16:35:35 and nobody noticed 16:35:35 jungleboyj: it's an admin action to show resources in every project 16:35:45 so we should special case it in the filtering code and do it correctly, like it used to work 16:36:32 eharney: i checked it with the previous code, it just pops out the all_tenants and proceeds normally. so a special case would be good. 16:37:27 eharney: and other filters causing issues could be directly added to resource_filters file. 16:37:28 eharney: Ok, that makes sense. 16:37:51 whoami-rajat: that just depends on what we want to allow by default vs leave up to deployers to enable 16:38:02 whoami-rajat: we probably don't want to just add everything 16:38:22 eharney: also i tried handling that in clients for the findall resource method https://review.openstack.org/#/c/641310/ 16:38:39 eharney: Oh, this was added for the read-only admin case. Right? 16:38:57 jungleboyj: what was? 16:38:58 eharney: no, not all, just some important ones like is_public for volume types. 16:39:11 Adding all_tenants 16:39:23 i think all_tenants has always been there 16:39:56 Hmmm. Ok. 16:40:02 I will just stay out of the way. 16:41:51 It sounds, however, like we should special case it and set some reasonable set of defaults to avoid spamming the logs. 16:42:22 The the defaults aren't right for the end user they will need to update accordingly. 16:42:57 it looks like we can just throw away or ignore "all_tenants" in reject_invalid_filters if the caller isn't admin 16:42:59 yeah 16:43:44 Cool. That makes sense. 16:43:52 eharney: but when a non-admin user intentionally passes all-tenants, is it ok not to error out? 16:44:20 we used to just ignore it so it didn't do anything 16:44:30 i don't see why we couldn't decide to start doing that again 16:44:55 Especially if all-tenants:false 16:45:09 That makes sense. 16:45:40 smcginnis that we used to ignore in client 16:45:56 so we want to ignore that in server now? 16:46:09 we should fix the client to not send the arg when it isn't appropriate, that's the easier part to fix though 16:46:48 Easier for the python client. There are a lot of other clients out there though, so handling this sanely on the server side is the best option. 16:47:17 we should fix both, the client shouldn't be sending all_tenants=0 for no reason 16:47:29 but the server fix is the more important for sure 16:48:14 eharney: smcginnis ok. this seems to be the better workaround to fix at both places. 16:48:21 whoami-rajat: Yep. 16:48:47 will do 16:49:11 eharney: Thank you for the input. 16:49:47 jungleboyj: eharney smcginnis thanks for all the useful feedback. 16:49:49 whoami-rajat: Know what to do? 16:50:02 jungleboyj: yes. 16:50:16 whoami-rajat: Awesome! Thank you for all the work you have been doing. 16:50:39 #action whoami-rajat to propose fixes to handle all_tenants properly on both the client and server sides. 16:51:49 Ok. So, I think that is all on that topic. 16:52:11 Last topic. rosmaita Are those the requirement changes you proposed earlier? 16:52:30 looking 16:52:36 yes 16:53:02 jungleboyj: np :) thanks. 16:53:17 Ok. So we already covered that. 16:53:21 jungleboyj: what eharney said 16:53:24 #topic open discussion 16:53:53 Anything else that needs to be discussed? 16:54:21 Oh, reminders to put topics in for the forum. 16:54:23 https://review.openstack.org/#/c/641414/ is a couple of nice reverts for someone to review 16:54:47 We seriously can't have nothing but what I proposed to talk about there: 16:54:56 #link https://etherpad.openstack.org/p/cinder-denver-forum-brainstorming 16:55:11 Are those submissions due tomorrow, of Friday. 16:55:23 Friday, so I will propose what is there. 16:55:28 Please add topics 16:55:38 hemna: You had something. 16:56:17 i think raghavendra has a patch for HPE 3par driver, he has been pinging continuously mail and IRC for the review. https://review.openstack.org/#/c/634119/ 16:57:11 oh hey 16:57:24 you mentioned feature freeze 16:57:30 whoami-rajat: Need eharney and hemna To look at the responses there. 16:57:33 hemna: Yeah. 16:57:44 so I wanted to ask for some love on my feature patch for driver options 16:57:52 hemna: Yeah. Link? 16:57:53 i'm still skeptical about that 3par patch 16:58:01 https://review.openstack.org/#/c/635255/ 16:58:20 https://review.openstack.org/#/c/639193/ 16:58:22 those 2 are related 16:58:27 one is for cinder 16:58:30 and the other is for cinderlib 16:58:52 hemna: Ok. I will try to get my love on with those. 16:59:05 thanks 16:59:54 fwiw, you can see all of the driver options in the drivers list docs now with that patch 16:59:57 http://logs.openstack.org/55/635255/10/check/openstack-tox-docs/0200672/html/drivers.html 17:00:16 and I ordered all the drivers alphabetically 17:00:40 Nice! 17:00:40 and put the unsupported drivers at the bottom 17:00:59 Ok. Will definitely take a look at the patch. 17:01:04 Ok, we are out of time. 17:01:08 Thanks everyone! 17:01:14 #endmeeting