| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-07-15 | |||
| 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 | |
| 14:12:21 | opendevreview | Stephen Finucane proposed openstack/nova master: Remove unused requirement https://review.opendev.org/c/openstack/nova/+/850006 | |
| 14:12:21 | opendevreview | Stephen Finucane proposed openstack/nova master: Fix compatibility with jsonschema 4.x https://review.opendev.org/c/openstack/nova/+/849867 | |
| 14:12:37 | stephenfin | sean-k-mooney: Sorry, I missed your ping earlier. Respun with an arbitrary version now ^ | |
| 14:14:30 | sean-k-mooney | cool | |
| 14:14:39 | sean-k-mooney | will look soon | |
| 15:23:45 | opendevreview | Stephen Finucane proposed openstack/nova master: Bump jsonschema minimum to 4.0.0 https://review.opendev.org/c/openstack/nova/+/850021 | |
| 15:55:41 | sean-k-mooney | artom: you are aware that what you sugesed in https://review.opendev.org/c/openstack/nova/+/849104 is not allowed or what you are ment to do right | |
| 15:56:13 | artom | sean-k-mooney, what's not allowed? | |
| 15:56:37 | sean-k-mooney | using _IntegratedTestBase in new regression tests | |
| 15:56:55 | artom | o_O | |
| 15:57:00 | artom | Is that a new thing? | |
| 15:57:06 | sean-k-mooney | the regression tests are explicatly not ment to use the full test infra and we are not ment to use _IntegratedTestBase for new test in genera | |
| 15:57:19 | artom | ... | |
| 15:57:30 | sean-k-mooney | artom: yes its docuemnted in the readme for the regression tests i linked it in the commend inline | |
| 15:57:38 | artom | 19 | |
| 15:57:38 | artom | [artom@zoe nova]$ ag _IntegratedTestBase nova/tests/functional/regressions/ | wc -l | |
| 15:57:49 | sean-k-mooney | yep there are cases where its used | |
| 15:57:59 | sean-k-mooney | but we are trying not to do ti any more | |
| 15:58:16 | artom | Are we? Why? Because backports? | |
| 15:58:42 | sean-k-mooney | because regression tests are ment to be independing of the rest fo the fucntest so that they are not impacted by refacortings | |
| 15:59:10 | sean-k-mooney | it also help with backprots but that is not the main reason | |
| 15:59:33 | artom | I mean https://review.opendev.org/c/openstack/nova/+/812126 | |
| 15:59:45 | artom | That's at the beginning of this year | |
| 15:59:56 | sean-k-mooney | yep i know that should not have used that | |
| 16:00:31 | artom | Like, we can discuss this, and others can correct me if I'm wrong, but this is the first I hear of it, and I wasn't aware it was an established thing that all core reviewers had agreed on. | |
| 16:00:46 | sean-k-mooney | its in the readme | |
| 16:00:52 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/README.rst#writing-regression-tests= | |
| 16:01:09 | artom | And clearly at least *some* people have not read it :) | |
| 16:01:54 | sean-k-mooney | right this is a very long standing policy for that subdirectory | |
| 16:02:03 | artom | Of not reading it? :D | |
| 16:02:08 | sean-k-mooney | it was intoduced when it was created https://github.com/openstack/nova/commit/5fe04c2ee8b721ecdaea4d06de54b4998a0df395 | |
| 16:02:43 | sean-k-mooney | i had condiered asking gibi to rewrite there repoducer to follow it a few minute ago | |
| 16:03:01 | sean-k-mooney | but we dont require all repoduced to be added as freestandign regressions | |
| 16:03:02 | sean-k-mooney | so i did not | |
| 16:03:15 | sean-k-mooney | but if they are in that folder they should be freestanding like that | |
| 16:06:15 | artom | Genuinely news to me. | |
| 16:06:32 | artom | If we want to make that a thing I won't stand in anyone's ways, but I disagree that it's already a thing. | |
| 16:06:44 | sean-k-mooney | it reallly is | |
| 16:06:57 | artom | I mean in that case let's add a hacking rule | |
| 16:07:16 | artom | If it finds _IntegratedTestCase in the regressions folder, pep8 -1 | |
| 16:07:27 | artom | If we're *actually* going to be strict about it | |
| 16:07:34 | sean-k-mooney | artom: the other thing to keep in mind is that stephen had been trying to remvoe _IntegratedTestCase at one point | |
| 16:07:48 | artom | stephenfin's been trying to remove *everything* at one point ;) | |
| 16:07:49 | sean-k-mooney | so we should not be directlly adding new tests that use it as a base | |