| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-23 | |||
| 13:56:40 | kashyap | + cpu.model = self._get_cpu_model_mapping(model) | |
| 13:56:40 | kashyap | - cpu.model = self._host.get_capabilities().host.cpu.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 | |
| 13:59:50 | sean-k-mooney | if you mena later in the series then no if you mean in addtion too then yes | |
| 14:00:05 | kashyap | The safest is to wrap the 2 calls in the workaround: and _then_ do any code-flow changes on top | |
| 14:00:15 | kashyap | Does that sound reasonable? (Cc: gibi) | |
| 14:00:17 | sean-k-mooney | kashyap: as it stand i cant really supprot backporting the changes you are proposing downstream or upstream | |
| 14:00:43 | kashyap | sean-k-mooney: Please see above comment - do you see what I mean? | |
| 14:01:10 | sean-k-mooney | i do but i dont agree that is what we should do | |
| 14:02:01 | gibi | some of the risk coming from the incomplete test covarege is mitigated by having the WA flag I assume | |
| 14:02:22 | kashyap | sean-k-mooney: Shall we please arrive at a reasonable compromise, instead of "ideally situations? | |
| 14:02:25 | kashyap | gibi: Exactly | |
| 14:02:40 | sean-k-mooney | yes but i dont think we can recommend that customer use that without closing some of those gaps and qe testing | |
| 14:02:46 | kashyap | We can't possibly test all cases. So I'm not confident in any code-flow w/o absolute confidence that it's introducing unwanted regressions | |
| 14:03:20 | sean-k-mooney | kashyap: i am confident that disablinbg the check is a regession and im not comfrotabel with use telling custoemr to do that | |
| 14:03:27 | kashyap | sean-k-mooney: Addressing gaps and QE testing can come later. We take step by step based on the available bandwidth. | |
| 14:04:14 | gibi | we don't have the coverage over the current code either, and we see a bug in it for the *lake CPUs. We want to fix the bug by removing a buggy check. But by removing the buggy check we remove the good part of the check too. | |
| 14:04:59 | gibi | sean-k-mooney proposed a fix that does not remove the buggy check but improve it | |
| 14:05:10 | gibi | sure it has a testing risk as we have no coverage | |
| 14:05:50 | gibi | so the we have to decide what is bigger the lost good part of the check if we remove it or the risk by fixing the check but not having full coverage | |
| 14:05:51 | kashyap | Okay, let me try sean-k-mooney's suggestion from their comment | |
| 14:06:02 | kashyap | And "see what happens" with tests | |
| 14:06:16 | kashyap | I'll come back and comment on the change. | |
| 14:06:20 | sean-k-mooney | we can test it with the functional tests we just need to write a test for it | |
| 14:06:48 | sean-k-mooney | but i agree the test coverage is not complete and we might want to do that as a followup | |
| 14:06:53 | gibi | sean-k-mooney: we don't have the real libvirt code running in functional | |
| 14:07:16 | gibi | we have the libvirt/driver but we don't have the compareCPU from libvirt itself | |
| 14:07:19 | sean-k-mooney | we dont but we could enhance the fixture if we really wanted | |
| 14:07:41 | sean-k-mooney | i was thinking if we could test this in ci via tempest but we dont really contole the enve enought to do that reliably | |
| 14:08:01 | sean-k-mooney | so i dont really want to go down that route | |
| 14:08:11 | gibi | yeah, best would be to test this with real hardver and the TSX case but that is haaard | |
| 14:08:34 | gibi | (should be possible downstream though) | |
| 14:09:29 | sean-k-mooney | with whitebox we coudl proably do it | |
| 14:09:46 | kashyap | I'm running the UTs with Sean's suggestion on top this: https://review.opendev.org/c/openstack/nova/+/870794 | |
| 14:10:02 | sean-k-mooney | how we woululd do it upstream in tempest is selct a cpu_model that we knwo is unaviable | |
| 14:10:11 | kashyap | (If that gives some confidence in a day or so. I can wrap it in the workaround.) | |
| 14:10:12 | sean-k-mooney | and disable the cpu flag to make it look like nehalem | |
| 14:10:37 | sean-k-mooney | the problem is that a provider could add that model in teh future | |
| 14:10:54 | sean-k-mooney | the best way to do that would be to use a model that requests a remvoed feature like TSX | |
| 14:11:01 | sean-k-mooney | and disable tsk via the extra flags | |
| 14:11:39 | sean-k-mooney | presumable our ci provireder also dont have tsx avaible anymore | |
| 14:11:48 | sean-k-mooney | if they have been updating htere kernels/microcode | |
| 14:12:08 | kashyap | Yeah, we don't know that, and can't rely on it. | |
| 14:15:31 | gibi | so my position is that let's take the risk and doing sean-k-mooney's fix but have the WA flag added. So if the change is good then all is golden, but if the change has some unforseen side effect then we can turn it off while we improve it. For me the risk of this scenario is acceptable. | |
| 14:16:20 | kashyap | Right, I'm testing Sean's suggestion with a workaroud wrapper | |
| 14:16:26 | sean-k-mooney | ack | |
| 14:16:35 | kashyap | gibi: Can you also review the code that sean-k-mooney suggested, please? Two eyes are better :) | |
| 14:16:38 | sean-k-mooney | so you testing just one check the new one i pasted ya | |
| 14:16:44 | sean-k-mooney | and that wrapped in the workaround flag | |
| 14:16:54 | kashyap | sean-k-mooney: Your comment on PS8, yeah | |
| 14:16:55 | gibi | kashyap: I checked it it looked OK to me at first glance | |
| 14:16:58 | sean-k-mooney | cool | |
| 14:18:05 | sean-k-mooney | ok i want to prepare for meeting ectra so ill be back in a bit | |
| 14:18:10 | kashyap | Sure; thanks, folks. | |
| 14:32:19 | kashyap | gibi: sean-k-mooney: The first brush looks good :) I only had to modify the mocked_compare in 3 unit tests to return a negative integer. | |
| 14:43:09 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: Replace usage of compareCPU() with compareHypervisorCPU() https://review.opendev.org/c/openstack/nova/+/869950 | |
| 14:43:09 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: At start-up rework compareCPU() usage with a workaround https://review.opendev.org/c/openstack/nova/+/870794 | |
| 14:43:31 | kashyap | sean-k-mooney: --^ (gibi: On the UTs: with the negative integer added, the extra mock is not required even.) | |
| 14:43:37 | kashyap | Thanks for bearing with me! | |
| 14:50:14 | opendevreview | Artom Lifshitz proposed openstack/nova master: DNM: demo 2 https://review.opendev.org/c/openstack/nova/+/871481 | |
| 14:50:14 | opendevreview | Artom Lifshitz proposed openstack/nova master: DNM: demo 1 https://review.opendev.org/c/openstack/nova/+/871480 | |
| 14:52:07 | opendevreview | Artom Lifshitz proposed openstack/nova master: DNM: demo 1 https://review.opendev.org/c/openstack/nova/+/871480 | |
| 14:52:07 | opendevreview | Artom Lifshitz proposed openstack/nova master: DNM: demo 2 https://review.opendev.org/c/openstack/nova/+/871481 | |
| 14:52:08 | opendevreview | Artom Lifshitz proposed openstack/nova master: DNM: demo middle https://review.opendev.org/c/openstack/nova/+/871482 | |
| 15:58:59 | sahid | o/ - quick question I have to rebase on conflict a micro-version change, I have some tests failing but struggling to find the issue https://paste.ubuntu.com/p/RNw8yxQtj6/ | |
| 15:59:05 | sahid | any idea that can help me? | |
| 16:01:34 | sahid | Template: ^2.95$ | |
| 16:01:36 | sahid | Sample: 2.94 | |
| 16:01:49 | sahid | I don't see where this 2.94 is comming from | |
| 16:57:55 | bauzas | sahid: are you sure you updated all your API change files so it now uses 2.95 ? | |
| 17:04:37 | bauzas | sahid: commented your patch | |
| 17:34:03 | sahid | bauzas: well I want to say yes but I have probably missed something :-) | |
| 17:34:10 | sahid | I will double check | |
| 17:34:45 | bauzas | sahid: I tried to quick looked at the test to understand why it autogenerates 2.94 | |
| 17:34:53 | bauzas | to quickly look* | |
| 17:35:11 | bauzas | but I haven't found a lot, if you still have the problem, you could try to pdb it | |
| 17:35:28 | bauzas | to find how it generates this template | |
| 17:35:50 | bauzas | if you can't, I can offer my help tomorrow | |
| 17:37:36 | sahid | no worries thanks to have looked at it. I will double check that tomorrow as-well, don't spend time on it I will ping you when it's ready :-) | |
| 18:28:21 | opendevreview | Sofia Enriquez proposed openstack/nova master: WIP: Implement encryption on backingStore https://review.opendev.org/c/openstack/nova/+/870012 | |
| 18:53:25 | opendevreview | Sahid Orentino Ferdjaoui proposed openstack/nova master: api: extend evacuate instance to support target state https://review.opendev.org/c/openstack/nova/+/858384 | |
| #openstack-nova - 2023-01-24 | |||
| 08:50:59 | 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:28:53 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: At start-up rework compareCPU() usage with a workaround https://review.opendev.org/c/openstack/nova/+/870794 | |
| 10:28:54 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: Replace usage of compareCPU() with compareHypervisorCPU() https://review.opendev.org/c/openstack/nova/+/869950 | |
| 10:29:24 | kashyap | sean-k-mooney: --^ Good catch in the workaround patch; addressed your remarks. When you can, lemme know if that looks fine :) | |