Wednesday, 2022-12-07

opendevreviewmelanie witt proposed openstack/nova stable/train: Fix the wrong exception used to retry detach API calls  https://review.opendev.org/c/openstack/nova/+/86609001:09
opendevreviewmelanie witt proposed openstack/nova stable/train: Retry attachment delete API call for 504 Gateway Timeout  https://review.opendev.org/c/openstack/nova/+/86609101:09
*** akekane is now known as abhishekk04:48
opendevreviewHiroki Narukawa proposed openstack/nova master: libvirt: add sftp driver  https://review.opendev.org/c/openstack/nova/+/86667204:57
opendevreviewHiroki Narukawa proposed openstack/nova master: libvirt: add sftp driver  https://review.opendev.org/c/openstack/nova/+/86667207:24
opendevreviewBalazs Gibizer proposed openstack/nova master: Support multiple config file with mod_wsgi  https://review.opendev.org/c/openstack/nova/+/86401410:24
gibibauzas: ^^ that is an easy fix10:24
bauzasgibi: ack, looking10:24
gibistephenfin: 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
gibisahid_: go for it! and thank you :)10:51
gibisahid_: please note that we landed the inventory reporting and allocation healing in zed, the patches open now is for the scheduling support10:51
gibisahid_: and also this is only covering PCI devices requested via the pci alias in the flavor. The neutron SRIOV support is planned for later10:51
sahid_ack thank you for the heads-up I noticed that on the first path some of them get landed during zed10:56
sean-k-mooneysahid_: the feature has basiclaly been code compelte for a while it just hit m3 so got pushed to A11:33
sean-k-mooneysahid_: 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 PTO11:34
opendevreviewBalazs Gibizer proposed openstack/nova master: Support multiple config file with mod_wsgi  https://review.opendev.org/c/openstack/nova/+/86401411:36
sahid_sean-k-mooney: ack i understand your point i guess11: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/FIXME11:56
* sahid_ hopes that he did not made the whole community angry against him :-)11:58
sean-k-mooneyyes i dislike having bare NOTE/FIXME without the name11:58
sean-k-mooneyit has come up before that we coudl drop the (name) portion11:59
sean-k-mooneyand some project have 11:59
sean-k-mooneywe have a hacking check that enforces it to keep nova consitent11:59
sahid_I would drop the whole NOTE thingm with name or not11:59
sean-k-mooneyand just use bare comments12:00
sahid_yes12:00
sean-k-mooneywe could but because we cant eaially use git blame on some of our files12:00
sahid_instead of for TODO(name) or FIXME(name), which make sense has there is an action behind it12:00
sean-k-mooneyi kind of like having the name so i know who to ask about the note12:00
sean-k-mooneysahid_: well the name is not ment to represent who is going to work on it12:01
sean-k-mooneyfor TODO() and FIXME()12:01
sahid_yes i would have use git blame, but if we can't I understand12:01
sean-k-mooneyso the libvirt driver and compute manager often time out on github12:01
sean-k-mooneyso while i coudl do blame locally it does not work well for github/gitia12:02
*** d34dh0r5| is now known as d34dh0r5312:05
sahid_BTW gibi, very impressive work12:06
gibisahid_: thanks, it was fun to get through it. I learned many things about our PCI codepath during that12:09
sahid_I imagine :-)12:09
sean-k-mooneygibi: im sure you now have the scares on your soul to prove it too :P12:13
sean-k-mooneygibi: with that said its both better and worse then it appears at first glance12:14
sean-k-mooneyonce 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-mooneybut there is a lot of other questionable choices that you ahve rectifed along the way12:15
gibisean-k-mooney: I agree with you I think the overal desing is OK, it just has a step learning curve13:22
sean-k-mooneyits proably comperable to emac/vims 13:36
sean-k-mooneywhich is not something to strive for13:36
*** akekane is now known as abhishekk13:56
*** whoami-rajat__ is now known as whoami-rajat14:00
*** dasm|off is now known as dasm14:43
sahido/ 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/+/85783815:40
opendevreviewKonrad Gube proposed openstack/nova-specs master: Use extend volume migration  https://review.opendev.org/c/openstack/nova-specs/+/85549016:29
opendevreviewKonrad Gube proposed openstack/nova-specs master: Use extend volume completion action  https://review.opendev.org/c/openstack/nova-specs/+/85549016:30
gibibauzas: zuul is happy now on https://review.opendev.org/c/openstack/nova/+/86401416:47
bauzasgibi: sent to the gate16:47
gibibauzas: thansk!~16:47
gibithanks 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/+/85549016:48
sean-k-mooneythansk i should have time to review it tomorrow17:00
melwittbauzas: fyi auniyal has joined the bug triage rotation and put his name on the roster to help out17:26
bauzasall cool17:26
gmannbauzas: 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/+/86146917:33
gmannand adding focal job in nova, the first link17:33
sean-k-mooneymelwitt: nice find https://review.opendev.org/c/openstack/nova/+/866090/1/nova/volume/cinder.py#57817:43
melwittsean-k-mooney: thanks for looking :)17:45
sean-k-mooneymelwitt: for api calls in functinal test we should be intersepting the calls in the cinder fixutre17:45
sean-k-mooneyso yes the get shoudl either be mocked in the fixutre or via requests later17:45
sean-k-mooneyits currently calling https://review.opendev.org/c/openstack/nova/+/866090/1/nova/volume/cinder.py#491 right17:46
melwittsean-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 happens17:47
melwittyes that's right17:48
sean-k-mooneywhat i dont really understand is why is this failing on python 27 17:48
sean-k-mooneybut not py317:48
melwittme neither. I googled a lot too and didn't find anything useful. you might have better luck17:48
sean-k-mooneydid the signiture of somethign change17:48
melwittI 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 understand17:49
sean-k-mooneydo you know where that deep copy happens17:50
sean-k-mooneyi assume one of the fixtures right17:50
melwittyes it's in _untranslate_volume_summary_view17:50
melwittin nova/volume/cinder.py17:50
melwittno17:50
sean-k-mooneyoh right17:51
sean-k-mooneyi see and calling deepcopy on MagicMock is causing issue17:51
melwittyeah. but there are other tests that have the same thing afaict and don't fail. I don't get it17:51
melwitt*other tests in the same test file17:52
melwittit's just this one test17:52
sean-k-mooneyits this depcopy ?     d['volume_image_metadata'] = copy.deepcopy(vol.volume_image_metadata)17:52
melwittyes17:53
sean-k-mooneyhttps://github.com/openstack/nova/blob/90c0c687a487601e009c72f60c88be92f6a55264/nova/volume/cinder.py#L31917:53
melwittthat's the one17:53
sean-k-mooneywhich was added 10 years ago https://github.com/openstack/nova/commit/fb32f1ed9be3e4f2f46d5aea405c62ef2139764017:54
sean-k-mooneyi think that maybe we have a self erference in the MagicMock in this case17:56
sean-k-mooneyliek perhaps the one you get form an exception traceback on python2.7 that does not happen in python317:56
sean-k-mooneythe deepcopy seams to be getting stuck in a loop right17:57
sean-k-mooneyif i rememerb the error or was that not the issue17:57
melwittyes17:57
melwittit looks like a loop17:57
melwittbut it only loops a handful of times before it fails on something's __init__17:58
sean-k-mooneywell that might not be a loop exactly17:59
sean-k-mooneyi mean it might be looping over the filed or it could be recusing17:59
sean-k-mooneybut ya it fails on an __init__ call at some point17:59
melwitthere'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.html17:59
melwittyeah I would think it's recursing18:00
sean-k-mooney so i am wondering if we just need to set volume_image_metadata to something18:00
melwittthat might do it. I was trying to pick the "most right" way to address it. I guessed mocking get()18:01
sean-k-mooneyya i was wondering about get or _untranslate_volume_summary_view18:03
sean-k-mooneybut also wondering if we can do something in the fixture to not have it be a magic mock18:04
sean-k-mooneylike initalise the field to a {}18:04
sean-k-mooneymelwitt: https://github.com/openstack/nova/commit/22e9d22369d34e150855eb1710b371e80d17ebb018:05
sean-k-mooneythats in yoga 18:06
sean-k-mooneyactully might need to go a bit futher back18:07
sean-k-mooneybut the volume_image_metadata is not empty there18:07
sean-k-mooneymelwitt: train is before stephen split out the test fixutres into there own module18:10
sean-k-mooneyso im wonderign are you just missign something that there on xena18:11
sean-k-mooneyor in ussuri sorry18:11
sean-k-mooneynames hard18:11
melwittI dunno, I don't see that CinderFixture is used anywhere for these tests?18:12
melwitteven on master18:12
sean-k-mooneyactully18:13
sean-k-mooneyagain i keep forgeting this works on python 318:13
sean-k-mooneyso likely 1 this is just not mocked properly 18:13
sean-k-mooneyand 2 the deepcopy and majicmock issue was proably fixed in python 318:14
melwittit even works in other tests with that path in the same file on python 218:14
sean-k-mooneyya18:15
sean-k-mooneyalthough those were calling idffernt methods like terminate right18:15
sean-k-mooneyor was it working in other cases wehre there was a deepcopy18:15
melwittdouble checking..18:16
sean-k-mooneymelwitt: in anycase yes i think treaking this on train is vaild.18:16
melwittsean-k-mooney: like this one, it doesn't fail https://github.com/openstack/nova/blob/master/nova/tests/unit/volume/test_cinder.py#L71718:16
sean-k-mooneywe might also want to fix this on master but not really sure how to go about that18:16
melwittand it's pretty much exactly the same18:16
sean-k-mooney but it does not raise18:17
melwittit raises the first call18:17
sean-k-mooneyand as i said im wondering if this is the issue with excptions have a circurlar dep18:17
sean-k-mooneyright but it does not return the excption18:17
sean-k-mooneyare we storign the excption into the magicmock when it does raise18:18
melwittno, ok18:18
melwittwhat about this one https://github.com/openstack/nova/blob/master/nova/tests/unit/volume/test_cinder.py#L73118: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 it18:20
melwittohhh ok, I see what you're saying18:21
melwittok, so it is "unique"18:21
sean-k-mooneybut it caught here i think https://github.com/openstack/nova/blob/85c954444493199c6edb01d9bdaa07fd9cf6d729/nova/virt/block_device.py#L52018:21
sean-k-mooneyactully not 400 is bad request not found18:22
sean-k-mooneywell the test that is failing is test_detach_internal_server_error18:22
melwittyeah18:23
melwittand it's the only one that raises after a retrying, like you pointed out18:23
sean-k-mooneywhich you did not modify  https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py#70618:23
sean-k-mooneyya its a 500 so it will hit the retry decorator18:23
sean-k-mooneyalthough it did that before too18:24
melwittyeah.. exactly18:24
melwittthe 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-mooneythis is where you did that chagne 18:30
sean-k-mooneyhttps://review.opendev.org/c/openstack/nova/+/866090/2/nova/tests/unit/volume/test_cinder.py#67218:30
sean-k-mooneywell where that change was made18:30
sean-k-mooneyi dont know if you wrote this patch orginailly18:30
sean-k-mooneyno it was takashi18:30
sean-k-mooneyoh sorry18:31
melwittyeah. and yeah L672 is the thing I added as a one-off to make this pass18:31
sean-k-mooneyso that is where you added the mock18:31
melwittyes18:31
sean-k-mooney@mock.patch('nova.volume.cinder.API.get', new=mock.MagicMock())18:31
sean-k-mooneyand have you just not rebased the second patch where it failing18:31
sean-k-mooneycause i did not see the mock there18:31
melwittit should be there ... looking18:32
sean-k-mooneyoh sorry it is18:32
sean-k-mooneybut its havving issue with deepcopy18:32
melwittit's still there https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py#70418:32
sean-k-mooneyya18:33
sean-k-mooneythis is really strange18:33
melwittagreed18:34
sean-k-mooneyso this https://review.opendev.org/c/openstack/nova/+/866091/2/nova/volume/cinder.py is the only code change that could affect that test18:35
sean-k-mooneyadding extra tests can possibel cause the exsting on to fail unless...18:36
sean-k-mooneycan you put a patch on top that remove the extra tests18:36
sean-k-mooneybasically revert https://review.opendev.org/c/openstack/nova/+/866091/2/nova/tests/unit/volume/test_cinder.py18:37
sean-k-mooneyi really dont think we are leaking any state18:37
melwittsean-k-mooney: but it's the bottom patch that was failing, it's where this started. before any tests were added18:40
sean-k-mooneywell the bottom patch now work with the addtion of the mock right18:48
sean-k-mooneybut then the top patch fails18:48
sean-k-mooneyon the test you added the mock too18:48
sean-k-mooneyHhttps://review.opendev.org/c/openstack/nova/+/866090/1..2/nova/tests/unit/volume/test_cinder.py18:49
sean-k-mooneythat is the only code change between v1 and v2 and it worked18:49
sean-k-mooneyand then that exact same test fails on teh next patch18:49
sean-k-mooneyoh wait18:50
sean-k-mooneyid didnt18:50
sean-k-mooneysorry i tought the -v on https://review.opendev.org/c/openstack/nova/+/866091/218:51
sean-k-mooneywas for the unit test failure18:51
sean-k-mooneyits not its form tempest-slow-py318:51
* sean-k-mooney is not with it today18:51
sean-k-mooneymelwitt: then  yes i think that single mock is fine18:51
melwittoh, yeah. stable/train is never with it so its CI fails half the time :P18:52
sean-k-mooneytest_volume_swap failed in the slow job on the second patch18:52
sean-k-mooneyDetails: volume 29450c9c-2303-46b1-a3c7-46c22abd2a90 failed to reach available status (current in-use) within the required time (196 s).18:53
sean-k-mooneyi dont think that is related to your patch18:53
sean-k-mooneyso i guess im +1 on both18:53
sean-k-mooney+1 becasue we have not merged this on the newwer branches18:54
melwittyeah18:54
melwittok, cool, thanks18:54
sean-k-mooneysoory it took so long to get to that point18:57
melwittthat's ok :D18:57
sean-k-mooneygiven 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 food18:58
melwittI really wanted to know why it's failing too, like why 2.7 only18:58
sean-k-mooneyya its odd18:58
sean-k-mooneyi wonder if it was just flaky18:59
sean-k-mooneylike woudl it alwasy fail18:59
melwittI ran it locally a bunch and it was very consistent18:59
sean-k-mooneyweird18:59
melwittI didn't run it in a long running loop but just while I was messing with it I ran it several times18:59
melwittif I left it running in a loop overnight, maybe it would pass at some point 😂19:00
sean-k-mooneythats a lower pass rate then is desireable in ci19:06
sean-k-mooneyso i think we are good with your change19:06
melwitta little 19:06
*** blarnath is now known as d34dh0r5321:23
*** dasm is now known as dasm|off22:11

Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!