Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-26
17:44:10 sean-k-mooney we have other local reset too
17:45:03 artom Otherwise it's whackamole, no?
17:45:22 sean-k-mooney well we would have to reset alto more state tehere then we curerntly do
17:45:34 sean-k-mooney normally we reset them in the test that tested the code tha tuse the global
17:45:50 melwitt I didn't come up with the approach but I have followed the pattern when I had patches that introduced new global vars
17:46:59 sean-k-mooney melwitt: i have always followed a differnt pattern like this https://github.com/openstack/nova/blob/50fdbc752a9ca9c31488140ef2997ed59d861a41/nova/tests/unit/test_cinder.py#L87 https://github.com/openstack/nova/blob/50fdbc752a9ca9c31488140ef2997ed59d861a41/nova/volume/cinder.py#L55
17:47:00 melwitt well, for things like a CELL_CACHE *lots* of things use it, even if they're not specifically setting out to use it
17:47:06 sean-k-mooney this is the first time i have seen it centralised
17:47:32 dansmith I dunno what patch ya'll are talking about,
17:47:34 sean-k-mooney i expect that modules have a reset funciton in them to rest the module sttate
17:47:51 dansmith but anything the test expects should be mocked, and only incidental globals i.e. CELL_CACHE should be reset in test.py each time, IMHO
17:48:20 sean-k-mooney dansmith: https://review.opendev.org/c/openstack/nova/+/805663
17:48:42 sean-k-mooney we cache the alembic config in a global
17:49:21 sean-k-mooney dansmith: so i was mocking the dict use for the cache so that we start form an empty dict and dont leak the state
17:49:35 dansmith yeah, that's exactly the sort of incidental global we _should_ reset in test.py I think
17:49:54 sean-k-mooney but we dont do that for a lot of other cases
17:49:55 dansmith presumably that gets touched by some tests that don't even realize it right?
17:49:58 artom sean-k-mooney, wait, shouldn't the clearing be happening *after* the test?
17:50:06 artom Or before the other tests run, in setUp()?
17:50:17 dansmith artom: in setUp() is conventional yeah
17:50:18 sean-k-mooney artom: no with the mock it happens before and it restores the previous state after
17:50:29 dansmith artom: i.e. don't clean up from other tests, reset state to known before running next test
17:50:30 artom On right, the mock context manager
17:50:45 artom But it'd still restore any state that was there previously, if another test leaked it...
17:51:09 sean-k-mooney yes
17:51:17 artom So yeah, I vote in setUp() in a follow up patch :)
17:51:37 artom Also, the Database fixture connection= kwarg is weird and needs to go, it's misleading
17:51:37 sean-k-mooney i can put it in setup but i dont think it should be in test.py
17:51:47 sean-k-mooney https://github.com/openstack/nova/search?q=reset_globals
17:52:16 sean-k-mooney i thikn that patern is cleaner the reset_gloals function is called in setup of the test class that test that module
17:52:17 dansmith sean-k-mooney: is it only ever set by this test, or can it be set by other tests that use the DB and don't really pay attention to the fact that it's set?
17:52:36 artom Nothing actually uses the engine that it creates with connection=url passed in
17:52:38 sean-k-mooney dansmith: its only set by things that run the db migrations
17:52:51 dansmith sean-k-mooney: well, everything runs the db migrations
17:53:01 sean-k-mooney dansmith: they dont use creat_all?
17:53:02 dansmith sean-k-mooney: do you mean only things that *test* the db migrations in specific ways?
17:53:06 sean-k-mooney that woul dbe much quicker
17:53:31 sean-k-mooney dansmith: i think its only set by thign that call migration.db_sync
17:53:43 dansmith sean-k-mooney: but that's a ton of things, right?
17:53:46 dansmith incidentally
17:54:05 dansmith things that want a DB setup have that done for them in the background right?
17:54:30 sean-k-mooney i dont know sound like i should add a function that will reset the gloabls and then either call that in test.py or in setup in the current test class
17:54:42 dansmith ++ for test.py for db things
17:54:57 dansmith DB stuff is so global and sticky IME that it belongs in the test.py, IMHO
17:55:03 sean-k-mooney dansmith: there are 2 ways to create the db. either start with blank schema or tell the model to create_all
17:55:18 sean-k-mooney ok ill do that so
17:55:36 sean-k-mooney still not conviced it the best way long term but ill do that for now
17:56:32 melwitt this is where we db_sync for every use of the Database fixture https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L643
17:56:36 dansmith I feel like melwitt and I get extra argument points for the scars we have from all the cellsv1 and v2 test setup experience
17:56:38 melwitt and the CellDatabases fixture https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L397
17:56:58 melwitt 😂
17:57:20 dansmith by "argument points" i mean.. votes :)
17:57:55 dansmith I didn't fully read the backscroll, but melwitt you're pro-test.py right?
17:57:55 sean-k-mooney dansmith: well its more i have previosue used a different pattern for reseting global state mainly for cached stuff in the past
17:58:23 sean-k-mooney dansmith: that was nto the db cofnig but its the first time i have seen a prefernce to do this in test.py centrally
17:58:31 melwitt dansmith: yes
17:58:32 sean-k-mooney rahter then in the test module that cares
17:59:21 sean-k-mooney ok ill add a reset_globals function to the nova.db.migration module and then call that form test.py
17:59:26 sean-k-mooney ill push that in a few minutes
#openstack-nova - 2021-08-27
01:31:15 opendevreview Qiu Fossen proposed openstack/nova master: Allow migrating PMEM's data https://review.opendev.org/c/openstack/nova/+/802225
01:32:55 opendevreview melanie witt proposed openstack/nova master: Revert "consumer gen: more tests for delete allocation cases" https://review.opendev.org/c/openstack/nova/+/806292
01:32:56 opendevreview melanie witt proposed openstack/nova master: Revert "Consumer gen support for delete instance allocations" https://review.opendev.org/c/openstack/nova/+/806293
01:36:51 melwitt gibi, sean-k-mooney, lyarwood: fyi I have uploaded reverts https://review.opendev.org/c/openstack/nova/+/806292 and https://review.opendev.org/c/openstack/nova/+/806293 as one possible option for addressing the gate bug. another option is the WIP patch https://review.opendev.org/c/openstack/nova/+/688802 to add a "force" kwarg to allocation delete
01:44:20 opendevreview Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294
06:42:53 slaweq gibi: hi
06:43:09 slaweq gibi: I see a lot of job's failures with error on deleting instance, like e.g. https://a91ddba9ffca8a8b5d72-1d82fafbdd07c6a56856d7b1449f73a8.ssl.cf5.rackcdn.com/800059/3/check/neutron-ovn-tempest-ovs-release/fd05a18/testr_results.html
06:43:20 slaweq and it's not only on neutron-tempest-plugin jobs
06:43:30 slaweq I saw it also on some devstack jobs too
06:43:41 slaweq gibi: is it known issue or should I open one?
07:04:18 gibi slaweq: hi
07:04:26 frickler slaweq: I would be hoping that these are solved by the above reverts https://review.opendev.org/c/openstack/nova/+/806293 etc.
07:04:33 gibi just couple lines above melwitt proposed patches
07:05:35 slaweq gibi: frickler thx a lot
07:05:56 gibi I've just started my day but I will prioritize this issue
07:06:35 slaweq gibi: thx, I just wanted to ask as I knew that You will know better if that's something already reported :)
07:06:40 gibi sure
07:07:13 frickler slaweq: if you check n-cpu.log for errors, you'll see tracebacks about conflicts in placement allocations
07:09:07 slaweq frickler: yes, now I see it :)
07:37:04 gibi melwitt: thanks for proposing patches I read up on the bug discussion and the patches and I prefer the force kwarg that gives us a more fine grained approach to handle deletes in different situations
07:37:28 gibi I will resolve the merge conflict in https://review.opendev.org/c/openstack/nova/+/688802 and then I will +2 it
07:37:34 gibi sean-k-mooney, lyarwood, stephenfin: ^^
08:19:55 lyarwood Morning
08:33:12 gibi lyarwood: morning
08:55:15 lyarwood gibi: sorry got distracted with $child things
08:55:35 lyarwood gibi: so how's the rebase going? Are we not going ahead with the reverts just yet?
08:55:48 gibi lyarwood: almost done
08:55:55 lyarwood ack
08:56:17 gibi lyarwood: for me the reverts are more complex than the force kwargs fix
08:56:26 lyarwood yup agreed
09:01:47 opendevreview Balazs Gibizer proposed openstack/nova master: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/c/openstack/nova/+/688802
09:02:18 gibi lyarwood, sean-k-mooney, stephenfin: ^^
09:15:30 lyarwood gibi: LGTM, had to do a little background reading to make sure but yeah still agree this is the better approach
09:19:10 gibi lyarwood: yepp the bug report has a good summary of what we are dealing with
09:28:06 opendevreview Fabian Wiesel proposed openstack/nova master: VmWare: Remove unused legacy_nodename regex https://review.opendev.org/c/openstack/nova/+/806336
09:46:50 gibi lyarwood: thanks for the review on the pps series. I replied in https://review.opendev.org/c/openstack/nova/+/793619/15#message-a2916b6a95c6f4061429e9fb2898ed738dfc3427
10:33:51 opendevreview FossenQiu proposed openstack/nova master: Allow migrating PMEM's data https://review.opendev.org/c/openstack/nova/+/802225
10:36:06 opendevreview Fabian Wiesel proposed openstack/nova master: Vmware: Fix spelling in test https://review.opendev.org/c/openstack/nova/+/806348
10:43:43 opendevreview Fabian Wiesel proposed openstack/nova master: VmWare: Use of id shadows built-in function https://review.opendev.org/c/openstack/nova/+/806390
10:45:41 lyarwood gibi: https://review.opendev.org/c/openstack/nova/+/688802 has some unit and functional failures if you have time to look
10:47:00 opendevreview Fabian Wiesel proposed openstack/nova master: Vmware: Fix indentation in conditionals https://review.opendev.org/c/openstack/nova/+/806391

Earlier   Later