| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-26 | |||
| 13:46:02 | sean-k-mooney | artom: im mocking out get_enging in the migration | |
| 13:46:05 | sean-k-mooney | module | |
| 13:46:17 | sean-k-mooney | so it shoudl be useing the new fixtures engine | |
| 13:46:43 | artom | Right, I get that. I'm trying to understand why it's even necessary with the database fixture | |
| 13:47:08 | sean-k-mooney | because the engine.url is imutable | |
| 13:47:15 | sean-k-mooney | and its reused between multipel tests | |
| 13:47:26 | sean-k-mooney | so it end up with a different connection string then the one we pass | |
| 13:47:32 | artom | Right - but we can't use different engines? | |
| 13:47:40 | sean-k-mooney | even when we use the connection parmater in the db fixture | |
| 13:47:45 | artom | Per test, for tests that need it? | |
| 13:48:30 | sean-k-mooney | that is what https://review.opendev.org/c/openstack/nova/+/805663/6/nova/tests/unit/db/test_migration.py#50 is doing | |
| 13:49:15 | sean-k-mooney | artom: the engine url used to be mutable im not sure our db fixtuer has worked with custom connections since that change in sqlalcahme about a year or 2 agao | |
| 13:49:24 | artom | Yeah... I just thought the fixture would handle that | |
| 13:49:25 | artom | ? | |
| 13:49:53 | artom | Especially with the enticingly named connection=url kwarg :) | |
| 13:50:16 | sean-k-mooney | i think it used too but it definetly does not now | |
| 13:50:50 | artom | Weird. | |
| 13:50:59 | artom | And it's only used in 1 spot, the kwarg | |
| 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 | |