| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-12-07 | |||
| 18:00:30 | sean-k-mooney | so i am wondering if we just need to set volume_image_metadata to something | |
| 18:01:59 | melwitt | that might do it. I was trying to pick the "most right" way to address it. I guessed mocking get() | |
| 18:03:49 | sean-k-mooney | ya i was wondering about get or _untranslate_volume_summary_view | |
| 18:04:12 | sean-k-mooney | but also wondering if we can do something in the fixture to not have it be a magic mock | |
| 18:04:21 | sean-k-mooney | like initalise the field to a {} | |
| 18:05:56 | sean-k-mooney | melwitt: https://github.com/openstack/nova/commit/22e9d22369d34e150855eb1710b371e80d17ebb0 | |
| 18:06:47 | sean-k-mooney | thats in yoga | |
| 18:07:15 | sean-k-mooney | actully might need to go a bit futher back | |
| 18:07:28 | sean-k-mooney | but the volume_image_metadata is not empty there | |
| 18:10:34 | sean-k-mooney | melwitt: train is before stephen split out the test fixutres into there own module | |
| 18:11:02 | sean-k-mooney | so im wonderign are you just missign something that there on xena | |
| 18:11:47 | sean-k-mooney | or in ussuri sorry | |
| 18:11:56 | sean-k-mooney | names hard | |
| 18:12:32 | melwitt | I dunno, I don't see that CinderFixture is used anywhere for these tests? | |
| 18:12:52 | melwitt | even on master | |
| 18:13:23 | sean-k-mooney | actully | |
| 18:13:34 | sean-k-mooney | again i keep forgeting this works on python 3 | |
| 18:13:49 | sean-k-mooney | so likely 1 this is just not mocked properly | |
| 18:14:05 | sean-k-mooney | and 2 the deepcopy and majicmock issue was proably fixed in python 3 | |
| 18:14:41 | melwitt | it even works in other tests with that path in the same file on python 2 | |
| 18:15:09 | sean-k-mooney | ya | |
| 18:15:20 | sean-k-mooney | although those were calling idffernt methods like terminate right | |
| 18:15:39 | sean-k-mooney | or was it working in other cases wehre there was a deepcopy | |
| 18:16:01 | melwitt | double checking.. | |
| 18:16:09 | sean-k-mooney | melwitt: in anycase yes i think treaking this on train is vaild. | |
| 18:16:43 | melwitt | sean-k-mooney: like this one, it doesn't fail https://github.com/openstack/nova/blob/master/nova/tests/unit/volume/test_cinder.py#L717 | |
| 18:16:45 | sean-k-mooney | we might also want to fix this on master but not really sure how to go about that | |
| 18:16:51 | melwitt | and it's pretty much exactly the same | |
| 18:17:02 | sean-k-mooney | but it does not raise | |
| 18:17:27 | melwitt | it raises the first call | |
| 18:17:30 | sean-k-mooney | and as i said im wondering if this is the issue with excptions have a circurlar dep | |
| 18:17:43 | sean-k-mooney | right but it does not return the excption | |
| 18:18:02 | sean-k-mooney | are we storign the excption into the magicmock when it does raise | |
| 18:18:04 | melwitt | no, ok | |
| 18:18:26 | melwitt | what about this one https://github.com/openstack/nova/blob/master/nova/tests/unit/volume/test_cinder.py#L731 | |
| 18:20:46 | sean-k-mooney | ok that a 400 so https://review.opendev.org/c/openstack/nova/+/866091/2/nova/volume/cinder.py is not going to discard it | |
| 18:21:23 | melwitt | ohhh ok, I see what you're saying | |
| 18:21:32 | melwitt | ok, so it is "unique" | |
| 18:21:48 | sean-k-mooney | but it caught here i think https://github.com/openstack/nova/blob/85c954444493199c6edb01d9bdaa07fd9cf6d729/nova/virt/block_device.py#L520 | |
| 18:22:00 | sean-k-mooney | actully not 400 is bad request not found | |
| 18:22:47 | sean-k-mooney | well the test that is failing is test_detach_internal_server_error | |
| 18:23:07 | melwitt | yeah | |
| 18:23:34 | melwitt | and it's the only one that raises after a retrying, like you pointed out | |
| 18:23:49 | sean-k-mooney | which you did not modify https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py#706 | |
| 18:23:58 | sean-k-mooney | ya its a 500 so it will hit the retry decorator | |
| 18:24:21 | sean-k-mooney | although it did that before too | |
| 18:24:26 | melwitt | yeah.. exactly | |
| 18:25:58 | melwitt | the change from type(e) == cinder_apiclient.exceptions.InternalServerError to (isinstance(e, cinder_exception.ClientException) and e.code == 500)) makes it fail (in the patch below is where it started failing) | |
| 18:30:16 | sean-k-mooney | this is where you did that chagne | |
| 18:30:19 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/866090/2/nova/tests/unit/volume/test_cinder.py#672 | |
| 18:30:31 | sean-k-mooney | well where that change was made | |
| 18:30:39 | sean-k-mooney | i dont know if you wrote this patch orginailly | |
| 18:30:59 | sean-k-mooney | no it was takashi | |
| 18:31:25 | sean-k-mooney | oh sorry | |
| 18:31:29 | melwitt | yeah. and yeah L672 is the thing I added as a one-off to make this pass | |
| 18:31:33 | sean-k-mooney | so that is where you added the mock | |
| 18:31:38 | melwitt | yes | |
| 18:31:39 | sean-k-mooney | @mock.patch('nova.volume.cinder.API.get', new=mock.MagicMock()) | |
| 18:31:49 | sean-k-mooney | and have you just not rebased the second patch where it failing | |
| 18:31:56 | sean-k-mooney | cause i did not see the mock there | |
| 18:32:16 | melwitt | it should be there ... looking | |
| 18:32:32 | sean-k-mooney | oh sorry it is | |
| 18:32:50 | sean-k-mooney | but its havving issue with deepcopy | |
| 18:32:54 | melwitt | it's still there https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py#704 | |
| 18:33:08 | sean-k-mooney | ya | |
| 18:33:49 | sean-k-mooney | this is really strange | |
| 18:34:36 | melwitt | agreed | |
| 18:35:01 | sean-k-mooney | so this https://review.opendev.org/c/openstack/nova/+/866091/2/nova/volume/cinder.py is the only code change that could affect that test | |
| 18:36:10 | sean-k-mooney | adding extra tests can possibel cause the exsting on to fail unless... | |
| 18:36:39 | sean-k-mooney | can you put a patch on top that remove the extra tests | |
| 18:37:01 | sean-k-mooney | basically revert https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py | |
| 18:37:18 | sean-k-mooney | i really dont think we are leaking any state | |
| 18:40:39 | melwitt | sean-k-mooney: but it's the bottom patch that was failing, it's where this started. before any tests were added | |
| 18:48:08 | sean-k-mooney | well the bottom patch now work with the addtion of the mock right | |
| 18:48:18 | sean-k-mooney | but then the top patch fails | |
| 18:48:27 | sean-k-mooney | on the test you added the mock too | |
| 18:49:05 | sean-k-mooney | Hhttps://review.opendev.org/c/openstack/nova/+/866090/1..2/nova/tests/unit/volume/test_cinder.py | |
| 18:49:25 | sean-k-mooney | that is the only code change between v1 and v2 and it worked | |
| 18:49:47 | sean-k-mooney | and then that exact same test fails on teh next patch | |
| 18:50:47 | sean-k-mooney | oh wait | |
| 18:50:50 | sean-k-mooney | id didnt | |
| 18:51:01 | sean-k-mooney | sorry i tought the -v on https://review.opendev.org/c/openstack/nova/+/866091/2 | |
| 18:51:07 | sean-k-mooney | was for the unit test failure | |
| 18:51:23 | sean-k-mooney | its not its form tempest-slow-py3 | |
| 18:51:58 | sean-k-mooney | melwitt: then yes i think that single mock is fine | |
| 18:52:03 | melwitt | oh, yeah. stable/train is never with it so its CI fails half the time :P | |
| 18:52:47 | sean-k-mooney | test_volume_swap failed in the slow job on the second patch | |
| 18:53:17 | sean-k-mooney | Details: volume 29450c9c-2303-46b1-a3c7-46c22abd2a90 failed to reach available status (current in-use) within the required time (196 s). | |
| 18:53:40 | sean-k-mooney | i dont think that is related to your patch | |
| 18:53:51 | sean-k-mooney | so i guess im +1 on both | |
| 18:54:11 | sean-k-mooney | +1 becasue we have not merged this on the newwer branches | |
| 18:54:28 | melwitt | yeah | |
| 18:54:31 | melwitt | ok, cool, thanks | |
| 18:57:40 | sean-k-mooney | soory it took so long to get to that point | |
| 18:57:55 | melwitt | that's ok :D | |
| 18:58:34 | sean-k-mooney | given its not 7 here its proably a sign that my brain has finshed for today so im going to follw its lead and go have food | |
| 18:58:40 | melwitt | I really wanted to know why it's failing too, like why 2.7 only | |
| 18:58:51 | sean-k-mooney | ya its odd | |
| 18:59:04 | sean-k-mooney | i wonder if it was just flaky | |
| 18:59:22 | sean-k-mooney | like woudl it alwasy fail | |