| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-23 | |||
| 05:12:59 | opendevreview | Amit Uniyal proposed openstack/nova stable/zed: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871413 | |
| 05:14:37 | opendevreview | Amit Uniyal proposed openstack/nova stable/yoga: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871414 | |
| 05:15:43 | opendevreview | Amit Uniyal proposed openstack/nova stable/xena: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871415 | |
| 05:16:41 | opendevreview | Amit Uniyal proposed openstack/nova stable/wallaby: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871416 | |
| 05:17:58 | opendevreview | Amit Uniyal proposed openstack/nova stable/victoria: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871417 | |
| 05:19:41 | opendevreview | Amit Uniyal proposed openstack/nova stable/ussuri: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871419 | |
| 06:09:36 | opendevreview | Amit Uniyal proposed openstack/nova stable/train: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871444 | |
| 06:26:41 | opendevreview | Amit Uniyal proposed openstack/nova stable/train: Improving logging at '_allocate_mdevs'. https://review.opendev.org/c/openstack/nova/+/871444 | |
| 07:52:13 | opendevreview | Rajesh Tailor proposed openstack/nova master: Handle InstanceInvalidState exception https://review.opendev.org/c/openstack/nova/+/861738 | |
| 09:47:50 | opendevreview | Rajesh Tailor proposed openstack/nova master: Handle InstanceInvalidState exception https://review.opendev.org/c/openstack/nova/+/861738 | |
| 10:00:28 | opendevreview | Sahid Orentino Ferdjaoui proposed openstack/nova master: api: extend evacuate instance to support target state https://review.opendev.org/c/openstack/nova/+/858384 | |
| 10:00:28 | opendevreview | Sahid Orentino Ferdjaoui proposed openstack/nova master: compute: enhance compute evacuate instance to support target state https://review.opendev.org/c/openstack/nova/+/858383 | |
| 10:07:58 | bauzas | gibi: so, wants me to move https://blueprints.launchpad.net/nova/+spec/pci-device-tracking-in-placement to Implemented so , | |
| 10:08:00 | bauzas | ? | |
| 10:08:50 | gibi | bauzas: yes please | |
| 10:08:54 | bauzas | ack | |
| 11:06:54 | opendevreview | Rajesh Tailor proposed openstack/nova master: Handle InstanceInvalidState exception https://review.opendev.org/c/openstack/nova/+/861738 | |
| 12:35:39 | opendevreview | Rajesh Tailor proposed openstack/nova stable/zed: Correct config help message related options https://review.opendev.org/c/openstack/nova/+/871247 | |
| 12:44:53 | kashyap | gibi: If you have time, appreciate a look at this failure: https://zuul.opendev.org/t/openstack/build/42cccae5e7b64091a9a63ed0f808c980. (I thought mocking _register_all_undefined_instance_details should suffice; but fails differently after that: https://paste.opendev.org/show/bNMZwe9Sh4vT5V9gA95t/) | |
| 12:59:08 | gibi | kashyap: you set up mocked_compare to return 2 but the code around self._host.compare_cpu in driver.py expect an libvirtError exception when failure happens | |
| 13:00:31 | kashyap | gibi: You're talking about this test in the above paste-bin, yeah? - test__check_cpu_compatibility_advance_model | |
| 13:00:42 | gibi | yepp | |
| 13:00:52 | gibi | with the extra mock form the paste applied | |
| 13:01:32 | kashyap | Hmm | |
| 13:02:04 | kashyap | gibi: The extra mock is the first step here, at least right | |
| 13:02:19 | gibi | kashyap: yepp, I think so | |
| 13:02:42 | gibi | what you mocked would call into the db and that is not allowed in that unit test hence the need for the mock | |
| 13:03:12 | kashyap | Right; I see. | |
| 13:04:15 | kashyap | gibi: Unless I'm being dense, the test is trying to raise an exception there, isn't it - line-12. | |
| 13:04:21 | kashyap | (In the pastebin) | |
| 13:04:54 | gibi | kashyap: nope, that point the test *expects* an exception is being raied by calling drvr.init_host | |
| 13:06:09 | kashyap | gibi: Hmm, how would you suggest to fix this? I'm a bit out of brain cells here | |
| 13:09:57 | kashyap | I didn't paste the last line of test traceback, but probably you saw it in Zuul: it's the "impl.MismatchError ... <bound method [...] returned None>" | |
| 13:10:47 | gibi | so looking at https://github.com/openstack/nova/blob/d8b4b7bebdc0f55353cd99f372044b9e30315a6d/nova/virt/libvirt/driver.py#L9978-L9993 | |
| 13:11:23 | gibi | you need to return a negative integer from mock_compare to trigger a failure case OR you have to raise libvirtError from mock_compare | |
| 13:12:00 | gibi | I'm not sure what the real libvirt behavior is, raise or return negative | |
| 13:12:08 | gibi | but the code linked above handles both | |
| 13:12:20 | kashyap | Aaah, the "if ret <=0" bit | |
| 13:15:38 | gibi | one more thing | |
| 13:15:46 | gibi | you mock nova.virt.libvirt.host.libvirt.Connection.compareCPU | |
| 13:16:10 | gibi | but I'm not sure that the actuall call reaches there in the unit test | |
| 13:16:20 | kashyap | Hmm | |
| 13:16:51 | gibi | I think in that unit test the call reaches nova.tests.fixtures.libvirt.Connection.compareCPU | |
| 13:16:55 | gibi | instead | |
| 13:17:05 | kashyap | (Aside: returning negative integer didn't help; I tried - "mocked_compare.side_effect = -1") | |
| 13:18:14 | kashyap | gibi: Hmm, it's odd that it is reaching the fixture in this case, rather than the host.libvirt | |
| 13:18:22 | gibi | let me check... | |
| 13:18:54 | kashyap | (Another test is failing the same way, actually. If you want to pull the patch in: https://review.opendev.org/c/openstack/nova/+/870794/) | |
| 13:23:19 | gibi | hm, there is a bug in the code. We removed the first check and kept the second but actually the second check runs on each cpu_model_extra_flags flag one by one. If no flags defined there then no check runs at all | |
| 13:23:32 | gibi | the latter happens in the unit test | |
| 13:24:34 | kashyap | gibi: Hmm, I sure tried adding extra_flags to this test. Maybe I mixed up, lemme try adding one in the test | |
| 13:25:24 | kashyap | gibi: You mean "bug in the code" or bug in the unit test code -- to be modified to reflect the new reality | |
| 13:25:44 | kashyap | gibi: Sure enough, adding this line succeeds the test: | |
| 13:25:45 | kashyap | + cpu_model_extra_flags = ["-aes"], | |
| 13:25:52 | gibi | it is more like a question. Do we want to run the check if on extra flags are configured? | |
| 13:26:54 | kashyap | gibi: Yeah, we do want to check to run (_compare_cpu) when the extra_flags are configured -- is that what you ask? | |
| 13:27:15 | gibi | scratch the nova.tests.fixtures.libvirt.Connection.compareCPU issue that was my mistake | |
| 13:27:46 | kashyap | gibi: I had to add the above diff, _and_ also return a negative integer for mocked_compare | |
| 13:28:16 | kashyap | gibi: So that's the fully modified test - https://paste.opendev.org/show/bPZk3eYZzuS4F7yazGqK/ | |
| 13:28:22 | kashyap | See line-6 and line-9 | |
| 13:28:42 | gibi | kashyap: my question is: if there is no extra flags configured, should we still run the compare_cpu or is it OK to only run the compare_cpu if there are extra flags configured | |
| 13:29:02 | gibi | kashyap: yepp tha paste matches what I did | |
| 13:29:04 | sean-k-mooney | gibi: we should run the compare if there are no extra flags | |
| 13:29:13 | sean-k-mooney | we just dont need to modify the model | |
| 13:29:20 | gibi | then we need to change the code up a bit | |
| 13:29:41 | sean-k-mooney | the code should treat it as an empty list of flags | |
| 13:29:41 | gibi | as this for loop is empty if no extra flags provided https://review.opendev.org/c/openstack/nova/+/870794/8/nova/virt/libvirt/driver.py#992 | |
| 13:29:57 | kashyap | sean-k-mooney: We never modify the model ourselves | |
| 13:30:13 | gibi | sean-k-mooney: also I'm not sure you see that but this code compares the extra flag independently one by one, not all at once | |
| 13:30:15 | sean-k-mooney | well not the model but the cpu definition | |
| 13:31:14 | sean-k-mooney | what we want the algortim to do is generate teh cpu xml from the modle then apply any flags that are present and then do one compare | |
| 13:31:26 | sean-k-mooney | it should do that for each model in cpu_models | |
| 13:31:28 | kashyap | gibi: Can you summarize your observation in the comment, please? (Also - the one-by-one flag comparison - I don't see an issue there) | |
| 13:31:36 | sean-k-mooney | and raise an error if any of them fail | |
| 13:31:58 | gibi | kashyap: sure | |
| 13:32:14 | sean-k-mooney | doing it flag by flag is an optimisation of that in that it will fail on the first flag that is not supported | |
| 13:32:30 | sean-k-mooney | but its not really sematicaly correct | |
| 13:32:45 | sean-k-mooney | for example to disable TSX you need to remove two flags | |
| 13:32:54 | sean-k-mooney | doing it one at a time will fail in that case | |
| 13:33:07 | kashyap | gibi: Also, I'm not sure I agree with sean-k-mooney on to run the _compare_cpu() when there are no flags there | |
| 13:33:22 | sean-k-mooney | no flags means do not modify the enabled model | |
| 13:33:24 | kashyap | sean-k-mooney: If you run it w/o the flags, then that means it's the same as the first call that we removed | |
| 13:33:31 | sean-k-mooney | so we still need to check that for validity | |
| 13:33:37 | kashyap | Does what I say above that make sense? | |
| 13:33:54 | sean-k-mooney | kashyap: the first call is correct where teh model is comaptible with the host | |
| 13:34:51 | sean-k-mooney | flags is optional and only needed fi teh default models shipped by lbivrt/qemu are not sufficent fro your usecase | |
| 13:35:04 | sean-k-mooney | where they are you should not be requried to use it but we should still validate the model | |
| 13:35:58 | gibi | there is a difference between the old check we removed and using the new check with the empty flag list | |
| 13:36:00 | sean-k-mooney | i mentioned in one of my reviews i was not sure the current compare was correct and the per flag iteration was one of my concerns | |
| 13:36:12 | kashyap | sean-k-mooney: Huh, if we retain the second call when no flags are given, then there's no difference with the _removed_ call in this patch -- gibi: does this make sense? | |
| 13:36:35 | gibi | the old check used cpu.model = self._get_cpu_model_mapping(model) as the the one to compare while the remaining check uses self._host.get_capabilities().host.cpu.model + the extra flags | |
| 13:36:38 | sean-k-mooney | the first call was only incorrect if you use extra flags | |
| 13:36:54 | kashyap | [ What's the diff with the new (or rather the 2nd check) with an empty flag list? Ah, you just write above] | |
| 13:38:06 | sean-k-mooney | well the second check is wrong because its not looping over al the models enabeld in cpu_models | |
| 13:38:13 | kashyap | Hmm, I'm trying to minimize the "impact surface" here. | |
| 13:38:47 | gibi | I guess we are back to the drawing board then | |
| 13:38:57 | kashyap | gibi: The first check was wrong anyway: so that we can remove w/o doubt. | |
| 13:39:10 | sean-k-mooney | no not in all cases | |
| 13:39:21 | gibi | the two check uses a different cpu.model for comparision | |
| 13:39:21 | sean-k-mooney | it was only wrogn if you enabled a modle that is not compatible with the cpu | |