Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-26
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?
17:52:36 artom Nothing actually uses the engine that it creates with connection=url passed in

Earlier   Later