| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-26 | |||
| 17:58:31 | melwitt | dansmith: yes | |
| 17:58:32 | sean-k-mooney | rahter then in the test module that cares | |
| 17:59:21 | sean-k-mooney | ok ill add a reset_globals function to the nova.db.migration module and then call that form test.py | |
| 17:59:26 | sean-k-mooney | ill push that in a few minutes | |
| #openstack-nova - 2021-08-27 | |||
| 01:31:15 | opendevreview | Qiu Fossen proposed openstack/nova master: Allow migrating PMEM's data https://review.opendev.org/c/openstack/nova/+/802225 | |
| 01:32:55 | opendevreview | melanie witt proposed openstack/nova master: Revert "consumer gen: more tests for delete allocation cases" https://review.opendev.org/c/openstack/nova/+/806292 | |
| 01:32:56 | opendevreview | melanie witt proposed openstack/nova master: Revert "Consumer gen support for delete instance allocations" https://review.opendev.org/c/openstack/nova/+/806293 | |
| 01:36:51 | melwitt | gibi, sean-k-mooney, lyarwood: fyi I have uploaded reverts https://review.opendev.org/c/openstack/nova/+/806292 and https://review.opendev.org/c/openstack/nova/+/806293 as one possible option for addressing the gate bug. another option is the WIP patch https://review.opendev.org/c/openstack/nova/+/688802 to add a "force" kwarg to allocation delete | |
| 01:44:20 | opendevreview | Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294 | |
| 06:42:53 | slaweq | gibi: hi | |
| 06:43:09 | slaweq | gibi: I see a lot of job's failures with error on deleting instance, like e.g. https://a91ddba9ffca8a8b5d72-1d82fafbdd07c6a56856d7b1449f73a8.ssl.cf5.rackcdn.com/800059/3/check/neutron-ovn-tempest-ovs-release/fd05a18/testr_results.html | |
| 06:43:20 | slaweq | and it's not only on neutron-tempest-plugin jobs | |
| 06:43:30 | slaweq | I saw it also on some devstack jobs too | |
| 06:43:41 | slaweq | gibi: is it known issue or should I open one? | |
| 07:04:18 | gibi | slaweq: hi | |
| 07:04:26 | frickler | slaweq: I would be hoping that these are solved by the above reverts https://review.opendev.org/c/openstack/nova/+/806293 etc. | |
| 07:04:33 | gibi | just couple lines above melwitt proposed patches | |
| 07:05:35 | slaweq | gibi: frickler thx a lot | |
| 07:05:56 | gibi | I've just started my day but I will prioritize this issue | |
| 07:06:35 | slaweq | gibi: thx, I just wanted to ask as I knew that You will know better if that's something already reported :) | |
| 07:06:40 | gibi | sure | |
| 07:07:13 | frickler | slaweq: if you check n-cpu.log for errors, you'll see tracebacks about conflicts in placement allocations | |
| 07:09:07 | slaweq | frickler: yes, now I see it :) | |
| 07:37:04 | gibi | melwitt: thanks for proposing patches I read up on the bug discussion and the patches and I prefer the force kwarg that gives us a more fine grained approach to handle deletes in different situations | |
| 07:37:28 | gibi | I will resolve the merge conflict in https://review.opendev.org/c/openstack/nova/+/688802 and then I will +2 it | |
| 07:37:34 | gibi | sean-k-mooney, lyarwood, stephenfin: ^^ | |
| 08:19:55 | lyarwood | Morning | |
| 08:33:12 | gibi | lyarwood: morning | |
| 08:55:15 | lyarwood | gibi: sorry got distracted with $child things | |
| 08:55:35 | lyarwood | gibi: so how's the rebase going? Are we not going ahead with the reverts just yet? | |
| 08:55:48 | gibi | lyarwood: almost done | |
| 08:55:55 | lyarwood | ack | |
| 08:56:17 | gibi | lyarwood: for me the reverts are more complex than the force kwargs fix | |
| 08:56:26 | lyarwood | yup agreed | |
| 09:01:47 | opendevreview | Balazs Gibizer proposed openstack/nova master: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/c/openstack/nova/+/688802 | |
| 09:02:18 | gibi | lyarwood, sean-k-mooney, stephenfin: ^^ | |
| 09:15:30 | lyarwood | gibi: LGTM, had to do a little background reading to make sure but yeah still agree this is the better approach | |
| 09:19:10 | gibi | lyarwood: yepp the bug report has a good summary of what we are dealing with | |
| 09:28:06 | opendevreview | Fabian Wiesel proposed openstack/nova master: VmWare: Remove unused legacy_nodename regex https://review.opendev.org/c/openstack/nova/+/806336 | |
| 09:46:50 | gibi | lyarwood: thanks for the review on the pps series. I replied in https://review.opendev.org/c/openstack/nova/+/793619/15#message-a2916b6a95c6f4061429e9fb2898ed738dfc3427 | |
| 10:33:51 | opendevreview | FossenQiu proposed openstack/nova master: Allow migrating PMEM's data https://review.opendev.org/c/openstack/nova/+/802225 | |
| 10:36:06 | opendevreview | Fabian Wiesel proposed openstack/nova master: Vmware: Fix spelling in test https://review.opendev.org/c/openstack/nova/+/806348 | |
| 10:43:43 | opendevreview | Fabian Wiesel proposed openstack/nova master: VmWare: Use of id shadows built-in function https://review.opendev.org/c/openstack/nova/+/806390 | |
| 10:45:41 | lyarwood | gibi: https://review.opendev.org/c/openstack/nova/+/688802 has some unit and functional failures if you have time to look | |
| 10:47:00 | opendevreview | Fabian Wiesel proposed openstack/nova master: Vmware: Fix indentation in conditionals https://review.opendev.org/c/openstack/nova/+/806391 | |
| 10:49:26 | gibi | lyarwood: whaat? I did run those tests before pushed | |
| 10:49:30 | gibi | looking | |
| 10:50:02 | lyarwood | yeah assumed you had, weird. | |
| 10:54:29 | gibi | obviously I run it on something else as I see failures that I havent seen before :/ | |
| 10:54:35 | gibi | anyhow fixing it ... | |
| 11:09:19 | gibi | the two failing functional test actually testing our bug that is now resulved with force=True | |
| 11:09:54 | sean-k-mooney | the reverts to me are still the better long term solution as over all i think it more complext to have 2 ways to delete things | |
| 11:10:40 | gibi | sean-k-mooney: but the automatic reclaim of soft deleted instance should not win over a user initiated restore of the same soft deleted instance | |
| 11:10:47 | sean-k-mooney | although the force flag patch is slightly smaller | |
| 11:12:18 | sean-k-mooney | hum... i can see that althoug i also dont think we should uspport soft deleted in general | |
| 11:12:47 | sean-k-mooney | so you are saying the added complexityu to support soft cdelete requires force | |
| 11:13:04 | sean-k-mooney | to ensure we can restore on the same host | |
| 11:13:22 | sean-k-mooney | without other allocations talking up the space | |
| 11:13:48 | sean-k-mooney | im still not sure we need to use a put in the soft delete case | |
| 11:14:00 | gibi | what I'm saying that a conflicting delete should be forced most of the time, except if the delete is conflicting with a restore of a soft deleted instance | |
| 11:14:49 | gibi | conflict means that restore receved first, we should not allow removing the allocation in this case blindly as it will lead to an active instance without allocaiton | |
| 11:15:16 | sean-k-mooney | would we not be able to do that with a lock on the instecak uuid | |
| 11:15:41 | sean-k-mooney | have restore aquire the lock and have the perodic try to aquire it too before it does the delete | |
| 11:16:28 | gibi | yes a lock would also be a solution | |
| 11:17:01 | sean-k-mooney | if we want to proceed with the force flag for now im not nessisarly against that | |
| 11:17:23 | sean-k-mooney | but im still not trilled by doing deletes with put in general | |
| 11:17:30 | opendevreview | Balazs Gibizer proposed openstack/nova master: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/c/openstack/nova/+/688802 | |
| 11:17:50 | gibi | sean-k-mooney: I'd like to proced with the force solution, reverting old patches feels messy. | |
| 11:18:04 | gibi | I need to go grab lunch, be back in 30 | |
| 11:19:16 | sean-k-mooney | ack enjoy | |
| 11:20:21 | sean-k-mooney | stephenfin: ya removing the globals is certenly another valid option. if there is no global state we have nothign to reset. ill take a look at that quickly | |
| 11:34:54 | opendevreview | sean mooney proposed openstack/nova master: db: Handle parameters in DB strings https://review.opendev.org/c/openstack/nova/+/805663 | |
| 11:34:54 | opendevreview | sean mooney proposed openstack/nova master: remove module level caching https://review.opendev.org/c/openstack/nova/+/806394 | |
| 11:35:42 | sean-k-mooney | stephenfin: ^ they were only ever used in that module so there was no test covergae for the gloabls so simple to remove them as you suggested | |
| 11:39:27 | gibi | sean-k-mooney: I agree less global is better | |
| 11:42:01 | sean-k-mooney | we can re add them if this really is a problem but personaly i would prefer to use the lru cache decorator on the function if it came to that | |
| 11:42:28 | sean-k-mooney | i tihnk stephen is correct that this likely is not providing much if any useful perfromance benifit today | |
| 11:44:14 | gibi | sean-k-mooney: lru cache decorator propbably store info on the func def, so that is also a global, just a hidden one. Still needs the same reset logic as a pure module level global | |
| 11:44:46 | gibi | I'm not sure what I like more, but I guess in this case a pure global is easier to notice | |
| 11:44:52 | sean-k-mooney | gibi: yes it would but i just prefer using libary function for caching then inventing it our selves | |
| 11:45:19 | sean-k-mooney | anyway im not suggesting we use caching here | |
| 11:45:50 | gibi | yepp, the reinventing-the-wheel is on the other side of the compromise | |
| 11:45:59 | gibi | agree, only add cache if we see slowness | |
| 11:46:56 | sean-k-mooney | for what its worth where we do have our own caching i lie the pater we have in the compute rpc module https://github.com/openstack/nova/blob/50fdbc752a9ca9c31488140ef2997ed59d861a41/nova/compute/rpcapi.py#L43-L50 and a few others | |
| 11:47:13 | sean-k-mooney | e.g. where we providee a reset_globals fucntion in the module to handel that | |
| 11:47:26 | sean-k-mooney | as an external caller i done need to look at the details then | |
| 11:48:24 | sean-k-mooney | by the way speaking fo global state there was a patch i wanted to flag to you | |
| 11:49:36 | sean-k-mooney | gibi: https://review.opendev.org/c/openstack/nova/+/804985 | |
| 11:50:15 | gibi | looking.. | |
| 11:52:00 | gibi | sean-k-mooney: I agree with your comment on ^^. I do think eventlet and python threads are not play nice together and we have extensive separation between them on the compute side to prevent issues | |
| 11:52:51 | sean-k-mooney | there change would not break anything but if they deployed the way we suggest with only 1 therad and multiple processes i dont think it would fix anything either right | |
| 11:53:21 | sean-k-mooney | they would have to deploy in a "unsupproted" configuration where wer wer usign mutiple thread curretly right? | |
| 11:55:41 | gibi | sean-k-mooney: if they use oslo lock without external=True then the lock is only help with threads not processes. | |
| 11:56:09 | gibi | and I do think we only support process based scaling | |
| 11:56:45 | sean-k-mooney | looking at there responce to my commnet https://review.opendev.org/c/openstack/nova/+/804985/3#message-e6eddd2104d423b51d8c4beb14e3eaa7998cf177 | |
| 11:57:07 | sean-k-mooney | im wondering if we are timing out on an io operation | |
| 11:57:12 | sean-k-mooney | rather then a race | |
| 11:57:46 | sean-k-mooney | i think im just going to ask them to file a bug. part of the porblem is we dont know what verion of nova this happens in | |
| 12:03:14 | gibi | sean-k-mooney: yepp, we need to find a way to see if they really using threads and that casuing the problem or there is a problem even with eventlet | |
| 12:05:43 | sean-k-mooney | ok i have asked them to file a bug report and linked them to the docs on our threading model in case that helps | |