| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-16 | |||
| 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 | |
| 18:27:52 | sean-k-mooney | as in like 2 weeks if not sooner | |
| 18:29:15 | sean-k-mooney | dansmith: mriedem: melwitt: could one of ye take a look when ye have time ^ | |
| 20:03:02 | mriedem | sean-k-mooney: commented, | |