| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-27 | |||
| 19:30:40 | sdague | yeh, something like that | |
| 19:31:02 | sdague | just so the test is more concisely valid | |
| 19:31:43 | sdague | the reset seems fine to me | |
| 19:31:52 | dansmith | done | |
| 19:34:22 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix policy check performance in 2.47+ https://review.openstack.org/507948 | |
| 19:36:55 | cfriesen_ | just throwing this out there...could we use a global variable for show_extra_specs such that it's None for the first instance and then the calculated value is used for subsequent ones? That'd avoid the API changes, but globals are icky. | |
| 19:37:17 | mriedem | this isn't an api change | |
| 19:37:37 | cfriesen_ | picky picky...function signature changes | |
| 19:37:41 | dansmith | cfriesen_: that doesn't work | |
| 19:37:58 | dansmith | cfriesen_: because this code is running lots of lists for lots of people, some of which do and some of which don't have that permission | |
| 19:38:12 | cfriesen_ | set it to None at the beginning of each call | |
| 19:38:27 | mriedem | that's what this does... | |
| 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 | |