| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-05-18 | |||
| 10:50:19 | sean-k-mooney | bauzas: its called directly from the api here https://github.com/openstack/nova/blob/4939318649650b60dd07d161b80909e70d0e093e/nova/api/openstack/compute/keypairs.py#L113 | |
| 10:50:35 | bauzas | yes | |
| 10:50:59 | sean-k-mooney | ok lets discuss when your back | |
| 10:51:08 | sean-k-mooney | but this looks like a trivial change to me honestly | |
| 10:51:13 | sean-k-mooney | since its all in the same process | |
| 10:51:25 | sean-k-mooney | we dont need to worry about rpc impact if we jsut add a paramater | |
| 10:51:53 | sean-k-mooney | its not remotable | |
| 10:52:54 | bauzas | the question is not about the remotable usage, but rather about the microversion needed or not | |
| 10:53:17 | sean-k-mooney | well it certenly does not need a second microversion | |
| 10:53:30 | sean-k-mooney | im not sure it need one at all since it backwards compatible | |
| 10:53:44 | bauzas | as we validate the name in the compute.api module, we need to pass some flag to it if we want a microversion for that | |
| 10:53:55 | sean-k-mooney | or we move the funciton | |
| 10:54:07 | sean-k-mooney | the only caller is in nova/api/openstack/compute/keypairs.py | |
| 10:54:08 | bauzas | we could | |
| 10:54:21 | bauzas | actually, it could be better | |
| 10:55:02 | sean-k-mooney | we could leave that to the patch review honestly im not sure it needs to be in the spec | |
| 10:55:10 | sean-k-mooney | this is an internal detail within the api process | |
| 10:55:20 | opendevreview | Sylvain Bauza proposed openstack/nova-specs master: Proposes to remove keypair generation https://review.opendev.org/c/openstack/nova-specs/+/840217 | |
| 10:55:21 | sean-k-mooney | go have lunch | |
| 10:55:31 | sean-k-mooney | we can chat when you are back | |
| 11:13:33 | gibi | sean-k-mooney: I've pushed a new PS for the tempest SSHABLE fix based on you suggestion, so we will see | |
| 11:21:36 | sean-k-mooney | i breifly looked at the tempest config and it looked liek verification was configured | |
| 11:22:14 | sean-k-mooney | so i would have expected it to trigger your chage | |
| 11:23:19 | sean-k-mooney | so notre really sure why your orgininal patch would not work other then maybe we have to wait for both | |
| 11:23:37 | opendevreview | Balazs Gibizer proposed openstack/nova master: Revert "zuul: Skip block migration with attached volumes tests due to bug #1931702" https://review.opendev.org/c/openstack/nova/+/812473 | |
| 11:24:24 | kashyap | gibi: So we're indeed skipping that | |
| 11:24:37 | kashyap | Err, it's a revert | |
| 11:24:59 | kashyap | Yep, makes sense, from reading the commit message. | |
| 11:25:21 | gibi | kashyap: I have no information why it should work now, but at least we can gather that information | |
| 11:25:36 | kashyap | Yeah | |
| 11:25:44 | gibi | it is related to another SSHABLE tempest fix in https://review.opendev.org/c/openstack/tempest/+/817772 | |
| 11:33:28 | tobias-urdin | sean-k-mooney: please have a quick look at https://review.opendev.org/c/openstack/nova/+/838976 when you have time, i need to go afk for a while but will be back later today | |
| 11:35:43 | sean-k-mooney | sure | |
| 11:36:05 | sean-k-mooney | oh this is for the nodedev name change | |
| 11:36:09 | sean-k-mooney | ya im aware of that | |
| 11:36:26 | tobias-urdin | ack :) | |
| 11:37:19 | gibi | I've just finished reviewing that | |
| 11:37:40 | gibi | I have some comments and a request for tests | |
| 11:38:39 | sean-k-mooney | just seeing them as i click | |
| 11:47:31 | sean-k-mooney | gibi: replied inline | |
| 11:51:29 | gibi | thanks | |
| 12:44:54 | opendevreview | Merged openstack/nova stable/train: [stable-only] Drop lower-constraints job https://review.opendev.org/c/openstack/nova/+/838037 | |
| 12:49:29 | opendevreview | Mohammed Naser proposed openstack/nova master: Fix race condition in _get_pci_passthrough_devices https://review.opendev.org/c/openstack/nova/+/840993 | |
| 13:07:25 | sean-k-mooney | gibi: are you working on https://bugs.launchpad.net/nova/+bug/1971760 i was going to try and find time today to push a patch to make spawn_n actully be spwan and see if that helps | |
| 13:08:31 | gibi | sean-k-mooney: I have an env where I try to reproduce the leak itself but I had no time to kick that env enough. I'm not sure I can reproduce the leak in a reproducible way | |
| 13:08:43 | gibi | so it is hard to test any changesd | |
| 13:09:21 | sean-k-mooney | i think it will hapen if a thread that is invoked with spawn_n raises an excpeiton | |
| 13:09:35 | sean-k-mooney | that is not caugt before the entry porint of the spawn_n call | |
| 13:09:43 | gibi | I tried that | |
| 13:09:48 | gibi | it does not create the leak | |
| 13:10:07 | gibi | I tried vif plug timeout | |
| 13:10:21 | sean-k-mooney | well my other tought was maybe itst related to https://bugs.launchpad.net/oslo.messaging/+bug/1949964 | |
| 13:10:42 | gibi | I havent looked at ^^ yet | |
| 13:11:24 | gibi | so feel free to propose a patch but it will be hard to prove it solved the issue except if mnaser are willing to take that patch to his env and let it running for a while | |
| 13:16:10 | sean-k-mooney | i was hoping to be able to tweak https://github.com/eventlet/eventlet/issues/731#issue-1032856809 to repoduce it but ya i just want to see if we do https://github.com/eventlet/eventlet/issues/731#issuecomment-968135262 will it help | |
| 13:16:45 | sean-k-mooney | well will it work with nova | |
| 13:17:12 | sean-k-mooney | i was considering putting it behind a workaround config option so we could get feedback | |
| 13:18:35 | gibi | yeah either we need a way to reproduce or we need to make this optional and ask for feedback | |
| 13:18:56 | gibi | overall I don't see problems replacing our spawn_n calls with spawn | |
| 13:19:04 | gibi | It should not cause any additional issue | |
| 13:19:11 | gibi | but it might not fix the current one | |
| 13:19:12 | gibi | :) | |
| 13:19:33 | sean-k-mooney | :) ya that is kind of what i was thinking too | |
| 13:20:05 | sean-k-mooney | it should not make things worse it just might not have any effect at all | |
| 13:21:51 | gibi | yepp | |
| 13:22:37 | gibi | there is a small memory / cpu overhead in case of spawn as it does wrap the greenlet into a GreenThread object but we don't have that much greenlets that it causes issues | |
| 13:23:02 | gibi | except when we start leaking them :D | |
| 13:35:46 | opendevreview | sean mooney proposed openstack/nova master: [DNM] allow mokey patching spawn_n to spawn https://review.opendev.org/c/openstack/nova/+/842359 | |
| 13:36:12 | opendevreview | sean mooney proposed openstack/nova master: [DNM] allow monkey patching spawn_n to spawn https://review.opendev.org/c/openstack/nova/+/842359 | |
| 13:36:58 | sean-k-mooney | i have not test ^ and it currently defaults to enabled | |
| 13:37:06 | sean-k-mooney | but we will see what the ci thinks | |
| 13:37:48 | sean-k-mooney | gibi shoudl i also add you greenlet state reporting | |
| 13:38:07 | gibi | you can pull the patch top of it just for data | |
| 13:38:40 | gibi | but it does not prove anything as CI is basically doing a lot of operations then stops, so there is now time for the numbers to settle to a baseline | |
| 13:39:04 | sean-k-mooney | ya but the data could be interesting to compare | |
| 13:39:18 | sean-k-mooney | shall i cherry pick your patch so? | |
| 13:39:31 | sean-k-mooney | or rebase on it | |
| 13:41:02 | gibi | just cherry pick top of yours | |
| 13:42:03 | opendevreview | sean mooney proposed openstack/nova master: DNM: log number of green(thread|let)s periodically https://review.opendev.org/c/openstack/nova/+/841040 | |
| 13:42:12 | sean-k-mooney | cool | |
| 13:42:20 | sean-k-mooney | so looking at the output of the previous run | |
| 13:42:29 | sean-k-mooney | we see both greenthreadds and greenlets | |
| 13:42:41 | sean-k-mooney | and we are not expecting to only see greenthreads with my patch | |
| 13:43:31 | gibi | yepp we expect only greenthreads and no naked greenlets | |
| 13:44:11 | gibi | greenlets are implemented in a C extension greenthreads are proper python objects implemented by eventlet | |
| 13:44:45 | sean-k-mooney | so looking at the output from the test run | |
| 13:45:00 | sean-k-mooney | teh greenthrad sayed pretty constant | |
| 13:45:13 | sean-k-mooney | but the greenlets were more or less slowly increaing over time | |
| 13:45:59 | gibi | I see same behavior locally too, but most of the time after couple of minutes of idle time the greenlets also decreased back to baseline. | |
| 13:46:04 | sean-k-mooney | https://termbin.com/dzg3 | |
| 13:46:18 | gibi | in CI there is no couple of minutes of idle time | |
| 13:46:25 | frickler | gibi: kashyap: fyi I had another fix for this in tempest recentish https://review.opendev.org/c/openstack/tempest/+/835382 | |
| 13:46:27 | gibi | but it would be interesting to see what happens there | |
| 13:46:47 | frickler | this = volume attachments | |
| 13:47:08 | sean-k-mooney | ah for tagged attachments | |
| 13:47:12 | gibi | frickler: yepp, that helps too. thanks. we need to track down all the detach scenarios | |
| 13:47:14 | sean-k-mooney | ya | |
| 13:47:41 | gibi | all volume detach operation is potenitally affected | |
| 13:48:29 | sean-k-mooney | and attach since we do detach as a cleanup action | |
| 13:48:40 | gibi | sean-k-mooney: good point, yes | |