[nova] Bug #1755266: How to proceed with test failures
Hi there, Since Pike I am struggling with instance migration/instance resize for flavors whose swap space is provided via an lvm volume; according to my understanding the default behavior if cinder uses lvm as a backend driver (at least I could not convince cinder to behave different …). I am somewhat surprised that I seem to be the only one who has some problems with that behavior - according to my understanding you are coming into this constellation automatically when simply following the manual installation procedure as being described in the official Openstack docs... Anyway I opened the bug above, however it did not find some interest and I tried then as a python newbie to get it fixed by my own. After a lengthy live test phase of my changes in the driver.py across Pike, Queens, Rocky, and now also Stein, I made then my first commit (yeee - got it done) to the master branch, had a good short conversation with Eric in Berlin on it, fixed some code format issues Zuul was rightfully complaining about but then unfortunately failed some further tests in Zuul and other test areas. (see https://review.opendev.org/#/c/618621/ <https://review.opendev.org/#/c/618621/>). Related to my changes, tox gets me an error: nova.tests.unit.virt.libvirt.test_driver.LibvirtDriverTestCase.test_finish_migration_power_on --------------------------------------------------------------------------------------------- Captured traceback: ~~~~~~~~~~~~~~~~~~~ Traceback (most recent call last): File "nova/tests/unit/virt/libvirt/test_driver.py", line 18707, in test_finish_migration_power_on self._test_finish_migration() File "/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched return func(*args, **keywargs) File "nova/tests/unit/virt/libvirt/test_driver.py", line 18662, in _test_finish_migration mock_raw_to_qcow2.assert_has_calls(convert_calls, any_order=True) File "/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py", line 983, in assert_has_calls ), cause) File "/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/six.py", line 737, in raise_from raise value AssertionError: (call('/tmp/tmpC60sPk/tmpOHikWL/8ea1de33-64d7-4d1d-af02-88e6f7ec91c1/disk.swap'),) not all found in call list but I failed yet to develop any idea what needs to be done to get the test failure fixed. I am missing context here how the logic of the test code works; therefore I would like to ask whether somebody could point me in the right direction what needs to be done to get the failed unit/Zuul/other Tests passed. Are there any docs or helps alongside testing in Nova where to learn what to do? Perhaps also someone could give me a hint whether there are some conceptually misleading ideas behind my fix proposal which need to be driven into another direction … I am looking forward to your reply Best regards Clemens Clemens Hardewig
Hi Clemens. First of all, thank you for digging into the code and working to fix the issue you're seeing.
I am missing context here how the logic of the test code works;
Note that it is normal (in fact almost always required) to change test code with any change to the production side. The purpose of unit tests (you can tell this is a unit test from '/unit/' in the file path) is to exercise a small chunk ("unit" :) of code and make sure branching and method calls are all as expected. You've changed some logic, so the test is (rightly) failing, and you'll need to change what it's expecting accordingly. These tests are making use of mock [0] to hide the guts of some of the methods being called by the unit in question, just to make sure that those methods are being invoked the correct number of times, with the correct arguments. In this case...
Related to my changes, tox gets me an error:
nova.tests.unit.virt.libvirt.test_driver.LibvirtDriverTestCase.test_finish_migration_power_on ---------------------------------------------------------------------------------------------
Captured traceback: ~~~~~~~~~~~~~~~~~~~ Traceback (most recent call last): File "nova/tests/unit/virt/libvirt/test_driver.py", line 18707, in test_finish_migration_power_on self._test_finish_migration() File "/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched return func(*args, **keywargs) File "nova/tests/unit/virt/libvirt/test_driver.py", line 18662, in _test_finish_migration mock_raw_to_qcow2.assert_has_calls(convert_calls, any_order=True) File "/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py", line 983, in assert_has_calls ), cause) File "/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/six.py", line 737, in raise_from raise value AssertionError: (call('/tmp/tmpC60sPk/tmpOHikWL/8ea1de33-64d7-4d1d-af02-88e6f7ec91c1/disk.swap'),) not all found in call list
The stack trace is pointing you at [1] for all three failures. I can see from your code change that you're skipping the qcow conversion for disk.swap here [2]. So when I remove 'disk.swap' from the list on L19438, the three tests pass. That will get you green in zuul, but I should warn you that one of the first things reviewers will notice is that you *only* had to change tests for the piece of your change at [2]. That means there's missing/incomplete test coverage for the other code paths you've touched, and you'll have to add some (or justify why it's unnecessary).
Are there any docs or helps alongside testing in Nova where to learn what to do?
I'm not sure if this is targeted at the right level for you, but here's the nova contributor guide [3]. If you're more of an interactive learner, feel free to jump on IRC in #openstack-nova and I'll be happy to walk you through some basics.
Perhaps also someone could give me a hint whether there are some conceptually misleading ideas behind my fix proposal which need to be driven into another direction …
Yup, that's what code review is for. Nova has a very high "open changes" to "reviewer bandwidth" ratio, so it's unfortunately pretty normal for changes to go unreviewed while they're still failing zuul testing. Getting those fixed up, and bringing attention to the issue here on the mailing list and/or IRC, should all get your change some better attention. Thanks again for diving in. efried [0] https://docs.python.org/3/library/unittest.mock.html [1] https://opendev.org/openstack/nova/src/branch/master/nova/tests/unit/virt/li... [2] https://review.opendev.org/#/c/618621/4/nova/virt/libvirt/driver.py@8410 [3] https://docs.openstack.org/nova/latest/contributor/index.html
Hi Eric, thank you that you have taken the time leading me through the process and answering extensively. Very much appreciated and insightful. Having digged now into the code in /nova/nova/tests/unit/virt/libvirt/test_driver.py, it is obvious that my proposal is not universal but fixes only my specific config (and make then other configs fail). However, it seems to me that a config that running cinder on each compute node with lvm backend creating root volume as lvm volume creating swap not as ephermal or swap (raw) disk but as lvm volume (as lvm/qemu does automatically) is not a supported model in nova yet to deal with instance resizing/migrations. Thanks again for your guidance, will go through it ... Br Clemens
Clemens-
However, it seems to me that a config that
* running cinder on each compute node with lvm backend * creating root volume as lvm volume * creating swap not as ephermal or swap (raw) disk but as lvm volume (as lvm/qemu does automatically)
is not a supported model in nova yet to deal with instance resizing/migrations.
I've asked a subject matter expert (which I certainly am not) to have a look at your change. Hopefully he can answer the above, which is pretty much Greek to me :) Thanks, efried .
participants (2)
-
Clemens Hardewig
-
Eric Fried