Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-01-23
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
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?

Earlier   Later