Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-09-27
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
20:38:02 mriedem so i assume you don't want the wildcard back
20:38:07 johnsom FYI, after collecting a mountain of logs on this missing network interface issue I decided to force a PCI bus rescan inside the instance, boom, the interface appears as it should have. So, ubuntu/kernel/something issue and not nova/neutron
20:38:09 mriedem so i'll just add a case for fake and make it the same as libvirt?

Earlier   Later