| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-12-07 | |||
| 17:45:06 | sean-k-mooney | melwitt: for api calls in functinal test we should be intersepting the calls in the cinder fixutre | |
| 17:45:56 | sean-k-mooney | so yes the get shoudl either be mocked in the fixutre or via requests later | |
| 17:46:59 | sean-k-mooney | its currently calling https://review.opendev.org/c/openstack/nova/+/866090/1/nova/volume/cinder.py#491 right | |
| 17:47:43 | melwitt | sean-k-mooney: I'm actually not sure if mocking 'nova.volume.cinder.API.attachment_get' would avoid the problem ... currently I have mocked 'nova.volume.cinder.API.get' (higher level). I can try mocking at the lower level and see what happens | |
| 17:48:12 | melwitt | yes that's right | |
| 17:48:17 | sean-k-mooney | what i dont really understand is why is this failing on python 27 | |
| 17:48:25 | sean-k-mooney | but not py3 | |
| 17:48:40 | melwitt | me neither. I googled a lot too and didn't find anything useful. you might have better luck | |
| 17:48:42 | sean-k-mooney | did the signiture of somethign change | |
| 17:49:33 | melwitt | I didn't think so... but the place it's failing is in deepcopy when it tries to "reconstruct the object" which is something I don't completely understand | |
| 17:50:11 | sean-k-mooney | do you know where that deep copy happens | |
| 17:50:25 | sean-k-mooney | i assume one of the fixtures right | |
| 17:50:31 | melwitt | yes it's in _untranslate_volume_summary_view | |
| 17:50:48 | melwitt | in nova/volume/cinder.py | |
| 17:50:50 | melwitt | no | |
| 17:51:06 | sean-k-mooney | oh right | |
| 17:51:28 | sean-k-mooney | i see and calling deepcopy on MagicMock is causing issue | |
| 17:51:50 | melwitt | yeah. but there are other tests that have the same thing afaict and don't fail. I don't get it | |
| 17:52:02 | melwitt | *other tests in the same test file | |
| 17:52:37 | melwitt | it's just this one test | |
| 17:52:53 | sean-k-mooney | its this depcopy ? d['volume_image_metadata'] = copy.deepcopy(vol.volume_image_metadata) | |
| 17:53:02 | melwitt | yes | |
| 17:53:06 | sean-k-mooney | https://github.com/openstack/nova/blob/90c0c687a487601e009c72f60c88be92f6a55264/nova/volume/cinder.py#L319 | |
| 17:53:20 | melwitt | that's the one | |
| 17:54:35 | sean-k-mooney | which was added 10 years ago https://github.com/openstack/nova/commit/fb32f1ed9be3e4f2f46d5aea405c62ef21397640 | |
| 17:56:17 | sean-k-mooney | i think that maybe we have a self erference in the MagicMock in this case | |
| 17:56:59 | sean-k-mooney | liek perhaps the one you get form an exception traceback on python2.7 that does not happen in python3 | |
| 17:57:18 | sean-k-mooney | the deepcopy seams to be getting stuck in a loop right | |
| 17:57:29 | sean-k-mooney | if i rememerb the error or was that not the issue | |
| 17:57:30 | melwitt | yes | |
| 17:57:37 | melwitt | it looks like a loop | |
| 17:58:37 | melwitt | but it only loops a handful of times before it fails on something's __init__ | |
| 17:59:04 | sean-k-mooney | well that might not be a loop exactly | |
| 17:59:17 | sean-k-mooney | i mean it might be looping over the filed or it could be recusing | |
| 17:59:29 | sean-k-mooney | but ya it fails on an __init__ call at some point | |
| 17:59:32 | melwitt | here's the trace if you want to see https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_80f/866090/1/check/openstack-tox-py27/80f58ef/testr_results.html | |
| 18:00:28 | melwitt | yeah I would think it's recursing | |
| 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 | |