| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-27 | |||
| 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 | dansmith | oh, right, | |
| 20:02:00 | sdague | as policy.json is blank | |
| 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 | |
| 20:18:47 | mriedem | sdague: i just git log -i --grep'ed for the first time i think | |
| 20:18:55 | mriedem | after you've said it in channel once per week i think | |
| 20:19:14 | mriedem | it might be the first thing you say when you wake up | |
| 20:19:15 | mriedem | not sure | |
| 20:22:22 | efried | oo. TIL. | |
| 20:26:05 | sdague | dansmith: yeh, it's clear the current call is down to 2 policy checks, which is good | |
| 20:27:51 | openstackgerrit | Merged openstack/nova master: _rollback_live_migration in live-migration seqdiag https://review.openstack.org/507871 | |
| 20:30:32 | openstackgerrit | melanie witt proposed openstack/nova master: Make setenv consistent for functional and api_sample_tests https://review.openstack.org/507976 | |
| 20:32:05 | mriedem | cdent: i know what's going on in my devstack patch | |
| 20:32:09 | mriedem | oh do i ever | |
| 20:32:18 | mriedem | i've been mtreinish'd i think | |
| 20:32:18 | cdent | oh? | |
| 20:32:58 | dansmith | mriedem: unfortunately a unit test failure snuck past me in that fix patch | |
| 20:33:14 | mriedem | where is my gd trombone | |
| 20:33:25 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix policy check performance in 2.47+ https://review.openstack.org/507948 | |
| 20:34:41 | mriedem | cdent: this case statement used to have a wildcard https://github.com/openstack-dev/devstack/blob/master/stackrc#L682 | |
| 20:34:42 | openstackgerrit | melanie witt proposed openstack/nova master: Set group_members when converting to legacy request spec https://review.openstack.org/507938 | |
| 20:36:46 | cdent | mriedem: whyreaka | |
| 20:36:49 | mriedem | GAH | |
| 20:36:50 | mriedem | f119121d21fa0446197b26378091677daac1606a | |
| 20:36:53 | mriedem | sdague'ed | |
| 20:37:27 | sdague | mriedem: ok, so what's the issue? | |
| 20:37:51 | mriedem | VIRT_DRIVER=fake means you don't get any default image downloaded | |