| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-06-03 | |||
| 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 | |
| 18:56:37 | sean-k-mooney | oh i guess its later in the test | |
| 18:56:53 | sean-k-mooney | ya it is | |
| 18:57:02 | melwitt | no yeah it's used | |
| 18:57:46 | sean-k-mooney | part of me wishes there was a clearn helper funciton or somthing for this but ya this looks correct to me | |
| 18:58:19 | sean-k-mooney | for example mocking time.time via a decorator at the top of the function | |
| 18:59:47 | melwitt | oh yeah, I guess that would work without needing reset of the mocks throughout, cause all you'd change is the return_value and not asserting calls | |
| 18:59:48 | sean-k-mooney | e.g. @mock.patch.object(time, 'time', return_value=time.time()) | |
| 19:00:08 | sean-k-mooney | yep | |
| 19:00:27 | sean-k-mooney | you could still reset the time mock if needed | |
| 19:00:45 | sean-k-mooney | but they dont check that as far as i can see | |
| 19:01:04 | melwitt | yeah. I was hoping to not though | |