| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-06 | |||
| 14:16:31 | cdent | s/over/confused/ | |
| 14:16:42 | mdbooth | However, I'd defer entirely to sdague :) | |
| 14:16:56 | sdague | cdent: what is the eventlet concern when tests are run? | |
| 14:17:10 | cdent | So I've been assuming it doesn't (as that would be bad as it introduces ambiguity). | |
| 14:17:43 | mdbooth | Incidentally, I'm just starting to look at this myself. | |
| 14:17:44 | cdent | sdague: If eventlet is present and monkey patching file io in some sense, you might get an unexpected yield to an unexpected greenthread? | |
| 14:18:11 | cdent | I don't really know, I'm experiening curiosity. Expressing it to see if anyone else knows what's up. | |
| 14:18:20 | sdague | cdent: eventlet is definitely present | |
| 14:20:06 | sdague | but, there are going to need to be spawns to have greenlets running, right? We use that to our advantage on the functional tests with the single process cloud | |
| 14:21:29 | cdent | That's effectively my question: If your first statement/question is true, not an issue. | |
| 14:22:20 | sdague | the parallel running of tests is via testr spawning dedicated processes with subsets of the tests, and running subunit serially in each | |
| 14:23:03 | cdent | yeah, that part I understand (and know too well, dealing with trying to get gabbi to behave properly in that scenario (it does)) | |
| 14:23:07 | sdague | so... as long as tests clean up after themselves, there shouldn't be interference between tests. But cleaning up after themselves is sometimes not super clear, especially if globals are set | |
| 14:38:18 | openstackgerrit | John Garbutt proposed openstack/nova: correct misleading wording https://review.openstack.org/413658 | |
| 14:38:44 | openstackgerrit | John Garbutt proposed openstack/nova: correct misleading wording https://review.openstack.org/413658 | |
| 14:49:54 | openstack | Launchpad bug 1654393 in OpenStack Compute (nova) ""keystoneauth1.exceptions.catalog.EmptyCatalog: The service catalog is empty." in test_cinderclient_unsupported_v1" [High,In progress] - Assigned to Matt Riedemann (mriedem) | |
| 14:49:54 | mdbooth | cdent: https://bugs.launchpad.net/nova/+bug/1654393/comments/2 | |
| 14:50:30 | cdent | go you. mdbooth++ | |
| 14:51:37 | johnthetubaguy | was just looking at matt's patch he has up for that | |
| 14:54:01 | mdbooth | johnthetubaguy: Yeah, I wasn't convinced. | |
| 14:55:02 | mdbooth | So, it looks like it fails because _SESSION already exists | |
| 14:55:48 | johnthetubaguy | yeah, someone already set it up somehow | |
| 14:56:03 | mdbooth | johnthetubaguy: See my comment | |
| 14:56:21 | mdbooth | Looks like it's pretty easy to initialise | |
| 14:56:33 | johnthetubaguy | so if _SESSION is set, the old mock would never get run | |
| 14:56:39 | johnthetubaguy | but thats not true after his patch | |
| 14:56:41 | johnthetubaguy | I think? | |
| 14:58:17 | johnthetubaguy | hang on, I am missreading this | |
| 14:58:36 | mdbooth | johnthetubaguy: I'm going to submit an alt patch which does cinder.reset_globals() instead | |
| 14:58:51 | johnthetubaguy | mdbooth: ah, gotcha | |
| 14:59:06 | mdbooth | Because that's the real issue here. It's not that mock is stamping on things, it's that something previously initialised global state. | |
| 14:59:30 | johnthetubaguy | yeah, its global state already being tweaked | |
| 14:59:52 | mdbooth | Ah... and BaseCinderTestCase also does that | |
| 15:00:33 | openstackgerrit | Stephen Finucane proposed openstack/nova: console: Move proxies to 'console/proxy' https://review.openstack.org/408192 | |
| 15:00:34 | openstackgerrit | Stephen Finucane proposed openstack/nova: tests: Replace use of CONF with monkey patching https://review.openstack.org/408191 | |
| 15:00:34 | openstackgerrit | Stephen Finucane proposed openstack/nova: conf: Deprecate 'console_driver' https://review.openstack.org/408193 | |
| 15:04:14 | cdent | Is it fair to ask "Why do we have global state?"? | |
| 15:04:45 | mriedem | i'm going to be sending off the user survey feedback question today so it's last call for questions | |
| 15:05:46 | johnthetubaguy | cdent: its a cache I believe | |
| 15:06:06 | openstackgerrit | Matthew Booth proposed openstack/nova: Reset global state in CinderV1TestCase https://review.openstack.org/417408 | |
| 15:06:21 | johnthetubaguy | cdent: still a valid question | |
| 15:06:22 | cdent | mriedem: "Do you wish there was an orchestration layer?" | |
| 15:06:23 | mriedem | mdbooth: i've already had a patch up for that since yesterday | |
| 15:06:48 | mdbooth | mriedem: Yeah, I read it earlier. | |
| 15:07:02 | mriedem | mdbooth: my first attempt at it failed, but the one that you -1ed has passed jenkins several times already | |
| 15:07:52 | mdbooth | mriedem: See ^^^. I'm doing reset_globals(). Your patch mocks _SESSION. It has the same effect, but reset_globals() is how the other tests do it. | |
| 15:08:04 | mdbooth | And it's more explicit as to why it's being done. | |
| 15:08:11 | mriedem | mdbooth: the reset_globals() thing seems like a hack to begin with | |
| 15:08:31 | mriedem | mdbooth: we shouldn't have to deal with a global between concurrently running tests | |
| 15:08:34 | mdbooth | You need a hack like this everywhere you have global state. | |
| 15:08:37 | mriedem | which is why i'm mocking out _SESSION altogether | |
| 15:08:45 | mdbooth | We don't have concurrently running tests. | |
| 15:09:02 | mriedem | sure we do, between test classes at least | |
| 15:09:10 | mdbooth | That's why I was -1 on your patch, because I think it obscured the issue. | |
| 15:09:34 | mdbooth | I don't think so. They run serially in multiple processes. | |
| 15:09:46 | mdbooth | The issue here wasn't a race. It was that something else previously initialised the global state. | |
| 15:10:00 | mriedem | mdbooth: then how is it that it was never reproduced locally? | |
| 15:10:04 | mdbooth | It is. | |
| 15:10:05 | mriedem | if it was dirty state? | |
| 15:10:12 | mriedem | i never hit it locally | |
| 15:10:17 | mdbooth | See my comment 2 on the bug. | |
| 15:10:38 | mdbooth | Running those 2 tests in order reproduces the bug deterministically. | |
| 15:11:03 | sdague | mriedem: test grouping and order is an awkward level of deterministic. | |
| 15:11:38 | sdague | so probably your .testrepository past results are impacting test run order for you and you just aren't getting the bad sequence | |
| 15:12:16 | mriedem | so why not just get rid of the need for reset_globals() altogether? | |
| 15:12:22 | mriedem | if we have to rely on that it seems we've failed | |
| 15:12:24 | sdague | mtreinish: do you know if there is a way for us to turn off the order optimizer? | |
| 15:13:08 | mriedem | mtreinish: isn't awake yet :) | |
| 15:13:26 | mdbooth | mriedem: To do that you'd need to remove the global state, i.e. not have a global _SESSION | |
| 15:13:56 | mdbooth | I think perhaps we could have a central repository for resetting global state, though. | |
| 15:14:04 | mdbooth | We currently do it piecemeal all over the place. | |
| 15:14:37 | mdbooth | If every time we added global state we also added it to a super-global reset_globals()? | |
| 15:16:33 | sdague | yeh, that would be less prone to errors. A general _reset_globals cleanup in the base test case | |
| 15:16:38 | mriedem | i guess i don't care at this point, i don't see a problem with how i've fixed this, but don't really want to argue (2 sips into coffee), so i'll let others on the core team decide which should go in | |
| 15:17:45 | sdague | so I'm inclined at this point to take mriedem's approach, because that good practice for that test in question - https://review.openstack.org/#/c/417206 | |
| 15:18:31 | sdague | but I also think a global cleanup on test.py (especially if it came with a warning about what globals it found that it was reseting so you could figure out what test cases weren't cleaning up) would be good | |
| 15:18:53 | mdbooth | WFM. Is it worth fixing up the commit message to not refer to races? That's what confused me, and is why I kept looking. | |
| 15:19:32 | cdent | Unfortunately we have a tendency to overload "race" to mean anything that is non-deterministic in practice. | |
| 15:19:43 | mtreinish | mriedem: I am awake | |
| 15:19:59 | mriedem | :) | |
| 15:20:13 | mtreinish | sdague: you mean the scheduler's optimization based on previous times | |
| 15:20:20 | sdague | mtreinish: yeh | |
| 15:20:32 | mtreinish | sdague: the easiest way is just to nuke times.dbm | |
| 15:20:41 | mtreinish | I don't think there is a flag in testr run to disable it | |
| 15:20:45 | sdague | mtreinish: right, but I want to turn it off in tox | |
| 15:20:54 | mriedem | fwiw i deleted all of .tox and .testrepository within the last week | |
| 15:21:21 | sdague | mriedem: sure, but anything there impacts the run order | |
| 15:21:27 | mtreinish | sdague: add rm .testrepository/times.dbm to the command in the tox.ini? | |
| 15:21:48 | mtreinish | I can push a patch up to testr to skip it, but it'll just go in my pile of open PRs | |
| 15:22:20 | mdbooth | sdague: It's easy enough to pull the test ordering out of the subunit output | |
| 15:22:26 | mdbooth | Which is what I did | |
| 15:22:50 | mdbooth | https://wiki.openstack.org/wiki/Testr#Reproducing_Failures | |
| 15:22:50 | sdague | mdbooth: yeh, it's a little less fun to replicate that run order | |
| 15:23:10 | sdague | oh, there it is | |
| 15:24:08 | mriedem | johnthetubaguy: dansmith: can you guys get this in unless there are any issues? https://review.openstack.org/#/c/415511/ - jay's API code is ready for +2 and i got the novaclient change up for it last night so would like to move forward | |
| 15:24:32 | openstackgerrit | Samantha Blanco proposed openstack/nova: Add missing CLI commands in support-matrix.ini https://review.openstack.org/417232 | |
| 15:24:48 | johnthetubaguy | mriedem: looking now ish | |
| 15:25:03 | mriedem | thanks | |
| 15:25:14 | mriedem | johnthetubaguy: all i added in the latest PS was the response example | |
| 15:33:48 | openstackgerrit | Ed Leafe proposed openstack/nova: Trivial indentation fix https://review.openstack.org/417422 | |