| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-07-15 | |||
| 11:36:38 | gibi | probably made a two broad mock | |
| 11:36:52 | artom | If you just start the server with a macvtap and mock the PCI code to return NotFound, you'll still get the exact same service startup error | |
| 11:36:58 | gibi | yeah | |
| 11:37:30 | artom | And like, I know in real life the NotFound comes from changing the vnic_type | |
| 11:37:52 | artom | OK, I think I got it. | |
| 11:38:33 | artom | No, wait. | |
| 11:41:06 | artom | The thing that Nova is doing wrong is attempting to find the wrong PCI device? Or because the device is already used by the instance, we can't find it? | |
| 11:42:27 | gibi | it is used by the instance so we cannot find it afaik | |
| 11:42:58 | gibi | or more precisely | |
| 11:43:02 | gibi | /sys/bus/pci/devices/0000:19:0a.7/net is not exists | |
| 11:43:22 | gibi | as the pci device is attached to the instance there is no netdev on the host | |
| 11:45:42 | gibi | after the vnic_type changed to macvtap the vif plug code wants to call set_vf_interface_vlan on the pci device | |
| 11:45:55 | artom | So wait, shouldn't that also happen if you just restart nova-compute with any macvtap device? | |
| 11:46:23 | gibi | hm | |
| 11:46:30 | gibi | that is a good question | |
| 11:46:33 | gibi | wait | |
| 11:46:38 | gibi | if it is consumed as a macvtap | |
| 11:46:44 | gibi | then the VF is not consumed | |
| 11:46:49 | gibi | and probably the netdev exists | |
| 11:47:02 | artom | Ah, right. | |
| 11:47:06 | artom | Sorry, creating confusion. | |
| 11:47:07 | gibi | the problem is that we consumed the VF and then looking up the VF's netdev | |
| 11:47:27 | artom | Bingo. | |
| 11:47:35 | artom | So the functional test should probably be asserting *that* | |
| 11:49:42 | gibi | so probably mocking get_ifname_by_pci_address is too much and I should inject the fault at set_vf_interface_vlan as that is the last specific call | |
| 11:49:55 | gibi | for the macvtap plug | |
| 11:49:56 | artom | Also, this is in the plug logic, right? | |
| 11:50:03 | artom | So should also happen during instance hard reboot? | |
| 11:50:18 | artom | We don't do any PCI accounting update during reboot... | |
| 11:50:22 | gibi | yes | |
| 11:50:30 | gibi | probably the plug fails at hard reboot too | |
| 11:50:45 | gibi | I can try as I have real sriov hw right now | |
| 11:52:25 | gibi | still the test will not be perfect as with mocking I can only simulate that the VF is consumed | |
| 11:52:40 | gibi | so if you move the port set before boot but keep the mock as is then it will still fail | |
| 11:52:57 | gibi | as it will still simulate that the VF is cosumed | |
| 11:53:04 | gibi | even if in reality it would not be | |
| 11:53:48 | artom | Right, there's no way around the mocking | |
| 11:54:00 | artom | But... I think I'd an asserting what what device we tried to look for | |
| 11:54:04 | artom | *assertion | |
| 11:54:14 | artom | "<gibi> the problem is that we consumed the VF and then looking up the VF's netdev" | |
| 11:54:15 | artom | :) | |
| 11:55:13 | gibi | even always try to look for the VF of the macvtap in both cases (a) when booting with a proper macvtap, b) when change the port to macvtap and then rebooting) | |
| 11:55:36 | gibi | s/even// | |
| 11:55:58 | artom | So the first case is legit, obviously | |
| 11:56:02 | gibi | yes | |
| 11:56:06 | gibi | second shoudl fail | |
| 11:56:17 | gibi | but I cannot distinguish on the mock level | |
| 11:56:25 | artom | The breakage in the second case from the fact that we didn't free up the device when the port type changed | |
| 11:56:58 | gibi | in the func env we never consume VF the pci stuff is mocked out globally | |
| 11:57:11 | artom | No? We can't do something like assert_called_with(<netdev_path>)? | |
| 11:57:40 | gibi | we can do both in both case the path would be the same | |
| 11:57:52 | gibi | *but | |
| 11:58:16 | gibi | the difference is that the boot consumed someting in b) but not in a) | |
| 11:58:22 | gibi | but we does not track consumption | |
| 11:58:27 | gibi | as that is on the host OS level | |
| 11:58:58 | artom | Like, I'm not saying "stop asserting the service start failure" | |
| 11:59:12 | artom | Oh, I think I get it | |
| 11:59:46 | artom | There's no assertion we can make that would be different between "start with legit macvtap device" and "start with vnic_type changed macvtap" | |
| 11:59:52 | gibi | yes | |
| 11:59:58 | gibi | as the path and the logic is the same | |
| 12:00:03 | gibi | the diff happens during boot | |
| 12:00:30 | gibi | this is like extrnal state that we don't modell in test | |
| 12:00:50 | gibi | the boot changes the external state (the host OS) and the reboot will depend on that state | |
| 12:00:57 | gibi | but we don't carry that state in the test env | |
| 12:01:16 | gibi | we could, but we don't today | |
| 12:01:35 | gibi | we could create a proper stub for the pci module and track pci devices | |
| 12:03:11 | artom | That smells like a lot of work... :) | |
| 12:03:38 | artom | But... you understand why I find the test weird, right? Like, we go through all these change vnic_type steps, but they're all moot because of the mock | |
| 12:04:00 | gibi | yes, that would be a piece of work :) | |
| 12:04:15 | gibi | and yes, I got you, I try to figure out something better... | |
| 12:05:00 | artom | Sorry for not bringing any better solution :P | |
| 12:05:11 | gibi | basically the mock and the vnic_type change need to be coupled somehow... | |
| 12:05:43 | gibi | artom: no worries, you have a valid point, and it was a good excersise to talk it through | |
| 12:20:20 | gibi | artom: interestingly the hard reboot did not fail | |
| 12:20:57 | gibi | it created a proper macvtap interface and passed to the instance | |
| 12:21:28 | gibi | it is probably works because hard reboot unplugs the vif -> the VF will be freed the plug can lookup the netdev | |
| 12:22:10 | gibi | and the accounting in nova side is actually correct as the macvtap dev needs to keep the parent VF allocated to the instance (and that does not change during the reboot) | |
| 12:22:21 | gibi | so probably a direct -> direct-phyical change would not work | |
| 12:29:31 | artom | Ah, right, because it's the host OS accounting | |
| 12:29:36 | artom | So as long as we unplug first, we're fine | |
| 12:31:14 | gibi | in direct -> macvtap yes we are fine both on the OS level an in the nova PCI tracking | |
| 12:31:36 | gibi | in case of direct -> direct-phyical the PCI tracking in nova will be inconsistent | |
| 12:31:59 | gibi | as the instance will have a VF allocated but consuming a PF instead (probably) | |
| 12:58:33 | gibi | artom: I think I see a way to make the test more logical. We can check in the get_ifname_by_pci_address mock if the device being looked up is actaully consumed as a VF by the instance | |
| 13:00:58 | opendevreview | Balazs Gibizer proposed openstack/nova master: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/849985 | |
| 13:01:02 | gibi | artom: ^^ | |
| 13:17:30 | artom | Oooo, nice | |
| 13:18:50 | gibi | now if you move the port update before the boot then the mock will not raise the exception and the test will fail | |
| 13:19:23 | gibi | we have the external state but it is stored the domain in the fake libvirt connectioin | |
| 13:19:34 | gibi | * stored in the domain | |
| 13:23:44 | opendevreview | Balazs Gibizer proposed openstack/nova master: Gracefully ERROR in _init_instance vnic_type changed https://review.opendev.org/c/openstack/nova/+/850003 | |
| 13:23:58 | gibi | and here is the "fix" (two error logs and a skip) | |
| 13:24:26 | gibi | I have to leave early today so I will add unit test and reno to the patch on monday | |
| 13:29:07 | artom | Sure | |
| 13:43:16 | sean-k-mooney | gibi: ack ill review that shortly i just got back form doctors appoinent they were runing 90 mins behind so took longer then planned | |
| 13:44:54 | gibi | thanks | |
| 13:45:04 | gibi | no rush, I will probably look at it only on Monday | |
| 13:45:44 | sean-k-mooney | ack | |
| 13:56:31 | sean-k-mooney | gibi: i have set the public security flag on https://bugs.launchpad.net/nova/+bug/1981813 by the way just to highlight it to the security team | |
| 13:57:15 | sean-k-mooney | it was a public bug downstream so that ship had already sailed when we triaged it | |