| opendevreview | Christophe Useinovic proposed openstack/ceilometer master: Update compute.discovery to get nova domain meta https://review.opendev.org/c/openstack/ceilometer/+/812508 | 14:06 |
|---|---|---|
| cuseinovic | Hi guys, First guerrit and patch. Can you tell me if it's good for you ? https://review.opendev.org/c/openstack/ceilometer/+/812508/ | 14:21 |
| cuseinovic | please :) | 14:21 |
| mrunge | cuseinovic: thank you for the patch | 14:39 |
| mrunge | cuseinovic: could you also please provide a test? | 14:39 |
| cuseinovic | Zuul crash but i think I don't know if it's due to my fix https://zuul.opendev.org/t/openstack/build/096d40aa8d364ea588edd72a5acb0153/log/job-output.txt?severity=2 | 14:41 |
| cuseinovic | hum with what kind of test ? with this ? https://docs.openstack.org/project-team-guide/project-setup/python.html#running-python-unit-tests | 14:42 |
| mrunge | the test is to make sure someone else would not break your change in a future change | 14:46 |
| cuseinovic | okey i will try to do that | 14:47 |
| mrunge | I wonder if it makes sense to make it configurable | 14:51 |
| mrunge | this seems to be dependent on libvirt versions | 14:54 |
| cuseinovic | yeah the patch it's for Wallby and next releases | 14:56 |
| mrunge | for example,. I have a newer version of libvirt on my train based deployment | 14:57 |
| mrunge | and also a newer version of qemu | 14:57 |
| cuseinovic | I though that libvirt version need to follow the nova version package like other packages of Openstack component | 15:00 |
| cuseinovic | But this change affects code that should already be tested | 15:02 |
| mrunge | you see it's not being tested, otherwise your patch would raise an issue | 15:04 |
| mrunge | okay, taking a step back, the namespace is defined here: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L40 | 15:08 |
| cuseinovic | okey, can you tell me how to test locally please ? :) | 15:08 |
| mrunge | and the switch from version 1.0 to 1.1 was from victoria to wallaby | 15:09 |
| mrunge | uhm, for running tests: tox -l | 15:09 |
| mrunge | tox -e (test_name) | 15:09 |
| mrunge | or just tox | 15:09 |
| cuseinovic | yeah exactly from V to W release | 15:11 |
| cuseinovic | ok | 15:11 |
| mrunge | cuseinovic: thinking again about a potential test, it wouldn't make so much sense to hard-code a test to check for a hard-coded string, no? | 15:28 |
| cuseinovic | yes ofc | 15:29 |
| mrunge | with that, I'm good with the patch as is | 15:29 |
| jpic | to test such a function you have two solutions: either mock libvirt, ether spawn an actual libvirt process, both will be pretty heavy | 15:30 |
| mrunge | the string is hardcoded in nova | 15:30 |
| mrunge | with that, you | 15:30 |
| mrunge | 'd have to spawn a vm with nova | 15:30 |
| jpic | or just mock libvirt and fake all that with unittest.mock | 15:32 |
| jpic | in both case you'll have to maintain the test ... | 15:33 |
| opendevreview | Christophe Useinovic proposed openstack/ceilometer master: Update compute.discovery to get nova domain meta https://review.opendev.org/c/openstack/ceilometer/+/812508 | 15:36 |
| jpic | cuseinovic: what happened before you added this change exactly? did you get an Exception with a Traceback? | 15:37 |
| cuseinovic | @jpic it's was somehtink like this | 15:40 |
| cuseinovic | 2021-06-01 16:01:18.302 1835684 ERROR ceilometer.compute.discovery [-] Fail to get domain uuid aff042c9-c311-4944-bc42-09ccd5a90eb7 metadata, libvirtError: metadata not found: Requested metadata element is not present: libvirt.libvirtError: metadata not found: Requested metadata element is not present | 15:40 |
| cuseinovic | i put the debug mode for ceilo | 15:41 |
| cuseinovic | i saw the problem when gnocchi didn't give me any informations, i check the container ceilometer-compute in debug mode and i see that | 15:42 |
| cuseinovic | I checked the libivrt file and the discovery.py and it's was not good | 15:43 |
| cuseinovic | mrunge: Thank for your review, do i need to do more step now ? | 15:49 |
| mrunge | cuseinovic: I am hoping the CI issue was just a fluke | 15:49 |
| mrunge | if that's the case, I'll +A the patch later today or tomorrow | 15:50 |
| mrunge | in order to get it merged, we need to have CI passing. There may be a different patch required to get CI happy, and that has to merge before your patch | 15:51 |
| cuseinovic | ok i see. Thanks for your reply and your answer | 15:52 |
| jpic | ok so maybe the patch should still try with 1.0 and expect libvirt.libvirtError of Requested metadata element is not present prior to trying with 1.1 | 16:20 |
| jpic | in which case a test is really going to be necessary, but I don't know if that's actually relevant, how long has 1.1 been deployed, what versions of nova is it compatible with, I don't know about these details | 16:21 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!