| opendevreview | melanie witt proposed openstack/nova stable/train: Fix the wrong exception used to retry detach API calls https://review.opendev.org/c/openstack/nova/+/866090 | 01:09 |
|---|---|---|
| opendevreview | melanie witt proposed openstack/nova stable/train: Retry attachment delete API call for 504 Gateway Timeout https://review.opendev.org/c/openstack/nova/+/866091 | 01:09 |
| *** akekane is now known as abhishekk | 04:48 | |
| opendevreview | Hiroki Narukawa proposed openstack/nova master: libvirt: add sftp driver https://review.opendev.org/c/openstack/nova/+/866672 | 04:57 |
| opendevreview | Hiroki Narukawa proposed openstack/nova master: libvirt: add sftp driver https://review.opendev.org/c/openstack/nova/+/866672 | 07:24 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Support multiple config file with mod_wsgi https://review.opendev.org/c/openstack/nova/+/864014 | 10:24 |
| gibi | bauzas: ^^ that is an easy fix | 10:24 |
| bauzas | gibi: ack, looking | 10:24 |
| gibi | stephenfin: a friendly poke about the pci in placement series. If you have time, review would be appreciated. bottom is: https://review.opendev.org/c/openstack/nova/+/852771 | 10:26 |
| sahid_ | gibi: o/ i will review it as well if you don't mind :-) | 10:50 |
| gibi | sahid_: go for it! and thank you :) | 10:51 |
| gibi | sahid_: please note that we landed the inventory reporting and allocation healing in zed, the patches open now is for the scheduling support | 10:51 |
| gibi | sahid_: and also this is only covering PCI devices requested via the pci alias in the flavor. The neutron SRIOV support is planned for later | 10:51 |
| sahid_ | ack thank you for the heads-up I noticed that on the first path some of them get landed during zed | 10:56 |
| sean-k-mooney | sahid_: the feature has basiclaly been code compelte for a while it just hit m3 so got pushed to A | 11:33 |
| sean-k-mooney | sahid_: our orginal goal was to try and land this all by m1 but time going away form us so we are trying to make an effort to finaly get this merged before people start disaparing for PTO | 11:34 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Support multiple config file with mod_wsgi https://review.opendev.org/c/openstack/nova/+/864014 | 11:36 |
| sahid_ | sean-k-mooney: ack i understand your point i guess | 11:49 |
| sahid_ | guys I have a question regarding the usage of NOTE(name) do we really think that we should continue using it? I mean most of the time such note a just comment, no actions are required. Where I see that makes sense is for TODO/FIXME | 11:56 |
| * sahid_ hopes that he did not made the whole community angry against him :-) | 11:58 | |
| sean-k-mooney | yes i dislike having bare NOTE/FIXME without the name | 11:58 |
| sean-k-mooney | it has come up before that we coudl drop the (name) portion | 11:59 |
| sean-k-mooney | and some project have | 11:59 |
| sean-k-mooney | we have a hacking check that enforces it to keep nova consitent | 11:59 |
| sahid_ | I would drop the whole NOTE thingm with name or not | 11:59 |
| sean-k-mooney | and just use bare comments | 12:00 |
| sahid_ | yes | 12:00 |
| sean-k-mooney | we could but because we cant eaially use git blame on some of our files | 12:00 |
| sahid_ | instead of for TODO(name) or FIXME(name), which make sense has there is an action behind it | 12:00 |
| sean-k-mooney | i kind of like having the name so i know who to ask about the note | 12:00 |
| sean-k-mooney | sahid_: well the name is not ment to represent who is going to work on it | 12:01 |
| sean-k-mooney | for TODO() and FIXME() | 12:01 |
| sahid_ | yes i would have use git blame, but if we can't I understand | 12:01 |
| sean-k-mooney | so the libvirt driver and compute manager often time out on github | 12:01 |
| sean-k-mooney | so while i coudl do blame locally it does not work well for github/gitia | 12:02 |
| *** d34dh0r5| is now known as d34dh0r53 | 12:05 | |
| sahid_ | BTW gibi, very impressive work | 12:06 |
| gibi | sahid_: thanks, it was fun to get through it. I learned many things about our PCI codepath during that | 12:09 |
| sahid_ | I imagine :-) | 12:09 |
| sean-k-mooney | gibi: im sure you now have the scares on your soul to prove it too :P | 12:13 |
| sean-k-mooney | gibi: with that said its both better and worse then it appears at first glance | 12:14 |
| 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:14 |
| sean-k-mooney | but there is a lot of other questionable choices that you ahve rectifed along the way | 12:15 |
| gibi | sean-k-mooney: I agree with you I think the overal desing is OK, it just has a step learning curve | 13:22 |
| sean-k-mooney | its proably comperable to emac/vims | 13:36 |
| sean-k-mooney | which is not something to strive for | 13:36 |
| *** akekane is now known as abhishekk | 13:56 | |
| *** whoami-rajat__ is now known as whoami-rajat | 14:00 | |
| *** dasm|off is now known as dasm | 14:43 | |
| 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 | 15:40 |
| opendevreview | Konrad Gube proposed openstack/nova-specs master: Use extend volume migration https://review.opendev.org/c/openstack/nova-specs/+/855490 | 16:29 |
| opendevreview | Konrad Gube proposed openstack/nova-specs master: Use extend volume completion action https://review.opendev.org/c/openstack/nova-specs/+/855490 | 16:30 |
| gibi | bauzas: zuul is happy now on https://review.opendev.org/c/openstack/nova/+/864014 | 16:47 |
| bauzas | gibi: sent to the gate | 16:47 |
| gibi | bauzas: thansk!~ | 16:47 |
| gibi | thanks even :) | 16:47 |
| kgube_ | sean-k-mooney, I rewrote the spec to reflect the current proposal in Cinder: https://review.opendev.org/c/openstack/nova-specs/+/855490 | 16:48 |
| sean-k-mooney | thansk i should have time to review it tomorrow | 17:00 |
| melwitt | bauzas: fyi auniyal has joined the bug triage rotation and put his name on the roster to help out | 17:26 |
| bauzas | all cool | 17:26 |
| 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 |
| gmann | and adding focal job in nova, the first link | 17:33 |
| sean-k-mooney | melwitt: nice find https://review.opendev.org/c/openstack/nova/+/866090/1/nova/volume/cinder.py#578 | 17:43 |
| melwitt | sean-k-mooney: thanks for looking :) | 17:45 |
| sean-k-mooney | melwitt: for api calls in functinal test we should be intersepting the calls in the cinder fixutre | 17:45 |
| sean-k-mooney | so yes the get shoudl either be mocked in the fixutre or via requests later | 17:45 |
| sean-k-mooney | its currently calling https://review.opendev.org/c/openstack/nova/+/866090/1/nova/volume/cinder.py#491 right | 17:46 |
| 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:47 |
| melwitt | yes that's right | 17:48 |
| sean-k-mooney | what i dont really understand is why is this failing on python 27 | 17:48 |
| sean-k-mooney | but not py3 | 17:48 |
| melwitt | me neither. I googled a lot too and didn't find anything useful. you might have better luck | 17:48 |
| sean-k-mooney | did the signiture of somethign change | 17:48 |
| 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:49 |
| sean-k-mooney | do you know where that deep copy happens | 17:50 |
| sean-k-mooney | i assume one of the fixtures right | 17:50 |
| melwitt | yes it's in _untranslate_volume_summary_view | 17:50 |
| melwitt | in nova/volume/cinder.py | 17:50 |
| melwitt | no | 17:50 |
| sean-k-mooney | oh right | 17:51 |
| sean-k-mooney | i see and calling deepcopy on MagicMock is causing issue | 17:51 |
| melwitt | yeah. but there are other tests that have the same thing afaict and don't fail. I don't get it | 17:51 |
| melwitt | *other tests in the same test file | 17:52 |
| melwitt | it's just this one test | 17:52 |
| sean-k-mooney | its this depcopy ? d['volume_image_metadata'] = copy.deepcopy(vol.volume_image_metadata) | 17:52 |
| melwitt | yes | 17:53 |
| sean-k-mooney | https://github.com/openstack/nova/blob/90c0c687a487601e009c72f60c88be92f6a55264/nova/volume/cinder.py#L319 | 17:53 |
| melwitt | that's the one | 17:53 |
| sean-k-mooney | which was added 10 years ago https://github.com/openstack/nova/commit/fb32f1ed9be3e4f2f46d5aea405c62ef21397640 | 17:54 |
| sean-k-mooney | i think that maybe we have a self erference in the MagicMock in this case | 17:56 |
| sean-k-mooney | liek perhaps the one you get form an exception traceback on python2.7 that does not happen in python3 | 17:56 |
| sean-k-mooney | the deepcopy seams to be getting stuck in a loop right | 17:57 |
| sean-k-mooney | if i rememerb the error or was that not the issue | 17:57 |
| melwitt | yes | 17:57 |
| melwitt | it looks like a loop | 17:57 |
| melwitt | but it only loops a handful of times before it fails on something's __init__ | 17:58 |
| sean-k-mooney | well that might not be a loop exactly | 17:59 |
| sean-k-mooney | i mean it might be looping over the filed or it could be recusing | 17:59 |
| sean-k-mooney | but ya it fails on an __init__ call at some point | 17:59 |
| 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 | 17:59 |
| melwitt | yeah I would think it's recursing | 18:00 |
| sean-k-mooney | so i am wondering if we just need to set volume_image_metadata to something | 18:00 |
| melwitt | that might do it. I was trying to pick the "most right" way to address it. I guessed mocking get() | 18:01 |
| sean-k-mooney | ya i was wondering about get or _untranslate_volume_summary_view | 18:03 |
| sean-k-mooney | but also wondering if we can do something in the fixture to not have it be a magic mock | 18:04 |
| sean-k-mooney | like initalise the field to a {} | 18:04 |
| sean-k-mooney | melwitt: https://github.com/openstack/nova/commit/22e9d22369d34e150855eb1710b371e80d17ebb0 | 18:05 |
| sean-k-mooney | thats in yoga | 18:06 |
| sean-k-mooney | actully might need to go a bit futher back | 18:07 |
| sean-k-mooney | but the volume_image_metadata is not empty there | 18:07 |
| sean-k-mooney | melwitt: train is before stephen split out the test fixutres into there own module | 18:10 |
| sean-k-mooney | so im wonderign are you just missign something that there on xena | 18:11 |
| sean-k-mooney | or in ussuri sorry | 18:11 |
| sean-k-mooney | names hard | 18:11 |
| melwitt | I dunno, I don't see that CinderFixture is used anywhere for these tests? | 18:12 |
| melwitt | even on master | 18:12 |
| sean-k-mooney | actully | 18:13 |
| sean-k-mooney | again i keep forgeting this works on python 3 | 18:13 |
| sean-k-mooney | so likely 1 this is just not mocked properly | 18:13 |
| sean-k-mooney | and 2 the deepcopy and majicmock issue was proably fixed in python 3 | 18:14 |
| melwitt | it even works in other tests with that path in the same file on python 2 | 18:14 |
| sean-k-mooney | ya | 18:15 |
| sean-k-mooney | although those were calling idffernt methods like terminate right | 18:15 |
| sean-k-mooney | or was it working in other cases wehre there was a deepcopy | 18:15 |
| melwitt | double checking.. | 18:16 |
| sean-k-mooney | melwitt: in anycase yes i think treaking this on train is vaild. | 18:16 |
| 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 |
| sean-k-mooney | we might also want to fix this on master but not really sure how to go about that | 18:16 |
| melwitt | and it's pretty much exactly the same | 18:16 |
| sean-k-mooney | but it does not raise | 18:17 |
| melwitt | it raises the first call | 18:17 |
| sean-k-mooney | and as i said im wondering if this is the issue with excptions have a circurlar dep | 18:17 |
| sean-k-mooney | right but it does not return the excption | 18:17 |
| sean-k-mooney | are we storign the excption into the magicmock when it does raise | 18:18 |
| melwitt | no, ok | 18:18 |
| melwitt | what about this one https://github.com/openstack/nova/blob/master/nova/tests/unit/volume/test_cinder.py#L731 | 18:18 |
| 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:20 |
| melwitt | ohhh ok, I see what you're saying | 18:21 |
| melwitt | ok, so it is "unique" | 18:21 |
| sean-k-mooney | but it caught here i think https://github.com/openstack/nova/blob/85c954444493199c6edb01d9bdaa07fd9cf6d729/nova/virt/block_device.py#L520 | 18:21 |
| sean-k-mooney | actully not 400 is bad request not found | 18:22 |
| sean-k-mooney | well the test that is failing is test_detach_internal_server_error | 18:22 |
| melwitt | yeah | 18:23 |
| melwitt | and it's the only one that raises after a retrying, like you pointed out | 18:23 |
| 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 |
| sean-k-mooney | ya its a 500 so it will hit the retry decorator | 18:23 |
| sean-k-mooney | although it did that before too | 18:24 |
| melwitt | yeah.. exactly | 18:24 |
| 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:25 |
| sean-k-mooney | this is where you did that chagne | 18:30 |
| sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/866090/2/nova/tests/unit/volume/test_cinder.py#672 | 18:30 |
| sean-k-mooney | well where that change was made | 18:30 |
| sean-k-mooney | i dont know if you wrote this patch orginailly | 18:30 |
| sean-k-mooney | no it was takashi | 18:30 |
| sean-k-mooney | oh sorry | 18:31 |
| melwitt | yeah. and yeah L672 is the thing I added as a one-off to make this pass | 18:31 |
| sean-k-mooney | so that is where you added the mock | 18:31 |
| melwitt | yes | 18:31 |
| sean-k-mooney | @mock.patch('nova.volume.cinder.API.get', new=mock.MagicMock()) | 18:31 |
| sean-k-mooney | and have you just not rebased the second patch where it failing | 18:31 |
| sean-k-mooney | cause i did not see the mock there | 18:31 |
| melwitt | it should be there ... looking | 18:32 |
| sean-k-mooney | oh sorry it is | 18:32 |
| sean-k-mooney | but its havving issue with deepcopy | 18:32 |
| melwitt | it's still there https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py#704 | 18:32 |
| sean-k-mooney | ya | 18:33 |
| sean-k-mooney | this is really strange | 18:33 |
| melwitt | agreed | 18:34 |
| 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:35 |
| sean-k-mooney | adding extra tests can possibel cause the exsting on to fail unless... | 18:36 |
| sean-k-mooney | can you put a patch on top that remove the extra tests | 18:36 |
| sean-k-mooney | basically revert https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py | 18:37 |
| sean-k-mooney | i really dont think we are leaking any state | 18:37 |
| melwitt | sean-k-mooney: but it's the bottom patch that was failing, it's where this started. before any tests were added | 18:40 |
| sean-k-mooney | well the bottom patch now work with the addtion of the mock right | 18:48 |
| sean-k-mooney | but then the top patch fails | 18:48 |
| sean-k-mooney | on the test you added the mock too | 18:48 |
| sean-k-mooney | Hhttps://review.opendev.org/c/openstack/nova/+/866090/1..2/nova/tests/unit/volume/test_cinder.py | 18:49 |
| sean-k-mooney | that is the only code change between v1 and v2 and it worked | 18:49 |
| sean-k-mooney | and then that exact same test fails on teh next patch | 18:49 |
| sean-k-mooney | oh wait | 18:50 |
| sean-k-mooney | id didnt | 18:50 |
| sean-k-mooney | sorry i tought the -v on https://review.opendev.org/c/openstack/nova/+/866091/2 | 18:51 |
| sean-k-mooney | was for the unit test failure | 18:51 |
| sean-k-mooney | its not its form tempest-slow-py3 | 18:51 |
| * sean-k-mooney is not with it today | 18:51 | |
| sean-k-mooney | melwitt: then yes i think that single mock is fine | 18:51 |
| melwitt | oh, yeah. stable/train is never with it so its CI fails half the time :P | 18:52 |
| sean-k-mooney | test_volume_swap failed in the slow job on the second patch | 18:52 |
| 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 |
| sean-k-mooney | i dont think that is related to your patch | 18:53 |
| sean-k-mooney | so i guess im +1 on both | 18:53 |
| sean-k-mooney | +1 becasue we have not merged this on the newwer branches | 18:54 |
| melwitt | yeah | 18:54 |
| melwitt | ok, cool, thanks | 18:54 |
| sean-k-mooney | soory it took so long to get to that point | 18:57 |
| melwitt | that's ok :D | 18:57 |
| 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 |
| melwitt | I really wanted to know why it's failing too, like why 2.7 only | 18:58 |
| sean-k-mooney | ya its odd | 18:58 |
| sean-k-mooney | i wonder if it was just flaky | 18:59 |
| sean-k-mooney | like woudl it alwasy fail | 18:59 |
| melwitt | I ran it locally a bunch and it was very consistent | 18:59 |
| sean-k-mooney | weird | 18:59 |
| melwitt | I didn't run it in a long running loop but just while I was messing with it I ran it several times | 18:59 |
| melwitt | if I left it running in a loop overnight, maybe it would pass at some point 😂 | 19:00 |
| sean-k-mooney | thats a lower pass rate then is desireable in ci | 19:06 |
| sean-k-mooney | so i think we are good with your change | 19:06 |
| melwitt | a little | 19:06 |
| *** blarnath is now known as d34dh0r53 | 21:23 | |
| *** dasm is now known as dasm|off | 22:11 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!