| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-26 | |||
| 13:51:25 | sean-k-mooney | ok so i dont need the db fixture just my one for 'nova.db.migration._get_engine' | |
| 13:51:48 | sean-k-mooney | the test will still work if i remove the db fixture | |
| 13:52:21 | sean-k-mooney | good catch ill update that when i squah things | |
| 13:53:16 | sean-k-mooney | ooo is still runnign so i dont want to interupt that job | |
| 13:56:27 | sean-k-mooney | artom: you mentioned its used in only one place? | |
| 13:56:34 | sean-k-mooney | where is that | |
| 13:56:49 | sean-k-mooney | maybe i should remove the parmater in a follow up and fix that | |
| 13:56:50 | opendevreview | Jorhson Deng proposed openstack/nova master: recheck the attachment_id after the reschedule successful https://review.opendev.org/c/openstack/nova/+/796209 | |
| 13:57:35 | sean-k-mooney | or well make the connnection parmater work by creating an new engin instnace | |
| 14:14:10 | artom | sean-k-mooney, well, it appear to correctly create a new engine | |
| 14:14:20 | artom | .. which then never gets used anywhere, AFAICT | |
| 14:15:01 | artom | nova/tests/functional/db/test_connection_switch.py is where it gets used | |
| 14:15:05 | artom | self.useFixture(nova_fixtures.Database(connection=self.fake_conn)) | |
| 14:18:08 | lyarwood | stupid question if anyone with vif/neutron background can help, I'm trying to work out how to find a default for hw_vif_model when it's unset | |
| 14:18:27 | lyarwood | I thought osinfo.HardwareProperties(instance.image_meta).network_model was what I wanted but I'm seeing it return None | |
| 14:18:48 | sean-k-mooney | lyarwood: its virtio | |
| 14:18:56 | lyarwood | just hardcoded? | |
| 14:19:06 | sean-k-mooney | i think so i think i know where it is | |
| 14:19:17 | lyarwood | kk I've missed it then | |
| 14:19:33 | sean-k-mooney | its virt dirver dependent | |
| 14:19:50 | sean-k-mooney | vmware default to a differnt thing then libvirt | |
| 14:20:17 | opendevreview | Stephan Pampel proposed openstack/nova master: docs: admin/networking rename neutron_tunneled to neutron_tunnel https://review.opendev.org/c/openstack/nova/+/806197 | |
| 14:20:34 | lyarwood | https://github.com/openstack/nova/blob/master/nova/virt/osinfo.py#L99-L112 was where I was looking FWIW | |
| 14:20:55 | lyarwood | just without os_distro set this doesn't seem to do anything | |
| 14:21:11 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L211-L215 | |
| 14:21:32 | sean-k-mooney | sorry wrong link | |
| 14:21:42 | sean-k-mooney | but its not commig form osinfo its form vif.py | |
| 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 | |