| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-08-06 | |||
| 09:23:30 | songwenping | i replace VIR_NODE_DEVICE_DEFINE_XML_VALIDATE with 1 and the vm is created successful. | |
| 09:26:30 | frickler | I only found this single ref in nova code, does it mean this is not tested on 22.04? or does it need newer libvirt from UCA? can't check the latter right now https://opendev.org/openstack/nova/blame/branch/master/nova/virt/libvirt/host.py#L1267 | |
| 09:27:41 | gibi | nova declares libvirt min version as 8.0.0 | |
| 09:27:49 | gibi | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L221 | |
| 09:28:38 | frickler | but devstack loads uca, so maybe this isn't really tested anywhere | |
| 09:28:50 | gibi | yeah | |
| 09:29:27 | frickler | also MIN_LIBVIRT_VERSION is still 7.0.0, NEXT_MIN_LIBVIRT_VERSION is only for the future, isn't it? | |
| 09:29:56 | gibi | frickler: sorry you are right | |
| 09:30:07 | gibi | frickler: so yeah we should work with 7.0.0 | |
| 09:31:14 | gibi | but the persistent mdev feature that introduced the usage of the constant only enabled with 7.3.0 or higher https://github.com/openstack/nova/commit/74befb68a79f8bff823fe067e0054504391ee179#diff-486ddc37ce8e7c105a51566dd2cffbda034f28fae7eaad2e74abf470ab296352R248 | |
| 09:31:31 | gibi | still it seems the constant is missing in those old libvirts | |
| 09:42:18 | frickler | seems uca no longer has newer libvirt, so devstack is running with the same 8.0.0 currently that I checked. makes me wonder why tests aren't failing, or maybe there is no integrated test for mdevs? | |
| 09:42:30 | gibi | frickler: we test with 8.0.0 https://zuul.opendev.org/t/openstack/build/38facdd66a1e4e69bd6995fea4174d82/log/job-output.txt#9305 we just don't test mdevs | |
| 09:43:30 | sean-k-mooney | frickler: we dont test mdevs upstream yet | |
| 09:43:43 | sean-k-mooney | frickler: it requires us to compile a kernel module | |
| 09:43:49 | gibi | I think we will test is via https://review.opendev.org/q/topic:%22mtty_support%22 | |
| 09:43:55 | sean-k-mooney | frickler: bauzas is currently working on a job for that | |
| 09:44:12 | sean-k-mooney | gibi: yep that the series | |
| 09:44:48 | sean-k-mooney | although at the moment bauzas is working on it in a different job instead of nova-next like we orginally proposed | |
| 09:44:52 | gibi | frickler, sean-k-mooney: so to conclude we have a bug on master as with libvirt 7.3.0 (or 8.0.0) the persistent mdev feature does not work due to the missing constant libvirt.VIR_NODE_DEVICE_CREATE_XML_VALIDATE | |
| 09:45:05 | gibi | I will file a bug... | |
| 09:45:34 | sean-k-mooney | am we are ment to have a falback | |
| 09:45:42 | sean-k-mooney | thats form melwitt's series | |
| 09:46:11 | sean-k-mooney | we only supprot persistance in a later libvirt and falback to the old way when runingon older libvirt | |
| 09:46:17 | sean-k-mooney | at least that was teh plan | |
| 09:46:44 | gibi | https://github.com/openstack/nova/commit/74befb68a79f8bff823fe067e0054504391ee179#diff-486ddc37ce8e7c105a51566dd2cffbda034f28fae7eaad2e74abf470ab296352R248-R252 | |
| 09:46:47 | gibi | these are the checks | |
| 09:46:57 | gibi | on the libvirt version | |
| 09:47:04 | gibi | but the constant is not even in 8.0.0 | |
| 09:47:28 | sean-k-mooney | i wonder if they complied it out | |
| 09:48:31 | gibi | I don't know how to check that | |
| 09:48:49 | sean-k-mooney | that or that support came later. | |
| 09:49:31 | sean-k-mooney | https://github.com/libvirt/libvirt/blob/049846497b9e8a3eec82bfdcf3ebfe7c15e62c55/include/libvirt/libvirt-nodedev.h#L152 | |
| 09:49:43 | sean-k-mooney | so that looks liek the validation is a 8.10.0 feature | |
| 09:49:45 | gibi | https://github.com/libvirt/libvirt/commit/d8791c3c7caa6e3cadaf98a5a2c94b232ac30fed | |
| 09:50:01 | gibi | yeah I found the same thing | |
| 09:50:29 | sean-k-mooney | so looking at that | |
| 09:50:36 | sean-k-mooney | the validation was only added in 8.10. | |
| 09:50:37 | gibi | so we either need to remove the flag or change the version guard | |
| 09:50:45 | sean-k-mooney | right | |
| 09:50:59 | gibi | I lean towards changing th guard | |
| 09:51:02 | sean-k-mooney | so for 8.10+ we can pass the validation | |
| 09:51:03 | gibi | as this is on master | |
| 09:51:07 | sean-k-mooney | and below that no flag | |
| 09:51:30 | sean-k-mooney | why? | |
| 09:51:43 | sean-k-mooney | schma validation is nice to have but it was never part of the feature | |
| 09:52:25 | frickler | ah, likely that's why the experimental job runs on 24.04 which has 10.0.0 | |
| 09:52:37 | sean-k-mooney | frickler: thats not the only reason but one of them | |
| 09:52:37 | gibi | OK from that perspective we can make the flag version dependent. My viewpoint was that on master we can require newer libvirt | |
| 09:52:50 | sean-k-mooney | frickler: this code was tested with centos 9 with 8.10 | |
| 09:53:27 | sean-k-mooney | gibi: ya im not sure if we are alowed to do that bump this cycle or if we are allwoed to do it next | |
| 09:53:34 | sean-k-mooney | i wanted to do it on the non slurps | |
| 09:54:01 | sean-k-mooney | so advertise the next version during a slurp and bump in the next non slurp | |
| 09:54:19 | sean-k-mooney | meanign we could bump it now but we cant really do that until we move to noble | |
| 09:54:35 | sean-k-mooney | so for this release i woudl just add the version check | |
| 09:54:40 | gibi | sean-k-mooney: ack, make sense, we have a dependency on noble :/ | |
| 09:54:46 | sean-k-mooney | and we can remove that when we bump | |
| 09:58:02 | gibi | https://bugs.launchpad.net/nova/+bug/2076163 | |
| 10:57:30 | elodilles | auniyal Uggla : when you have some time, could you please take a look at these release patches? https://review.opendev.org/c/openstack/releases/+/925646 & https://review.opendev.org/c/openstack/releases/+/925647 | |
| 10:57:53 | elodilles | if you have any question then feel free to ping me | |
| 12:21:48 | jlejeune | elodilles: hello, after your release, will you have some time to review my patchs please ? https://review.opendev.org/c/openstack/nova/+/899379 and https://review.opendev.org/c/openstack/nova/+/901655 | |
| 12:54:50 | elodilles | jlejeune: just to note, i'm not core reviewer on master branch, I don't have +2 rights on master, I'm only stable maintainer core member o:) | |
| 13:01:26 | jlejeune | elodilles: ho sorry for the misunderstanding, I saw that you were very active so I thought you were a core reviewer, my bad :) | |
| 13:04:22 | sean-k-mooney | jlejeune: https://review.opendev.org/c/openstack/nova/+/899379 is not a correct fix | |
| 13:06:03 | sean-k-mooney | jlejeune: also as a aside no one form the core team has reviewd https://bugs.launchpad.net/nova/+bug/2040986 to confirm if its even valid and ^ did not include a repoducer functional test to demponstarte the bug | |
| 13:17:04 | jlejeune | ok, sorry I'm not yet very familiar with the process... | |
| 13:17:30 | jlejeune | now you have seen the bug report, what do you think about it ? | |
| 13:21:03 | sean-k-mooney | so https://bugs.launchpad.net/nova/+bug/2040986 is partly valid. the request spec shoudl not have the property for iamge b | |
| 13:21:06 | sean-k-mooney | when it faild | |
| 13:21:23 | sean-k-mooney | but the issue is not that we dont revert request spec content | |
| 13:21:33 | sean-k-mooney | it shoudl not have been updated unless it passed the schduler | |
| 13:22:15 | sean-k-mooney | fixing the issue in the excpt class is invliad as propsoed because its trying to use the info form glance and not corm the cell db copy | |
| 13:22:32 | jlejeune | yes we are treating the symthom and not the bug itself, agree about that | |
| 13:22:37 | sean-k-mooney | ideally we woudl fix the orginal issue by not saving the modifed request spec until after the schduler says its ok | |
| 13:23:44 | sean-k-mooney | jlejeune: back to the procedure topic the first step in fixing the bug should be to create a functional regression test for the bug to demonstarte the incorrect behaivor | |
| 13:24:54 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/test_bug_1732947.py and https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/test_bug_2040264.py are two examples of rebuidl regression tests for past bugs | |
| 13:25:48 | sean-k-mooney | https://github.com/openstack/nova/commit/21fd0c430c714d21c52e0a0c996351c374a3e3d6 and https://github.com/openstack/nova/commit/ee9ed0f7c6abf7c4847e6dc31f6d3d79b25b9d99 | |
| 13:25:55 | sean-k-mooney | demontrate the standard pattern | |
| 13:41:04 | jlejeune | are the regression tests are always mandatory ? because I see a lot of changes without them | |
| 13:45:02 | artom | jlejeune, strongly encouraged when fixing bugs. Our so-called "functional" tests are advanced enough that most bugs, even races, can be reproduced with them, with judicious use of stubs or mocks. The accepted standard practice is to first demonstrate the bug in a functional test, then with the patch on top demonstrate that the fix does what it's supposed to. | |
| 13:45:28 | artom | jlejeune, other than that, regression tests are not mandatory (and even for bug fixes there can be exceptions, depending on the nature of the bug). | |
| 13:47:24 | sean-k-mooney | jlejeune: we dont always requrie a funcitonal repoducer sometime unit test repoducer are also fine but for anyting like rebuild or an change that is not targeted to a very speciifc part of code i.e. the xml generation or logging its almost alwayys requried | |
| 13:51:12 | jlejeune | ok so in that case, I need to ? | |
| 13:52:12 | sean-k-mooney | yes in this case you are trying to fix an issue that hapens as a result of multipel api action and an interaction betwwen the conductor and schduler resulting in incositenies betwwen two dbs | |
| 13:52:31 | sean-k-mooney | that is not somethign that unit tests are sufficent to demondatate | |
| 13:52:43 | sean-k-mooney | so we woudl generally requrie a fucntioanl repoducer test | |
| 13:53:01 | jlejeune | ok | |
| 13:54:31 | sean-k-mooney | this is a littel out of date but this is where that was orignally documented https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/README.rst#tests-for-specific-regressions | |
| 13:54:56 | sean-k-mooney | the bit that is a littel out of date is "These should be full stack tests which inherit from nova.test.TestCase directly." | |
| 13:55:04 | sean-k-mooney | that is still prefered but not required anymore | |
| 13:55:32 | sean-k-mooney | it can be a heavy lift to set up all the fixtures unless you are deeply familar with how our test work | |
| 13:56:15 | opendevreview | Artom Lifshitz proposed openstack/nova master: POC/DNM: Remove vTPM live migration block https://review.opendev.org/c/openstack/nova/+/925771 | |
| 13:56:25 | sean-k-mooney | so we do allow inheriting form ntegrated_helpers._IntegratedTestBase | |
| 13:56:55 | sean-k-mooney | as is done in https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/test_bug_1732947.py | |
| 13:57:45 | sean-k-mooney | or sometimes even the libvirt specific vairiant however if its not virt driver specific we avoid using the more complex base classes | |
| 13:59:26 | jlejeune | ok I will try | |
| 14:11:25 | jlejeune | artom: sean-k-mooney: and what about that one: https://review.opendev.org/c/openstack/nova/+/901655 ? | |
| 14:13:01 | artom | jlejeune, IMO that's simple enough to not need a reproducer. | |
| 14:13:44 | sean-k-mooney | that ism more incliend to say is a valid bug but on the other hand im not eintrly sure its safe to not mark the instnace as in error | |
| 14:14:27 | sean-k-mooney | if the rpc timeout happens in pre live migrate i agree the instance should stay in active | |