| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-03 | |||
| 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 | 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: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: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 :) | |
| 16:07:36 | erlon | I was to bring the shared group to attention again | |
| 16:07:55 | erlon | we have addressed the last reviews suggestions | |
| 16:07:58 | sean-k-mooney | shared security groups or somethign else? | |
| 16:08:15 | erlon | shared security groups | |
| 16:08:20 | erlon | https://review.opendev.org/c/openstack/nova/+/811521 | |
| 16:08:30 | erlon | from this series: https://review.opendev.org/q/topic:sg-shared-filter | |
| 16:08:34 | sean-k-mooney | ah well for that i think we will liikely jsut reapprove that for next cycle | |
| 16:08:59 | sean-k-mooney | so once RC 1 is creeated in 2 weeks master will reopen for new work and it can proceed | |
| 16:10:28 | erlon | ok, sounds good | |
| 16:10:53 | erlon | is there a way to flag it like that so the team can approve it next? | |
| 16:10:58 | erlon | I | |
| 16:12:04 | sean-k-mooney | put it in the team adjenta for the next meeting and you can link to this irc converstiaon if you like | |
| 16:12:37 | sean-k-mooney | so add it to open disucsion here https://wiki.openstack.org/wiki/Meetings/Nova#Weekly_Nova_team_meeting | |
| 16:13:54 | sean-k-mooney | erlon: skiming the updated version it looks ok but i will need to review it properly later. it looks like you have simplifed it a little since the last version which is nice to see | |
| 16:14:21 | haleyb | sean-k-mooney: i tried to address all the comments | |
| 16:14:51 | sean-k-mooney | haleyb: sorry you are the most recent uploader :) yes i think you have | |
| 16:15:22 | haleyb | i just wanted to move that along, np | |
| 16:15:22 | sean-k-mooney | i basically am getting a headache right now which is why im holding of reviewing it proeprly | |
| 16:15:38 | haleyb | hope you feel better | |
| 16:16:13 | sean-k-mooney | i think its just a lack of drinking water so im goign to go remiday that | |
| 17:22:14 | opendevreview | Merged openstack/nova stable/2024.1: libvirt: call get_capabilities() with all CPUs online https://review.opendev.org/c/openstack/nova/+/927556 | |
| 17:41:26 | opendevreview | Merged openstack/nova stable/2024.1: Fix port group network metadata generation https://review.opendev.org/c/openstack/nova/+/923689 | |
| 17:41:35 | opendevreview | Merged openstack/nova master: Skip snapshot test when missing qemu-img https://review.opendev.org/c/openstack/nova/+/925208 | |
| 19:07:12 | gmann | sean-k-mooney: if you are around https://review.opendev.org/c/openstack/placement/+/926935 | |
| 19:27:21 | sean-k-mooney | +2w | |
| 19:41:04 | gmann | thanks | |
| 20:54:12 | opendevreview | Brian Haley proposed openstack/nova master: Support creating servers with RBAC SGs https://review.opendev.org/c/openstack/nova/+/811521 | |