| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-23 | |||
| 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 | |
| 13:54:49 | sean-k-mooney[m] | i would perfer if you used that | |
| 13:54:59 | kashyap | Not yet; /me goes to look | |
| 13:55:54 | sean-k-mooney | my network went down for a minute or two so i read scrollback on matrix so i think im caught back up | |
| 13:56:03 | gibi | sean-k-mooney: does that change works for the *lake issue even if the compare_cpu is not replaced with the compare_hypervisor_cpu call? | |
| 13:56:37 | sean-k-mooney | oh am i ment to use the new api but i think it would yes | |
| 13:56:38 | kashyap | sean-k-mooney: So you did this: | |
| 13:56:40 | kashyap | - cpu.model = self._host.get_capabilities().host.cpu.model | |
| 13:56:40 | kashyap | + cpu.model = self._get_cpu_model_mapping(model) | |
| 13:56:51 | kashyap | sean-k-mooney: (The new API patch comes on top of the workaround.) | |
| 13:57:05 | sean-k-mooney | not just that i also did a loop over teh enabled cpu models | |
| 13:57:23 | sean-k-mooney | you added support for multiple models a few release ago | |
| 13:57:39 | sean-k-mooney | but the cpu_flags aware code path | |
| 13:57:49 | sean-k-mooney | was not updated to loop over the list of models | |
| 13:57:53 | sean-k-mooney | so that is an existing bug | |
| 13:58:04 | kashyap | Oh, wait - you wrapped it under a for loop: "for model in models:" | |
| 13:58:10 | sean-k-mooney | yes | |
| 13:58:24 | kashyap | Lemme try the tests to see what breaks; again - such a change will be done on _top_ of the workaround of both the calls | |
| 13:58:30 | sean-k-mooney | and i made sure the compare was not condtional on the flags | |
| 13:58:40 | sean-k-mooney | kashyap: no | |
| 13:58:46 | sean-k-mooney | this need to be in the first patch | |
| 13:59:31 | sean-k-mooney | well | |
| 13:59:33 | kashyap | sean-k-mooney: Well, we can't be 100% sure this works in all cases. So that's too much of a risk to take | |
| 13:59:38 | sean-k-mooney | it depend on what you mean by on top of | |
| 13:59:44 | kashyap | Because we don't have the bandwidth to test all the possibilities | |