Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-16
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 return { $everything }
14:38:27 efried def get_fake_alloc_reqs():
14:38:55 efried return some_unreliable_copy_method(EVIL_GLOBAL)
14:38:55 efried def get_fake_alloc_reqs():
14:38:55 efried EVIL_GLOBAL
14:38:55 efried rather than
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 stephenfin but I can combine too
14:40:42 efried tbc, the patch in question is "Add support for translating CPU policy extra specs, image meta"
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: libvirt: Mock 'libvirt_utils.file_open' properly https://review.opendev.org/681061
14:47:39 openstackgerrit Stephen Finucane proposed openstack/nova master: fakelibvirt: Make 'Connection.getHostname' unique https://review.opendev.org/681060
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,
14:49:06 mriedem and is important if y'all ever want to drop the legacy volume attachment compat code
14:50:57 efried stephenfin: +A, and re+W up the pile. Nice work, thank you.
14:59:24 stephenfin mriedem: done
15:00:13 efried mriedem: one adjustment requested pls
15:00:17 mriedem doing it
15:00:28 efried alex_xu: yt?
15:06:32 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Create volume attachment during boot from volume in compute https://review.opendev.org/541420
15:06:32 openstackgerrit Matt Riedemann proposed openstack/nova master: Centralize volume create code during boot from volume https://review.opendev.org/682378
15:09:30 efried mriedem: are we merging stuff like that ^ at this point or waiting for ussuri to fork?
15:09:51 mriedem which one? the refactor is trivial and i've added the latter to https://etherpad.openstack.org/p/nova-train-release-todo
15:10:06 mriedem as i said, it's been around forever without much core review outside melwitt
15:10:12 mriedem the mox->mock stuff in the tests blew it all up
15:10:29 mriedem but if we ever want to migrate off the legacy volume attach code, we need to be creating all volumes with the new style attachment stuff
15:10:56 mriedem iow, the longer we wait, the bigger the data migration is going to be
15:11:10 mriedem e.g. https://review.opendev.org/#/c/549130/
15:11:40 mriedem i don't expect to get ^ into train at this point
15:11:46 mriedem nor is it probably the only way to skin that cat
15:12:29 mriedem at some point in the future we can add a nova-status upgrade check and fail to start if you haven't migrated old bdm records

Earlier   Later