| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-06-03 | |||
| 16:21:00 | gibi | sean-k-mooney: the original plan was that we are back on Wednesday morning, and then I would ask somebody to run it | |
| 16:21:15 | gibi | sean-k-mooney: but turned out that we have to get back on Tuesday morning | |
| 16:22:28 | gibi | so I think I will use Tuesday to do some chores and meeting fits well to that list :) | |
| 16:31:15 | melwitt | gibi: easy test-requirements update for placement, dunno if you also ran into this when running func tests the first time https://review.opendev.org/c/openstack/placement/+/787289 | |
| 16:35:58 | opendevreview | Balazs Gibizer proposed openstack/nova master: Remove unnecessary mocks from unit test https://review.opendev.org/c/openstack/nova/+/794637 | |
| 16:36:42 | gibi | melwitt: I don't remember I saw it but the change itself does not hurt | |
| 16:37:58 | gibi | could be that I have that package already locally | |
| 16:39:13 | gibi | yepp, I have it already that package installed globally in my dev machine | |
| 16:39:18 | gibi | that is why I did not see it | |
| 16:49:04 | opendevreview | Lee Yarwood proposed openstack/nova master: libvirt: Set driver_iommu when attaching virtio devices to SEV instance https://review.opendev.org/c/openstack/nova/+/794639 | |
| 16:51:45 | lyarwood | ^ should be WIP, I'll finish it up later with volume attach tests | |
| 16:51:52 | lyarwood | and maybe a seperate functional test | |
| 17:04:26 | sean-k-mooney | lyarwood: the set_driver_iommu_for_device should be called form vif_driver.get_config more then likely | |
| 17:06:02 | sean-k-mooney | lyarwood: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L560 | |
| 17:06:17 | lyarwood | sean-k-mooney: I'm assuming that wasn't done originally to ensure *all* devices end up with it set not just when disks and vifs we define | |
| 17:07:27 | sean-k-mooney | lyarwood: well curently you are just alwasy checkign it without considering the vif model correct | |
| 17:07:57 | lyarwood | sean-k-mooney: no that's checked in the designer | |
| 17:08:04 | lyarwood | sean-k-mooney: at least that it's virtio | |
| 17:08:16 | sean-k-mooney | ya just looked at that now | |
| 17:09:13 | sean-k-mooney | i dont see any reason not to put it in get config other then we will need to store the config and then return it | |
| 17:09:59 | sean-k-mooney | although you probably could do it here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L528 | |
| 17:11:07 | sean-k-mooney | we really need to get rid of the non os-vif code path | |
| 17:11:33 | lyarwood | sean-k-mooney: I'd rather do this explicitly in both hot plug flows first and then refactor the vif part later if you think it's worth it | |
| 17:12:03 | lyarwood | sean-k-mooney: we can't do this in the volume drivers as we don't pass in the instance so it would be weird to change this just for vifs IMHO | |
| 17:12:05 | sean-k-mooney | well i dont like spreading out the config genration like that | |
| 17:12:40 | sean-k-mooney | i dont think that the attach or detach function should have to care about this explictly | |
| 17:13:21 | sean-k-mooney | to me that is a poor speeration of concerns | |
| 17:15:15 | lyarwood | Yup I agree but the original SEV code was already doing this during spawn | |
| 17:15:40 | lyarwood | how about we fix this and then refactor both sets of config generation to do this correctly | |
| 17:16:19 | lyarwood | I really don't want to do that as part of the bugfix we need to backport | |
| 17:16:20 | sean-k-mooney | we could yes by passing the flavor/image metadata to _get_volume_config | |
| 17:16:33 | sean-k-mooney | and then moving this into vif_driver.get_config | |
| 17:16:44 | lyarwood | right I'm not going to do that in the backportable change | |
| 17:16:47 | lyarwood | someone else can | |
| 17:17:01 | sean-k-mooney | well honestly its not really a big change is it | |
| 17:17:04 | lyarwood | but to me that's something that can follow on master | |
| 17:18:25 | lyarwood | _get_volume_config isn't generating the actual config so if we were to do the same thing as vifs we'd be passing that down into the volume drivers right? | |
| 17:18:47 | lyarwood | otherwise there's no difference to what the change is already doing in attach_volume | |
| 17:19:51 | sean-k-mooney | there is a change presumable _get_volume_config is used for every operation that gets the confi for a volume | |
| 17:20:05 | sean-k-mooney | as vif_driver.get_config is | |
| 17:20:46 | sean-k-mooney | so if we put the if and call to the desinger in _get_volume_config and vif_driver.get_config | |
| 17:20:51 | sean-k-mooney | we can never forget to do it | |
| 17:21:13 | sean-k-mooney | what i dislike about your current patch is that it leave open the need to do this on all relevent code paths | |
| 17:21:58 | lyarwood | the original did that | |
| 17:22:29 | lyarwood | anyway let me respin and see what the fallout is | |
| 17:22:29 | sean-k-mooney | did what | |
| 17:22:32 | lyarwood | nvm | |
| 17:24:53 | sean-k-mooney | for get_volume config its just passing the info and doing the if here right https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L1887 | |
| 17:25:31 | lyarwood | right I thought you were suggesting changing the actual get_config methods in the volume drivers | |
| 17:25:56 | lyarwood | this is still going to cause test fallout that I'd rather avoid for backports but I'll give it a go | |
| 17:25:56 | sean-k-mooney | no | |
| 17:26:08 | sean-k-mooney | and for vif.py its just replaceing the returns here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L578-L601 | |
| 17:26:23 | sean-k-mooney | with an asignment to conf and dong the if then returning | |
| 17:26:55 | sean-k-mooney | lyarwood: the rest of the approch im ok with | |
| 17:27:49 | sean-k-mooney | lyarwood: due to the changing of the signiture of an privete funcion _get_volume_config | |
| 17:28:22 | sean-k-mooney | i would hope the fallout from tha twould be relitivly minimal | |
| 17:28:26 | lyarwood | sean-k-mooney: what about get_base_config for the vif? | |
| 17:28:40 | sean-k-mooney | you dont need to modify that | |
| 17:28:53 | sean-k-mooney | you could but you cna do it in get_config | |
| 17:29:29 | sean-k-mooney | lyarwood: that said https://github.com/openstack/nova/blob/cd084aeeb8a2110759912c1b529917a9d3aac555/nova/virt/libvirt/vif.py#L180 | |
| 17:29:47 | sean-k-mooney | it has the flaovr and image_meta | |
| 17:29:57 | sean-k-mooney | and that shoudl be called in all code paths | |
| 17:30:04 | lyarwood | right I was looking at the end of that method | |
| 17:30:07 | sean-k-mooney | so ya you can just add it there | |
| 17:30:30 | sean-k-mooney | lyarwood: to be honest i tought it was already there | |
| 17:31:03 | lyarwood | ah the only issue is checking if sev is enabled from here is going to be awkward | |
| 17:31:12 | sean-k-mooney | you need to do it here though https://github.com/openstack/nova/blob/cd084aeeb8a2110759912c1b529917a9d3aac555/nova/virt/libvirt/vif.py#L192 | |
| 17:31:37 | sean-k-mooney | lyarwood: becuase you dont have aceese to sev_enabled? | |
| 17:31:44 | lyarwood | _sev_enabled yeah | |
| 17:32:03 | sean-k-mooney | that is just https://github.com/openstack/nova/blob/cd084aeeb8a2110759912c1b529917a9d3aac555/nova/virt/libvirt/driver.py#L6716-L6743 | |
| 17:32:18 | sean-k-mooney | you can put that in desinger.py | |
| 17:32:28 | sean-k-mooney | if you wanted too | |
| 17:32:30 | lyarwood | right but that's checking _host | |
| 17:32:31 | sean-k-mooney | well no | |
| 17:32:38 | sean-k-mooney | it need _host | |
| 17:33:18 | sean-k-mooney | ok well we have _host in vif.yp | |
| 17:33:23 | lyarwood | lol | |
| 17:33:42 | sean-k-mooney | but if you want to do this in a follow up then ok | |
| 17:33:54 | sean-k-mooney | i think this would still be a clearer way to do this | |
| 17:34:30 | sean-k-mooney | we likely woudl just want to pass host as a parmater to sev_enabled | |
| 17:35:28 | sean-k-mooney | actully you could put the if check in set_driver_iommu_for_device | |
| 17:35:53 | sean-k-mooney | and pass host, flavor, image_meta and conf | |
| 17:36:47 | sean-k-mooney | anyway i guess leave it for now | |
| 17:37:07 | sean-k-mooney | at some point we really need to get ride of the desginer.py as well | |
| 17:37:32 | lyarwood | tbh this is the first time I've really looked at it | |
| 17:37:58 | sean-k-mooney | the designer.py has always been a pet pev of mine | |
| 17:38:04 | sean-k-mooney | i want driver.py to be smaller | |
| 17:38:12 | sean-k-mooney | but designer.py does not relaly add value today | |
| 17:38:13 | lyarwood | brb baby bedtime | |
| 17:38:47 | sean-k-mooney | it would be beter to merge it with config.py | |
| 17:39:57 | sean-k-mooney | it was seperated out so that config.py could just be the xml object creation and designer.py woudl have the logic to do some of the compostion | |
| 17:40:23 | sean-k-mooney | but we never too the xml logic out of vif.py or the volume modules | |
| 17:40:34 | sean-k-mooney | so really designer just complicated things | |
| 17:42:05 | opendevreview | Merged openstack/nova stable/wallaby: rbd: Get rbd_utils unit tests running again https://review.opendev.org/c/openstack/nova/+/790836 | |
| 18:04:59 | opendevreview | melanie witt proposed openstack/nova stable/victoria: rbd: Get rbd_utils unit tests running again https://review.opendev.org/c/openstack/nova/+/794624 | |
| 18:31:31 | melwitt | fix for intermittent unit test failure if anyone interested https://review.opendev.org/c/openstack/nova/+/794396 | |
| 18:53:01 | sean-k-mooney | melwitt: oh it was somethime slow an now did not match? | |
| 18:53:32 | sean-k-mooney | or something like that that change the behavior of _refresh_associations | |
| 18:53:37 | melwitt | sean-k-mooney: yeah, learned of it cause it hit my patch heh https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_b0d/794186/1/check/openstack-tox-py38/b0dd1dd/testr_results.html | |
| 18:53:58 | sean-k-mooney | that was going to be my next question good catch | |