Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-16
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,
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: Centralize volume create code during boot from volume https://review.opendev.org/682378
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: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
15:12:50 mriedem and then drop all that compat code
15:15:44 mriedem melwitt: do you want to send this through? https://review.opendev.org/#/c/677736/
15:16:19 melwitt I do. thanks
15:26:06 openstackgerrit Balazs Gibizer proposed openstack/nova master: Follow up for the bandwidth series https://review.opendev.org/682389
15:26:29 gibi mriedem, efried: two smallish nits for the bandwidth series. I have two other testing enhancements on my TODO list that I plan to propose this week. None of these is critical, just nice to have.
15:27:17 mriedem gibi: i got the fup
15:27:37 gibi mriedem: thanks
15:27:55 gibi mriedem: and thanks for the review on the bug fix https://review.opendev.org/#/c/666857/ I will respin that
15:36:03 bauzas gibi: I can help you :)
15:56:29 gibi bauzas: thanks
17:26:10 openstackgerrit Eric Fried proposed openstack/nova master: objects: use all_things_equal from objects.base https://review.opendev.org/681397
17:39:31 openstackgerrit Merged openstack/nova master: Deprecate the XenAPIDriver https://review.opendev.org/680732
18:20:09 openstackgerrit Merged openstack/nova master: libvirt: Fix service-wide pauses caused by un-proxied libvirt calls https://review.opendev.org/677736
18:25:33 sean-k-mooney cool that ^ will make mdbooth happy
18:27:04 sean-k-mooney by the way can i get a second +2+w on https://review.opendev.org/#/c/670585/
18:27:39 sean-k-mooney i really want to ensure that is deprecated in train so we can remove it in Ussuri

Earlier   Later