Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-01-23
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
13:39:21 gibi the two check uses a different cpu.model for comparision
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)
13:51:25 gibi kashyap: you will only see the changed behaivor if the test is configuring incompatible model without extra flags.
13:51:50 kashyap gibi: True
13:51:55 gibi in the baseline that will fail the startup of nova-compute in the new code it will let the nova-compute start
13:52:08 gibi but fail all instance boot instead
13:52:15 kashyap I'm trying to find a reasonable compromise here: so shall I put both the blocks back into the code and add the workaround?
13:52:24 kashyap (s/blocks/calls/)
13:53:08 kashyap gibi: Right, failing at instance boot is still "something" - as it still provides a libvirt error. But I know we've already discussed and that's not 'acceptable'
13:53:50 sean-k-mooney i condier the bug you are introduceing by removign all cpu checks in the case that cpu flags is empty to be more serios then the bug you are trying to fix
13:53:54 kashyap sean-k-mooney: Keeping the first call in tact will break upstream deployments whenever using Icelake and Cascadelake
13:54:21 sean-k-mooney[m] the code i put in a comment in the reveiw will work for that case
13:54:25 kashyap sean-k-mooney: gibi: Okay: I'll keep both the checks in and wrap 'em around the workaround.
13:54:29 sean-k-mooney[m] and provide the corect behavior in other cases
13:54:32 kashyap Does that suit?
13:54:42 sean-k-mooney[m] have you looked at the code i provided

Earlier   Later