| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-03 | |||
| 12:35:19 | sean-k-mooney | jlejeune: to be honest i had tought we merged that already | |
| 12:36:09 | jlejeune | I know I have some work to do to tka in consideration your comment but I didn't have time to work on | |
| 12:36:28 | sean-k-mooney | thats fine we didnt have time to review it | |
| 12:46:13 | jlejeune | ok so I don't change my code and let someone else review it based on your opinion ? | |
| 12:55:24 | opendevreview | Takashi Kajinami proposed openstack/nova master: Replace deprecated glanceclient exception https://review.opendev.org/c/openstack/nova/+/914761 | |
| 13:04:16 | sean-k-mooney | jlejeune: dan has a stonger prefernce then i do. i was ok with the reductionin code execution because i dont think a unit test should be as large as that one is and the coverage shoudl eb provided by smaller more targed test or or functional test suite | |
| 13:32:47 | dansmith | sean-k-mooney: yes, merged, and that trace isn't in my code | |
| 13:33:08 | sean-k-mooney | dansmith: i know its just a missing mock | |
| 13:36:13 | dansmith | okay, I'm not sure what you're asking | |
| 13:37:34 | sean-k-mooney | i was just letting you knwo we still have once ase where its not mocked and jlejeune remindiend me that we had dicussed this on tehre patch previously https://review.opendev.org/c/openstack/nova/+/925208 | |
| 13:56:44 | jlejeune | sean-k-mooney: dansmith: not sure what I have to do about it now :) | |
| 13:59:08 | dansmith | tbh, I don't hate the very first rev of this patch, and I think it's better than the current one | |
| 13:59:22 | dansmith | I would str(e) and make sure there's a qemu-img in there so we only skip for that case | |
| 13:59:33 | dansmith | but that leaves us all the coverage we have today on systems that can test it, | |
| 13:59:42 | dansmith | and just skips it for limited environments | |
| 13:59:53 | dansmith | just like our opportunistic DB testing | |
| 14:07:34 | dansmith | jlejeune: ^ | |
| 14:11:59 | jlejeune | dansmith: ok, you mean I had to check the str(e) of the exception to detect if it's really related to qemu-img, in my first patchset ? | |
| 14:12:11 | dansmith | jlejeune: yeah | |
| 14:12:21 | jlejeune | ok clear, doing, thanks | |
| 14:12:27 | dansmith | sounded like sean-k-mooney was not okay with that approach though, but I'm not sure why.. it keeps the most coverage | |
| 14:12:39 | dansmith | jlejeune: might want to wait for sean-k-mooney to say that's okay as well | |
| 14:12:45 | jlejeune | ok | |
| 14:13:18 | sean-k-mooney | dansmith: i not entirly against it but form my point of view the unit test is not a unit test today | |
| 14:13:43 | sean-k-mooney | unit tests should not be calling code out side of the same call liek that | |
| 14:14:03 | sean-k-mooney | if you think we have a clean way to keep the coverage and make this work that you are happy with then thats fine | |
| 14:14:41 | dansmith | sean-k-mooney: you can argue that, sure, and I'm sure these exist since before the functional split, but unless you want jlejeune to refactor and move them out to functional, it seems like a reasonable "just fix the problem without the nuclear option of losing a bunch of coverage (regardless of the quality)" option | |
| 14:15:41 | sean-k-mooney | its a trade off between fragile tests and test coverage | |
| 14:16:06 | sean-k-mooney | dansmith: i dont have the review open right now but did the coverage report show a decresse in coverage? | |
| 14:16:48 | sean-k-mooney | a hte resulsts expired | |
| 14:16:53 | dansmith | sean-k-mooney: the only way it wouldn't is if we have all that code covered and mocked elsewhere, but the point of this test is to have some basic exercising of those things, | |
| 14:17:11 | dansmith | so that if we were to remove the specific tests we'd at least have this | |
| 14:17:12 | sean-k-mooney | dansmith: i dont think refactoring into the funcitonal test is a reasonable ask so i agree we shoudl just fix the exiting test for now | |
| 14:17:32 | dansmith | sean-k-mooney: right, me either, I was just using that to make the point :) | |
| 14:17:43 | sean-k-mooney | dansmith: ok so i disagree that that shoudl be its point | |
| 14:17:59 | sean-k-mooney | it should be testing the hihgh level interface | |
| 14:18:09 | sean-k-mooney | adn the lower leves shoudl be coverd elsewhere | |
| 14:18:23 | sean-k-mooney | btu for now if your happy with yoru proposed solution then lets proceed with that | |
| 14:18:27 | dansmith | again, I don't disagree with the "should" part.. just don't want to lose the "is currently" part | |
| 14:19:55 | sean-k-mooney | so you want to go back to https://review.opendev.org/c/openstack/nova/+/925208/1/nova/tests/unit/virt/test_virt_drivers.py then and add a check that the excption has qemu-img in it | |
| 14:20:04 | dansmith | if we get a process execution error, and that error mentions qemu-img in it, it's pretty obviously a very targeted case I think | |
| 14:20:09 | sean-k-mooney | just to make sure we only skip if that why | |
| 14:20:12 | dansmith | you could even look for "not found" or something if you want to get super targeted | |
| 14:20:13 | dansmith | right | |
| 14:21:06 | sean-k-mooney | if thats your prfernce then lets do that. i dont like skiping test after we tired to do soemthign but i think this is ok for now | |
| 14:21:29 | sean-k-mooney | when swaping to the oslo version hwoever we shoudl be mocking out the image inpection usage | |
| 14:21:46 | jlejeune | sean-k-mooney: +1 | |
| 14:21:53 | dansmith | again, I'm not sure what this has to do with moving to the oslo version | |
| 14:22:16 | dansmith | sean-k-mooney: you know this is *already* merged right? https://review.opendev.org/c/openstack/nova/+/925025 | |
| 14:22:48 | sean-k-mooney | no i was not aware that merged | |
| 14:24:52 | sean-k-mooney | the reason that relevent is while it might be ok to call | |
| 14:24:57 | sean-k-mooney | nova.image.format_inspector.detect_file_format | |
| 14:25:02 | sean-k-mooney | in a unit test inteded to test the driver interface | |
| 14:25:10 | sean-k-mooney | it feel much less ok to call "oslo_utils.imageutils.format_inspector.detect_file_format" | |
| 14:25:28 | dansmith | but we're not doing either of those here right?? | |
| 14:25:33 | dansmith | s/??/?/ | |
| 14:25:56 | sean-k-mooney | that what causing the qemu-image call as far as im aware | |
| 14:26:08 | dansmith | no, it's libvirt actually creating the snapshot | |
| 14:26:12 | dansmith | qemu-img create -f qcow2 ./tmpm1aqe2cp/25c6cab7c98f4cbba03223754b5d2e74.delta | |
| 14:26:19 | dansmith | this is definitely not from format inspector ^ | |
| 14:26:37 | opendevreview | Takashi Kajinami proposed openstack/nova master: Replace deprecated glanceclient exception https://review.opendev.org/c/openstack/nova/+/914761 | |
| 14:26:50 | sean-k-mooney | oh then this is an unrelatd case wehre we alredy depended on qemu-img? | |
| 14:26:56 | dansmith | this is all code that wasn't being tested at all before because it was being ignored since ImageMeta was missing a key and causing the test to bail super early for libvirt | |
| 14:27:00 | sean-k-mooney | i tought this was intoduced by the cve fix | |
| 14:27:21 | sean-k-mooney | oh right you changed it to not return NotImplemented | |
| 14:27:23 | dansmith | sean-k-mooney: yes, I fixed a case where we were not getting the coverage we expected and now we're actually running code we weren't before, that's why I don't want to go back to not covering that | |
| 14:27:25 | dansmith | right | |
| 14:27:27 | jlejeune | by the removal of AMI support actually | |
| 14:27:33 | sean-k-mooney | so its related but not directly because you fixed the missing test coverage | |
| 14:27:34 | dansmith | exactly, the AMI thing, unrelated to anything else | |
| 14:27:35 | sean-k-mooney | sorry | |
| 14:27:38 | opendevreview | Takashi Kajinami proposed openstack/nova master: Replace deprecated glanceclient exception https://review.opendev.org/c/openstack/nova/+/914761 | |
| 14:28:16 | sean-k-mooney | dansmith: with that context then yes im much more ok with your suggeed approch | |
| 14:28:51 | dansmith | cool | |
| 14:28:56 | dansmith | jlejeune: sorry for the runaround :) | |
| 14:29:01 | jlejeune | no prob :) | |
| 14:29:05 | jlejeune | let's do that ! | |
| 15:12:53 | opendevreview | Julien LE JEUNE proposed openstack/nova master: Skip snapshot test when missing qemu-img https://review.opendev.org/c/openstack/nova/+/925208 | |
| 15:14:08 | jlejeune | dansmith: sean-k-mooney: ^ | |
| 15:14:48 | jlejeune | something like that ? | |
| 15:18:03 | dansmith | sure, but that will fail pep8 | |
| 15:18:37 | jlejeune | my bad | |
| 15:18:42 | dansmith | but looks okay to me otherwise | |
| 15:26:07 | opendevreview | Julien LE JEUNE proposed openstack/nova master: Skip snapshot test when missing qemu-img https://review.opendev.org/c/openstack/nova/+/925208 | |
| 15:33:14 | dansmith | sean-k-mooney: I think you commented about using backslash continuations recently on another patch, not sure why you didn't on this one, but I also can't find out guideline about that | |
| 15:33:55 | sean-k-mooney | ya i personally hate them but we are not as concistent about not doing it as i would like | |
| 15:33:55 | dansmith | I know we have them in some places and I'm not sure I care about our previous hard and fast rule about it, other than just wanting to be consistent | |
| 15:34:15 | sean-k-mooney | and im presonally geting very tired of style related review comments | |
| 15:34:21 | dansmith | ah right here, second bullet: https://docs.openstack.org/hacking/latest/user/hacking.html | |
| 15:34:48 | sean-k-mooney | yep its documetned but not enforced by tools | |
| 15:34:51 | dansmith | jlejeune: ^ | |
| 15:34:52 | dansmith | right | |
| 16:02:13 | opendevreview | Julien LE JEUNE proposed openstack/nova master: Skip snapshot test when missing qemu-img https://review.opendev.org/c/openstack/nova/+/925208 | |
| 16:05:50 | erlon | Are we having the nova meeting today? | |
| 16:06:01 | gibi | erlon: no, it is cancelled | |
| 16:06:10 | gibi | bauzas is off today | |
| 16:06:17 | erlon | ah, ok, missed the announcement | |
| 16:06:51 | sean-k-mooney | sylvain is at the opensatck asia summit event in korea currently | |
| 16:07:19 | sean-k-mooney | erlon: did you have a pressing topic ot bring up? | |
| 16:07:25 | gibi | yeah that is the more precise term instead of being off :) | |