| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-27 | |||
| 19:03:13 | mriedem | ^ should help a bit with doc discovery | |
| 19:03:51 | Tengu | mriedem: \o/ thanks ! | |
| 19:04:07 | Tengu | for now I'm digging in glance, as apparently it's crashed. | |
| 19:07:42 | dansmith | sdague: mriedem: cfriesen_: https://imgur.com/a/IQ0Vh | |
| 19:07:58 | mriedem | dansmith: awesome | |
| 19:08:03 | mriedem | also, | |
| 19:08:30 | mriedem | i ran the 2.53 microversion, GET /servers/detail thing again w/o your patch, to see why i had just a big difference in numbers, and you're right, it's the vm | |
| 19:08:43 | mriedem | so w/o your patch, it's still closer to with your patch, | |
| 19:08:51 | mriedem | and over half of what it was the other day on the other vm | |
| 19:08:58 | mriedem | so just need to chalk that up to public cloud | |
| 19:09:09 | cdent | what happened at 2.47? | |
| 19:09:21 | dansmith | mriedem: cool | |
| 19:09:21 | mriedem | cdent: we started checking policy per instance when listing instances | |
| 19:09:29 | mriedem | which adds up when you're listing 1000 instances | |
| 19:09:30 | cdent | ouch | |
| 19:10:34 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix policy check performance in 2.47+ https://review.openstack.org/507948 | |
| 19:11:48 | cfriesen_ | cdent: my bad, I didn't realize policy check was expensive | |
| 19:12:24 | mriedem | i'm sure i approved the change so don't worry about it | |
| 19:12:28 | cdent | cfriesen_: a reasonable thing to assume in a reasonable universe, but we probably left that one long ago | |
| 19:12:32 | sdague | I kind of wonder if there are other places with embedded policy checks like that are expensive | |
| 19:13:17 | sdague | cfriesen_: there is an implicit fstat because policy is live reread | |
| 19:13:53 | cdent | speaking of, that’s a potential next microoptimization in the unit tests. that file gets read over and over and over over and over and over and ... | |
| 19:14:44 | sdague | honestly, it might behoove us to change that behavior entirely, as we've got the hup handler now | |
| 19:15:16 | bauzas | dansmith: you trampled me | |
| 19:15:27 | dansmith | bauzas: I did? | |
| 19:15:35 | bauzas | dansmith: with Twitter | |
| 19:15:47 | bauzas | :p | |
| 19:16:09 | bauzas | so, maybe you should be the next US president given you use Twitter for trampling folks :p | |
| 19:16:23 | bauzas | mmm, maybe "trample" is not the right verb | |
| 19:16:29 | dansmith | I'm not sure what trampling I did, but I definitely need not be president | |
| 19:16:41 | penick | too late i'm writing you in | |
| 19:16:58 | bauzas | I mean, I chilled :p | |
| 19:17:03 | mriedem | you made sylvain spit out his coffee | |
| 19:17:08 | mriedem | you "floored" him | |
| 19:17:35 | bauzas | when I saw the tweet for 2.47 :p | |
| 19:17:45 | bauzas | sorry for "trampling" | |
| 19:18:19 | dansmith | bauzas: okay I replied to you about two seconds before you pinged me here so I thought you meant my reply was rude in some way | |
| 19:18:33 | bauzas | emacron: maybe you should ask French folks to stop using French but rather English ? | |
| 19:19:05 | bauzas | dansmith: sorry, the verb wasn't good :) | |
| 19:19:11 | dansmith | ack | |
| 19:19:33 | bauzas | "chilling" is better | |
| 19:20:36 | bauzas | dansmith: anyway, thanks for your tweet | |
| 19:25:58 | cfriesen_ | dansmith: reviewing your patch. I assume the version check is a performance optimization to avoid the policy check if we can? | |
| 19:28:02 | mriedem | cfriesen_: it's because we only ever care about showing flavor extra specs if you're requesting 2.47 or above | |
| 19:28:09 | mriedem | so don't even make the policy check otherwise | |
| 19:28:14 | dansmith | cfriesen_: yeah | |
| 19:28:39 | sdague | dansmith: so one thing to consider on that test, there is nothing in that test asserting the server list is > 1 right now | |
| 19:28:50 | sdague | because it's all common setup | |
| 19:28:54 | dansmith | sdague: true, but I did check that its 4 | |
| 19:29:02 | dansmith | I can add another | |
| 19:29:03 | sdague | I thought it was 5 | |
| 19:29:08 | sdague | I was just running it | |
| 19:29:11 | dansmith | it was 4 | |
| 19:29:40 | mriedem | so just self.assertGreater(len(instances), 1) ? | |
| 19:29:54 | dansmith | oh no, | |
| 19:29:58 | dansmith | top index was 4 | |
| 19:29:59 | dansmith | 0-4 | |
| 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 | |