| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-21 | |||
| 15:57:35 | jpic | hi all, any idea why this VM won't boot anymore after being resized from 32 to 64 vCPU? It seems to complain about memory but the hypervisor seems to have way enough, boot logs: https://dpaste.org/b5Cis | |
| #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: compute: enhance compute evacuate instance to support target state https://review.opendev.org/c/openstack/nova/+/858383 | |
| 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: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 | 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:41 | sean-k-mooney | the code should treat it as an empty list of flags | |
| 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 | sean-k-mooney | it was only wrogn if you enabled a modle that is not compatible with the cpu | |