| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-27 | |||
| 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? | |
| 20:39:09 | cdent | sounds right to me | |
| 20:39:09 | johnsom | stock, current, ubuntu 16.04 cloud image | |
| 20:39:21 | cdent | I’m dead, will check back in the morn | |
| 20:39:23 | sdague | mriedem: ok, because later everything assumes a real image? | |
| 20:40:07 | mriedem | sdague: well, tempest tries to get the image from glance to put in tempest.conf | |
| 20:40:11 | mriedem | the image id i mean | |
| 20:41:54 | mriedem | working a patch | |
| 20:43:34 | sdague | yeh, so, honestly we should probably figure out a better setup strategy for "throw away this image" | |
| 20:44:13 | sdague | mriedem: also, the qemu 2.10 thing wasn't a race, it's actually python2.7 and 3.5 evaluating the mock sentinel differently in the comparison | |
| 20:44:30 | sdague | for one of them it passes a >= check and the other it does not | |
| 20:45:53 | openstackgerrit | Sean Dague proposed openstack/nova master: Support qemu >= 2.10 https://review.openstack.org/505673 | |
| 20:47:44 | mriedem | oh | |
| 20:49:43 | mriedem | melwitt: pep8 | |
| 20:50:18 | mriedem | 507938 | |
| 20:50:20 | mriedem | oops | |
| 20:50:30 | mriedem | ./nova/tests/functional/regressions/test_bug_1719730.py:17:1: F401 'cast_as_call' imported but unused | |
| 20:50:37 | melwitt | sigh, of course | |