Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-16
14:01:53 sean-k-mooney and ye are seeing it only when the test class is run
14:02:00 sean-k-mooney rather then the test directly
14:02:21 stephenfin yeah, I'm seeing it intermittently but efried notes that it's consistent if you run without test discovery
14:02:26 efried which gels with the problematic result, which is expecting from GET /a_c {normal results} and is getting {normal results + fallback results}
14:02:31 stephenfin so 'tox -e py27 -- -n nova/tests/unit/scheduler/test_scheduler.py'
14:03:21 efried stephenfin, sean-k-mooney: fwiw I'm running it from within the venv as
14:03:21 efried stestr run -n nova.tests.unit.scheduler.test_scheduler.SchedulerManagerTestCase
14:03:27 efried hitting it every time
14:03:42 sean-k-mooney ya i can run the full file in the debugger too although im not sure if that will show anything specifically
14:03:55 efried ^^ is just the one test class
14:05:22 sean-k-mooney yep it failed for me too
14:05:52 mriedem i haven't been following but is there a new gate failure?
14:05:59 mriedem and if so, do we have a bug and e-r query tracking it?
14:05:59 sean-k-mooney no
14:06:18 sean-k-mooney its a intermitent failure of one of the tests in the PCPU series
14:10:01 efried stephenfin: So I've narrowed it down, by commenting out all the tests in the suite *except* test_select_destination_with_pcpu_fallback_disabled and test_select_destination_with_pcpu_fallback I still get the repro; commenting out either of those and I'm fine.
14:10:18 efried I also factored out the 4_3 thing so the former ^ only runs the one test
14:10:38 efried I also commented out the self.flags in the latter ^ because it's setting the default -- and still get the repro
14:10:53 efried so those two tests are banging on each other somehow.
14:11:59 sean-k-mooney why are we calling a test method directly by the way instead fo factoring out the common code into a helper method
14:12:54 efried sean-k-mooney: that was the first thing I mentioned. But it doesn't make a difference, cause it's also the first thing I tried :P
14:13:06 stephenfin sean-k-mooney: Because the theory was that the test end result and assertions should be identical under both circumstances
14:13:16 stephenfin ditto
14:13:44 sean-k-mooney yes but i was wonddering if we where sharing mocks by not doing it
14:14:15 stephenfin possibly. I tried reseting the 'select_destinations' mock to no avail
14:15:14 artom Why does discovery affect it, though? Discovery doesn't run any tests, does it?
14:15:24 stephenfin I think it's just ordering
14:15:27 efried no, it just makes the tests run much more quickly
14:15:28 efried yeah
14:15:57 artom Ah, so a timing issue
14:15:58 efried it seems as though discovery even slows down the running of the tests themselves
14:16:00 efried yes
14:16:14 sean-k-mooney well its not a timeing issue when we are leaking shared state
14:16:20 artom It causes thing to not run at the same time
14:16:26 sean-k-mooney no
14:16:34 sean-k-mooney but it may change the order
14:16:36 efried I would have thought it would do all discovery, come up with a list of tests, and then run 'em, and by the time you got to that last thing it was the same as if you did no discovery. But clearly that's not how it's happening.
14:16:42 efried yeah, or possibly ordering I guess
14:16:51 stephenfin efried: I've noticed that all the other mock assertions pass too
14:17:14 stephenfin so maybe it's not the global conf :-\
14:18:25 artom stephenfin, but... you said you've observed an option being False when it should be True...
14:18:28 efried stephenfin: waitwait
14:18:39 efried you're saying GET /a_c is called the appropriate number of times??
14:19:05 stephenfin That's what I'm seeing. Put the 'select_destinations.assert_called_once_with' to the end
14:19:13 stephenfin in select_destinations.assert_called_once_with
14:19:19 stephenfin sorry, test_select_destination_with_4_3_client
14:19:51 sean-k-mooney i have it open in the debuger now so ill check
14:19:52 stephenfin assert_called_once_with checks that things are called exactly once, right/
14:20:05 sean-k-mooney yes its called once
14:21:24 sean-k-mooney stephenfin: it check its called exactly once and has the correct args
14:21:28 sean-k-mooney so yes
14:21:53 efried I put a call_count assertion just in case
14:23:56 stephenfin moving the mocks inline and using context managers instead of function decorators doesn't help
14:24:47 sean-k-mooney if i put the select_destinations.assert_called_once_with( call at the end all the rest pass
14:27:43 sean-k-mooney its failing in assert_called_with when its comparing the args
14:32:46 stephenfin efried, sean-k-mooney: got it
14:32:50 efried tell
14:33:23 stephenfin https://review.opendev.org/#/c/671801/50/nova/scheduler/manager.py@191
14:33:33 stephenfin we're using extend, which modifies a list in place
14:33:52 stephenfin and our mock is returning 'fakes.ALLOC_REQS'
14:34:15 efried oy vay
14:34:17 stephenfin so that's getting modified by the non-disabled fallback test
14:34:40 efried stephenfin: I think I saw those globals only used by this one test suite?
14:34:53 stephenfin correct
14:35:11 efried so make 'em instance vars, for future safety
14:35:44 stephenfin wdym?
14:35:57 stephenfin I was doing to do 'fakes.ALLOC_REQS[:]'
14:36:10 stephenfin though I could add a 'get_fake_alloc_reqs' helper too
14:36:11 efried I mean, you could deepcopy 'em to fix this problem, but it's just going to bite us in the ass again later, somewhere else.
14:36:26 efried global test artifacts bad
14:36:32 stephenfin v bad.
14:36:43 efried first 205 lines of fakes, bad.
14:37:59 efried stephenfin: I guess for now you could just make it a helper method that returns a fresh new copy every time
14:38:08 efried but if it were me, I would make it completely fresh
14:38:27 efried def get_fake_alloc_reqs():
14:38:27 efried return { $everything }
14:38:55 efried rather than
14:38:55 efried EVIL_GLOBAL
14:38:55 efried def get_fake_alloc_reqs():
14:38:55 efried return some_unreliable_copy_method(EVIL_GLOBAL)
14:39:05 stephenfin gotcha
14:39:07 stephenfin coming right up
14:39:17 efried this is gonna be partway down the series, yah?
14:39:29 stephenfin yeah, just before that patch. I'll do it separately
14:39:57 efried stephenfin: you're going to have to fix the patch anyway, so might as well do it in place, nah?
14:40:09 efried don't try to fix the race afterward
14:40:39 stephenfin I meant add the helper function in a precursor patch and modify the intermittently failing patch to use it
14:40:42 efried tbc, the patch in question is "Add support for translating CPU policy extra specs, image meta"
14:40:42 stephenfin but I can combine too
14:41:03 efried yeah, just combine, help me justify a fast approve
14:41:08 stephenfin ack
14:47:38 openstackgerrit Stephen Finucane proposed openstack/nova master: Add support for translating CPU policy extra specs, image meta https://review.opendev.org/671801
14:47:39 openstackgerrit Stephen Finucane proposed openstack/nova master: fakelibvirt: Make 'Connection.getHostname' unique https://review.opendev.org/681060
14:47:39 openstackgerrit Stephen Finucane proposed openstack/nova master: libvirt: Mock 'libvirt_utils.file_open' properly https://review.opendev.org/681061
14:47:40 openstackgerrit Stephen Finucane proposed openstack/nova master: Add reshaper for PCPU https://review.opendev.org/674895
14:47:46 stephenfin efried: ^
14:48:06 stephenfin I'll remove the rest of those global fakes now (separate patch)
14:48:24 efried ++
14:48:47 openstackgerrit Matt Riedemann proposed openstack/nova master: Centralize volume create code during boot from volume https://review.opendev.org/682378
14:48:51 mriedem ^ is a simple refactor split off from https://review.opendev.org/#/c/541420/ which has been around since february of 2018,

Earlier   Later