Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-06-03
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
18:55:03 melwitt after awhile I realized it's because refresh associations records the time at which it ran, and the test was relying on the fact that that time would be sufficiently soon enough after "now" was recorded prior
18:56:28 sean-k-mooney that looks like now = time.time() was previously unused

Earlier   Later