| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-27 | |||
| 19:38:29 | mriedem | w/o a global | |
| 19:38:37 | dansmith | cfriesen_: that also won't work | |
| 19:38:54 | dansmith | cfriesen_: because we don't necessarily complete a whole call through the stack before we go on to the next one | |
| 19:39:00 | dansmith | cfriesen_: that would be known as a "CVE" | |
| 19:39:30 | cfriesen_ | dansmith: due to eventlets I guess? | |
| 19:39:46 | dansmith | threads in general | |
| 19:40:02 | mriedem | i wonder if the fedex guy is required to jog from the truck to the house and back | |
| 19:40:18 | mriedem | like, is there a camera watching him to make sure he jogs? | |
| 19:40:35 | dansmith | there is at my house | |
| 19:40:45 | dansmith | I call and complain any time he saunters instead of jogs | |
| 19:40:54 | mriedem | what if he mosey's? | |
| 19:41:01 | cfriesen_ | dansmith: I thought we were using processes for nova-api, not threads | |
| 19:41:15 | dansmith | mosey is a saunter with slightly more vigor | |
| 19:41:23 | mriedem | more tude | |
| 19:41:35 | dansmith | cfriesen_: there are threads (greenthreads currently) in each process | |
| 19:41:55 | cfriesen_ | ah, got it. | |
| 19:41:59 | dansmith | cfriesen_: but seriously, setting a global for a permission flag and hoping it gets reset before the next call is like the worst idea ever :) | |
| 19:42:41 | cfriesen_ | I'm pretty sure I've had worse. :) I just didn't like the fact that we were checking it in two different places depending on flow. | |
| 19:43:10 | cfriesen_ | In C/C++ I'd just pass a pointer or pass it by reference. | |
| 19:43:34 | mriedem | in fortran i'd goto that mothertrucker | |
| 19:43:40 | cdent | dansmith: None and False meaning different things. ballsy. | |
| 19:43:57 | cfriesen_ | I saw old fortran back in my engineering days that had goto with multiple targets....that was messed up. | |
| 19:44:13 | dansmith | cdent: they are wholly different things :) | |
| 19:44:19 | sdague | cfriesen_: if you wanted to handle it in a different way, the nova local caching model would be just to make context.can cache the parameter lists and answers | |
| 19:44:21 | cdent | sure, but still | |
| 19:44:29 | sdague | because contexts are constructed all the time | |
| 19:44:45 | sdague | and that would fastpath the check in tight loops like this | |
| 19:45:06 | dansmith | sdague: so I was thinking about a way to make the fixture raise if a test checked the same exactly policy action/target in a single run | |
| 19:45:12 | cfriesen_ | sdague: yes, that'd be nice. but this isn't horrible | |
| 19:45:16 | dansmith | sdague: to ferret out some of what you were saying might be there | |
| 19:45:23 | dansmith | cfriesen_: sdague: this has to be minimal backport though | |
| 19:45:29 | dansmith | because this _has_ to be backported | |
| 19:45:34 | dansmith | else we lose our license to code | |
| 19:45:41 | cfriesen_ | agreed | |
| 19:45:49 | mriedem | retain our license to ill? | |
| 19:46:08 | sdague | dansmith: yeh, I think context.can is the right place to shadow that | |
| 19:46:15 | sdague | in tests | |
| 19:46:21 | dansmith | sdague: yeah | |
| 19:46:47 | sdague | I also really wonder how much better perf would get if we remove the policy reload entirely | |
| 19:47:07 | sdague | because that's just kind of there because of the early rax operational model | |
| 19:47:11 | dansmith | sdague: if you can give me a line to comment out I can run it on my rig while it's still built | |
| 19:47:22 | mriedem | i think dims was looking at making the policy check call an external service...if that makes you worry at all | |
| 19:47:47 | sdague | mriedem: yeh, I think this small thing has pretty much shown you can only do that if it's a load on startup | |
| 19:47:54 | sdague | otherwise you are toast | |
| 19:48:09 | sdague | dansmith: yeh, let me go look, it's been a minute since I've been in that code | |
| 19:48:13 | dansmith | mriedem: jesus | |
| 19:49:07 | bauzas | dansmith: sdague: mriedem: any reason why https://review.openstack.org/#/c/507948/3 isn't yet +W'd ? | |
| 19:49:12 | bauzas | can I pull the trigger? | |
| 19:49:43 | mriedem | apparently there is a cache https://github.com/openstack/oslo.policy/blob/master/oslo_policy/policy.py#L630 | |
| 19:50:08 | dansmith | mriedem: that means we're still doing the fstat() each time right? | |
| 19:50:20 | dansmith | that's the painful bit I think, especially given policy files are empty now | |
| 19:50:39 | mriedem | true | |
| 19:50:40 | dansmith | actually two calls | |
| 19:50:45 | dansmith | exists and getmtime | |
| 19:50:48 | mriedem | just return True from _is_directory_updated all the time? | |
| 19:51:29 | dansmith | or only check the directory if it's been 30s since we last did it | |
| 19:51:55 | dansmith | or only check it after you've sighup'd :) | |
| 19:52:48 | cdent | inotify | |
| 19:52:55 | dansmith | I guess you meant for this q&d check, yeah | |
| 19:53:09 | mriedem | yeah just change that to return False all the time | |
| 19:53:17 | mriedem | it would be False based on how it's used | |
| 19:53:29 | mriedem | see if you shave a few seconds | |
| 19:54:20 | sdague | mriedem: yeh, that's the stat calls | |
| 19:54:40 | sdague | _is_directory_updated is different though | |
| 19:55:00 | sdague | because that's the different terrible issue of a policy.d | |
| 19:55:07 | sdague | which makes this *N worse | |
| 19:55:13 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Fix policy check performance in 2.47+ https://review.openstack.org/507965 | |
| 19:55:28 | dansmith | bauzas: pull the trigger | |
| 19:55:36 | dansmith | sdague: so wait, is there more I need to kill? | |
| 19:55:40 | bauzas | ack | |
| 19:55:45 | openstackgerrit | melanie witt proposed openstack/nova master: Set group_members when converting to legacy request spec https://review.openstack.org/507938 | |
| 19:55:54 | bauzas | honestly, I think fixing it now is better anyway | |
| 19:56:17 | bauzas | hah, jinxed | |
| 19:58:37 | sdague | dansmith: I think you want to short circuit here - https://github.com/openstack/oslo.policy/blob/70ba1beb3e3c93fafc147633360df838155a82a9/oslo_policy/_cache_handler.py#L31 | |
| 19:58:46 | sdague | and just return (True, "") | |
| 19:58:57 | sdague | actually (False, "") | |
| 19:59:40 | mriedem | "A tuple with a boolean specifying if the data is fresh or not" in the docstring doesn't seem to match the code | |
| 20:00:00 | sdague | mriedem: yeh it does | |
| 20:00:02 | mriedem | if the data "was refreshed" | |
| 20:00:15 | sdague | https://github.com/openstack/oslo.policy/blob/70ba1beb3e3c93fafc147633360df838155a82a9/oslo_policy/policy.py#L676 | |
| 20:00:40 | sdague | mriedem: ah, ok, english binary reversal | |
| 20:00:43 | mriedem | right | |
| 20:00:54 | sdague | honestly, I read it in the reversed context the first time | |
| 20:01:06 | dansmith | I thought I should return (False, cache['filename']['data']) no? | |
| 20:01:10 | mriedem | it's not not unfresh?! | |
| 20:01:23 | sdague | dansmith: it could, but it could also just return garbage | |
| 20:01:31 | dansmith | okay | |
| 20:01:32 | sdague | because if the first param is False, nothing is done with it | |
| 20:01:51 | dansmith | ah, okay | |
| 20:01:54 | sdague | "" would be the production default state | |
| 20:02:00 | sdague | as policy.json is blank | |
| 20:02:00 | dansmith | oh, right, | |
| 20:02:29 | dansmith | was thinking I had to return the actual thing | |
| 20:05:01 | sdague | though, I think the impact is going to be minimal, given that the issue was 1000 of these calls, which added 3s in your env, so were looking at 3ms per policy check | |
| 20:05:24 | dansmith | yeah | |
| 20:05:25 | sdague | so, unless we have another nested thing, it's going to be hard to see the impact | |
| 20:05:36 | dansmith | I see no impact as the first numbers are coming out | |
| 20:09:01 | mriedem | melwitt: couple comments on the test, but looks great otherwise | |
| 20:13:02 | melwitt | thanks, looking | |
| 20:13:48 | dansmith | mriedem: sdague: not likely any difference: https://imgur.com/a/gSIlq | |