Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-26
14:21:58 lyarwood kk
14:22:01 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L162-L178
14:22:45 sean-k-mooney we do check os_info but its more complicated then that
14:23:38 lyarwood ah
14:24:05 sean-k-mooney this is where its then called in get_base_config https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L205
14:25:06 sean-k-mooney lyarwood: out of interest why were you looking for that
14:25:34 lyarwood sean-k-mooney: for the device bus migration stuff, the initial part is to register the current values
14:26:04 lyarwood sean-k-mooney: so I was trying to see if I can pull the default for this easily like I can with hw_disk_bus/hw_cdrom_bus from disk_info
14:26:14 sean-k-mooney ah right ya so you can jsut call that driver function to get the model
14:26:16 lyarwood sean-k-mooney: without inspecting the instance itself etc
14:26:37 lyarwood sean-k-mooney: any idea how I'd get an instance of that VIF class?
14:26:53 sean-k-mooney we construct it in the driver
14:26:57 lyarwood kk
14:27:22 lyarwood ah self.vif_driver, awesome
14:27:33 sean-k-mooney if you need too though you can just constuct it with no args
14:28:05 sean-k-mooney get_vif_model does not rely on self.host but ya self.vif_driver
14:28:39 sean-k-mooney this used to be plugable
14:28:57 sean-k-mooney i dont think it is anymore
14:59:51 melwitt gibi, sean-k-mooney: I support reverting the patch that changed from DELETE of allocations and I've been thinking about it more lately and think it's the better thing to do anyway
15:00:59 gibi melwitt, sean-k-mooney: I have to load context for that. I only saw the placement conflict error appearnig more frequently in the last ~24 hours.
15:01:23 melwitt gibi: yes, it's because of consumer types I'm afraid :(
15:03:05 gibi melwitt: is there a connection?
15:03:44 melwitt gibi: I think there is but I want to look through to make sure it makes sense
15:05:48 gibi OK, I can also spend some time on it tomorrow
15:06:53 melwitt lyarwood: ack will look again
15:07:23 melwitt sean-k-mooney: ack will look at the db url patch again
15:54:31 sean-k-mooney melwitt: i have one update to push for that which is the db fixture is not required
15:54:59 sean-k-mooney and im going to suqah the follow up patch in to adress stephens nits but the main content of hte patch wont change
15:55:37 sean-k-mooney melwitt: actully the ooo job has now completed so ill push those change in the next 5 mins or so
15:57:30 sean-k-mooney ok that looks like it fixes ooo https://review.opendev.org/c/openstack/tripleo-heat-templates/+/806103
16:07:34 opendevreview sean mooney proposed openstack/nova master: db: Handle parameters in DB strings https://review.opendev.org/c/openstack/nova/+/805663
16:08:50 sean-k-mooney artom: melwitt lyarwood ^ let me know if there is anything else needed im going to switch to something else for a bit
16:52:47 sean-k-mooney artom: thanks ill adress those comments later today
17:01:06 opendevreview Rodrigo Barbieri proposed openstack/nova stable/victoria: Fix 1vcpu error with multiqueue and vif_type=tap https://review.opendev.org/c/openstack/nova/+/806004
17:31:42 sean-k-mooney melwitt: actully in terms of gloabls i dont think we should be resetting them i relaly think we should be mocking them instead
17:31:57 sean-k-mooney melwitt: but i didnt really want to open that discussion in those patches
17:32:21 sean-k-mooney i dont think resetting them is safe from a concurancy point of view
17:32:29 melwitt ok
17:32:30 sean-k-mooney where asd the mock approch should be
17:33:20 sean-k-mooney i guess it might be ok since we will be runing the test in different python processes
17:33:37 melwitt yeah, that's why it's ok to do
17:33:47 melwitt *ok to do in our case
17:34:34 melwitt it's about (random) test ordering and whether leaving a particular state will impact a test running after
17:34:36 sean-k-mooney right form a style point of view im not sure direct asignment is preferable vs the mock but ill pull this out into a follow up patch if that works for you and artom
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?

Earlier   Later