| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-12-07 | |||
| 12:00:04 | sean-k-mooney | and just use bare comments | |
| 12:00:09 | sahid_ | yes | |
| 12:00:34 | sean-k-mooney | we could but because we cant eaially use git blame on some of our files | |
| 12:00:47 | sahid_ | instead of for TODO(name) or FIXME(name), which make sense has there is an action behind it | |
| 12:00:50 | sean-k-mooney | i kind of like having the name so i know who to ask about the note | |
| 12:01:10 | sean-k-mooney | sahid_: well the name is not ment to represent who is going to work on it | |
| 12:01:18 | sean-k-mooney | for TODO() and FIXME() | |
| 12:01:18 | sahid_ | yes i would have use git blame, but if we can't I understand | |
| 12:01:39 | sean-k-mooney | so the libvirt driver and compute manager often time out on github | |
| 12:02:06 | sean-k-mooney | so while i coudl do blame locally it does not work well for github/gitia | |
| 12:06:45 | sahid_ | BTW gibi, very impressive work | |
| 12:09:05 | gibi | sahid_: thanks, it was fun to get through it. I learned many things about our PCI codepath during that | |
| 12:09:32 | sahid_ | I imagine :-) | |
| 12:13:57 | sean-k-mooney | gibi: im sure you now have the scares on your soul to prove it too :P | |
| 12:14:17 | sean-k-mooney | gibi: with that said its both better and worse then it appears at first glance | |
| 12:14:48 | sean-k-mooney | once you wrap your head aroudn teh design its elegant in a way in how the virt driver bits are entirly abstracted | |
| 12:15:10 | sean-k-mooney | but there is a lot of other questionable choices that you ahve rectifed along the way | |
| 13:22:06 | gibi | sean-k-mooney: I agree with you I think the overal desing is OK, it just has a step learning curve | |
| 13:36:47 | sean-k-mooney | its proably comperable to emac/vims | |
| 13:36:56 | sean-k-mooney | which is not something to strive for | |
| 15:40:47 | sahid | o/ bauzas do you think we could discuss whehter the impl can get the Priority-Review bit as the spec received it? https://review.opendev.org/c/openstack/nova-specs/+/857838 | |
| 16:29:11 | opendevreview | Konrad Gube proposed openstack/nova-specs master: Use extend volume migration https://review.opendev.org/c/openstack/nova-specs/+/855490 | |
| 16:30:09 | opendevreview | Konrad Gube proposed openstack/nova-specs master: Use extend volume completion action https://review.opendev.org/c/openstack/nova-specs/+/855490 | |
| 16:47:09 | gibi | bauzas: zuul is happy now on https://review.opendev.org/c/openstack/nova/+/864014 | |
| 16:47:49 | bauzas | gibi: sent to the gate | |
| 16:47:54 | gibi | bauzas: thansk!~ | |
| 16:47:58 | gibi | thanks even :) | |
| 16:48:55 | kgube_ | sean-k-mooney, I rewrote the spec to reflect the current proposal in Cinder: https://review.opendev.org/c/openstack/nova-specs/+/855490 | |
| 17:00:35 | sean-k-mooney | thansk i should have time to review it tomorrow | |
| 17:26:21 | melwitt | bauzas: fyi auniyal has joined the bug triage rotation and put his name on the roster to help out | |
| 17:26:44 | bauzas | all cool | |
| 17:33:01 | gmann | bauzas: can you please check the 2023.1 testing runtime updates changes (few are trivial: update python classifier) https://review.opendev.org/c/openstack/nova/+/861111 https://review.opendev.org/c/openstack/osc-placement/+/861470 https://review.opendev.org/c/openstack/placement/+/861471 https://review.opendev.org/c/openstack/os-traits/+/861466 https://review.opendev.org/c/openstack/python-novaclient/+/861469 | |
| 17:33:14 | gmann | and adding focal job in nova, the first link | |
| 17:43:30 | sean-k-mooney | melwitt: nice find https://review.opendev.org/c/openstack/nova/+/866090/1/nova/volume/cinder.py#578 | |
| 17:45:01 | melwitt | sean-k-mooney: thanks for looking :) | |
| 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 | |