| mkuroha | Hi gibi, sorry to bother you just after you got back from PTO. I’ve updated this patch based on your feedback. Please take a look and share any comments when you have time. https://review.opendev.org/c/openstack/nova/+/948304 | 02:39 |
|---|---|---|
| *** mhen_ is now known as mhen | 02:55 | |
| seyeongkim | could someone please review this patch or give me any advices about the approch? thanks so much. https://review.opendev.org/c/openstack/nova/+/963665 | 03:01 |
| *** EugenMayer4401802 is now known as EugenMayer440180 | 07:16 | |
| gibi | sambork: o/ I'm not sure you saw gmaan's post merge comments on the conductor eventlet series. https://review.opendev.org/c/openstack/nova/+/962478 I think it worth checking if there are dead code / missing cleanup based on his comments in that patch | 08:52 |
| sambork | hey gibi, thanks for pointing out this one! yeah I will check them ! | 08:55 |
| bauzas | Uggla: fancy mentioning today as implementation review day ? :) | 09:30 |
| Uggla | bauzas, you are right, I forgot about it yesterday. | 09:39 |
| bauzas | Uggla: can I use https://etherpad.opendev.org/p/nova-2026.1-status as a base for reviewing patches | 09:40 |
| bauzas | ? | 09:40 |
| Uggla | bauzas, I still not updated it (gonna do it). So unfortunately not. But you can have a look at r_taken's https://blueprints.launchpad.net/nova/+spec/generalize-sev-code patch | 09:43 |
| bauzas | thanks | 09:43 |
| bauzas | and I also said to tkajinam that I was wanting to review his patches, tkajinam any first ones I should look at ? | 09:43 |
| bauzas | Uggla: can you please set https://blueprints.launchpad.net/nova/+spec/generalize-sev-code to approved as we agreed at the PTG ? | 09:47 |
| Uggla | bauzas, yep | 09:54 |
| tkajinam | bauzas, all of my patches are in single chain so please start from https://review.opendev.org/c/openstack/nova/+/969263 (I triggered recheck because the failure in threading job looks unrelated (and has never seen before) | 11:04 |
| tkajinam | the first one is the preparation work which extends the existing functional test coverage so I believe it's not very controversial :-) | 11:05 |
| sean-k-mooney | seyeongkim: so that is a feature rather then a bug. its alos possibel to specify the boot order via teh block device mappings today | 13:45 |
| sean-k-mooney | lookign at the bug report this does not seam to be a nova bug | 13:50 |
| sean-k-mooney | the behvario depend on ght grub config and grub config is out os scope of nova/openstack | 13:50 |
| sean-k-mooney | in nova we also dont offically suprpot dual booting. | 13:51 |
| sean-k-mooney | at lest not in multipel disks like this | 13:51 |
| sean-k-mooney | we supprot a singel root disk(local or cidner volume) + additiaonl ephermal disks or cinder data volumes | 13:52 |
| sean-k-mooney | the bug report state that if both device hav ea boot_index set then it works today | 13:52 |
| sean-k-mooney | if you do not use cinder voluems for the non root disk that woudl also be lost on operations like shelve | 13:54 |
| gibi | sean-k-mooney: fyi the py312-treading job error tkajinam mentioned above in https://review.opendev.org/c/openstack/nova/+/969263 is complaining to FairLockGuard. So you might have imediate context on that issue | 16:25 |
| gibi | TypeError: Cannot enter FairLockGuard while it is already active. Create a new instance for nested usage or wait for the current context to exit. | 16:25 |
| gibi | https://zuul.opendev.org/t/openstack/build/ee184acdd25a404d872dbd294af556d2 | 16:25 |
| gmaan | gibi: sambork: more than dead code, my main concern is cleaning up the scatter-gather executer. we can discuss it in wed call. | 16:44 |
| gibi | gmaan: OK. I haven't read your comments deep enough. Indeed it is better to discuss on the tomorrows call. | 16:49 |
| gmaan | ++ | 16:49 |
| sean-k-mooney | gibi: i can take a look. its possibel that we are leakign locks between tests althoguh im not entirly sure how that woudl happen in this case. | 17:05 |
| sean-k-mooney | nova.tests.unit.test_utils.TestFairLockGuard.test_context_manager_shared_between_threads | 17:07 |
| sean-k-mooney | ok os it sthe test case that is testign the cross treadh sharing which is something i explictly did not want to supprot | 17:07 |
| sean-k-mooney | well i need to look at the test code to see what that is testing | 17:08 |
| gibi | sean-k-mooney: thanks for looking. I'm still trying to catch up after my PTO | 17:09 |
| sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/unit/test_utils.py#L1893-L1917 ya it is | 17:09 |
| sean-k-mooney | gibi: no wories did you enjoy the break | 17:10 |
| sean-k-mooney | i have not tested that in threaded mode so ill run that in a loop and see if it repoduces locally | 17:10 |
| gibi | there were parts I enjoyed very much :) | 17:10 |
| sean-k-mooney | ok ya so that test is not stable | 17:15 |
| sean-k-mooney | it cant work relyably because i expclity detect the case wehre we context switch betwen thread 1 and thread 2. https://github.com/openstack/nova/commit/22012360c40045ac1bf6dd0dc95aea3e15fed0cd#diff-d6d688e548906bcae5669480a03749c0a52f32634762e23adfbcd8de88b9d980R1236-R1240 | 17:15 |
| sean-k-mooney | biscly the exectpiotn that is beign raised happens if the first worker thread enters the context manager while the other worker thread is also in the context manger | 17:16 |
| sean-k-mooney | if the kernel runs them serially the test will pass | 17:16 |
| sean-k-mooney | if it decied to interleave them it wont | 17:16 |
| sean-k-mooney | we have 3 options. i can remove the raise which was orginally there to prevent this type of sharing | 17:17 |
| sean-k-mooney | i can remove the test | 17:17 |
| sean-k-mooney | or i can alter the test to prevent the interleving but that not very useful | 17:18 |
| sean-k-mooney | since the interation of the patch where i added that raise i added a buch of addtion lockign to make it safe to share the lockgrarud between threads | 17:18 |
| sean-k-mooney | so removing the raise is proably the correct thing to do and just make it a debug log or remove that if and the self._active varible entirly | 17:19 |
| sean-k-mooney | that raise only exists to provide a singal to use that | 17:21 |
| sean-k-mooney | thread1 = threading.Thread(target=worker, args=(1, lock_guard)) | 17:21 |
| sean-k-mooney | thread2 = threading.Thread(target=worker, args=(2, lock_guard)) | 17:21 |
| sean-k-mooney | is a programing logic error | 17:21 |
| sean-k-mooney | with the extra lockign i have added that shoudl now be safe even if i dislike it so the extra protection is not required | 17:22 |
| sean-k-mooney | any prefernce on how to fix it | 17:22 |
| sean-k-mooney | we can just say the comments are enouch to discourge that useage and delete the check, we can add a debug log or i can remove the test. | 17:23 |
| gibi | I don't know at the moment. Sorry. | 17:23 |
| gibi | I need to get back to this | 17:23 |
| sean-k-mooney | its fine ill create a quick patch to remvoe the raise and ill ask this quetion on the review | 17:23 |
| sean-k-mooney | and you can leave feedback when you have time | 17:23 |
| gibi | cool. Thanks! | 17:24 |
| sean-k-mooney | was there a bug for this if not ill just do "Related-Bug: #2048837" to tie it to the orgianal bug fix | 17:24 |
| sean-k-mooney | tkajinam: ^ | 17:25 |
| opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Add capability to load loader and nvram from xml https://review.opendev.org/c/openstack/nova/+/969086 | 17:46 |
| opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Add capability to load ssm feature from existing xml https://review.opendev.org/c/openstack/nova/+/969131 | 18:17 |
| -opendevstatus- NOTICE: An update to one of our base jobs roles broke another base job role. This update has been reverted and jobs should be working again. | 20:59 | |
| JayF | gibi: finally got back around to this post-holidays; no code change was needed and I have an explanation: https://review.opendev.org/c/openstack/nova/+/969321 | 22:59 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!