Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-01-23
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
13:39:39 sean-k-mooney gibi: right and the second check is not looping over the lsit fo cpu_modles
13:40:22 sean-k-mooney so we need to 1 loop over them using self._get_cpu_model_mapping(model) and 2 apply all cpu flags before doing the compare
13:41:17 sean-k-mooney 3 the comapre should not be condtional on extra flags and should happen even when empty
13:41:53 kashyap Hm, that could make whole code flow to be changed and a lot more surface impact :-( - I wonder if we try it on _top_ of the workaround?
13:41:57 gibi basically to see that each configred model with all the configured flags applied are still compatible with the hypervisor
13:42:39 kashyap gibi: Yeah, that's what libvirt will be doing under the hood "by default".
13:43:11 gibi kashyap: I don't think so. I guess libvirt will do the check during boot with the selected model + extra flags
13:43:24 gibi not each model + flags at startup
13:43:30 kashyap (I'm just thinking if we can "do this additional improvement" on _top_ of the current workaround + API replacement patch?)
13:43:56 gibi I guess if we want the workaround to land independently then we need to add back the old check and make both check optional with the WA flag.
13:44:12 gibi sean-k-mooney: ^^ what do you think?
13:44:39 kashyap Hmm, the first check is wrong unilaterally - sean-k-mooney agrees that too, IIUC
13:44:56 kashyap (As based on his suggestion is the current revision)
13:45:42 kashyap (And thanks for your patience so far, folks!)
13:45:55 gibi then I'm confused, as we discussed above that the second check alone is not enough.
13:46:07 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/870794/8/nova/virt/libvirt/driver.py#992
13:46:14 sean-k-mooney kashyap: actully i dont
13:46:17 gibi as it does nothing when no extra flags I configured
13:46:30 sean-k-mooney kashyap: i said its condtionally wrong if and only if you use extra_flags
13:47:03 sean-k-mooney anyway i posted what i think is the correct code in the comment above ^
13:48:16 kashyap sean-k-mooney: Well, I'm fine if you've refined your view _now_. But a few days ago you did write here:
13:48:19 kashyap (quote)
13:48:22 kashyap < sean-k-mooney> but if you want to future proof then sure you can put the remain check under a workaroudn
13:48:25 kashyap [...]
13:48:27 kashyap < sean-k-mooney> or just delete the first check
13:48:30 kashyap (/quote)
13:49:11 kashyap (Doing "nothing when no extra flags" are configured is still fine - as it was already explicitly tested by a downstream bug reporter in a real env)
13:49:19 sean-k-mooney yes i did not express my concern at that point with the looping as i tought i has already disucsst that with you
13:49:30 kashyap Okay.
13:49:59 sean-k-mooney kashyap: its not fine because its an upstream regression
13:50:00 kashyap gibi: sean-k-mooney: Also it was tested with the workaround as it stands: https://bugzilla.redhat.com/show_bug.cgi?id=2138381#c45
13:50:12 kashyap (Unforunately, it's a private comment, so only RHTers can see it)
13:51:12 kashyap (Hmm, perhaps ignore that test, I think he tested it with the first revision)

Earlier   Later