Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-26
17:35:11 sean-k-mooney melwitt: yep i have defiently seen the issues sharing state can cause
17:35:18 melwitt sean-k-mooney: I'm completely fine with waiting until it's a problem. it may be that we don't have any tests that would be impacted by leftover state
17:35:47 artom Don't fix problems we don't have :)
17:35:56 artom We have enough problems that we, err, have :P
17:36:23 sean-k-mooney artom: well the concern here is this is really really hard to debug when it happens
17:36:36 melwitt ++
17:37:22 artom Fair point, too
17:37:50 artom It's just weird seeing code that basically goes "weeeell just in case"
17:39:03 melwitt fwiw I agree the mocks in the patch should be removed, if we reset the globals it should be for all tests, not only the one
17:40:05 sean-k-mooney i partly agree with taht but not convifced how we do it today is correct :) ill remove it for now
17:40:31 melwitt re: resetting the globals in nova/test.py like the rest of them, I think that's the right thing to do (it's the correct thing to do) but it's not directly related to the patch at hand. that could and probably should be a separate patch
17:40:52 sean-k-mooney artom: just to be clear what melwitt was concerned about is https://github.com/openstack/nova/blob/e27a0135f8ec986d3583b11276715a5c0d0c302a/nova/db/migration.py#L75-L83 would retrun a cached copy of the config object and that would have state that would break us or cause inter test depency
17:41:27 sean-k-mooney which is technially a valid concern the connection stirg will proably leak if i remove the current mock
17:42:00 sean-k-mooney it should not break anything but ya it will leak state
17:42:16 artom Well, there's no proper way to fix that then, is there?
17:42:25 sean-k-mooney its what the mock does
17:42:32 melwitt it's just something I noticed and I have unfortunately broken the gate twice I think with forgetting to reset global vars I introduced, so I am particularly attuned to it 😂
17:42:33 sean-k-mooney it replace the dict its using as a cache
17:42:45 artom Because while we can definitely reset state *after* our tests, what happens for concurrently running tests?
17:42:58 sean-k-mooney artom: they are in differnt python processes
17:43:00 artom They're not running 1 per process, are they?
17:43:09 sean-k-mooney we only ever run one test in each proces at a time
17:43:26 melwitt artom: we currently do them in test.py. here's some examples and there are more if you read further down in setUp() https://github.com/openstack/nova/blob/2a78626a85954997d35f5fe62c50de297e2ca92d/nova/test.py#L227-L231
17:43:28 sean-k-mooney the test a loadbalnce over a set of process by tox
17:43:52 sean-k-mooney melwitt: well that is not the only place
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 :)

Earlier   Later