Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-27
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: remove module level caching https://review.opendev.org/c/openstack/nova/+/806394
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: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
12:15:28 sean-k-mooney gibi: im going to try and fix the pep8 issue in bauzas mdev series and then split out the docs patch as stephenfin requested into its own patch at the end. are you ok to re review them if i do that. bauzas should be back on monday or tuseday but i would prefer to move this forward if we can before hitting FF
12:17:29 gibi sean-k-mooney: sure, ping me and I will review it
12:22:34 sean-k-mooney oh its a mypy failure we declar we return a sting but if we have an excption we return none. that explains why fast8 did not see it but pep8 does
12:24:54 gibi yes mypy only runs in the pep8 target not in the fast8 one
12:28:24 stephenfin sean-k-mooney: small nits on https://review.opendev.org/c/openstack/nova/+/806394
12:34:10 opendevreview Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294
12:38:43 sean-k-mooney stephenfin: yep just saw them come in. ill gix those up once i finish with bauzas mdev series
12:56:35 opendevreview Merged openstack/nova master: [func test] move port resource request tests https://review.opendev.org/c/openstack/nova/+/801815
13:00:42 opendevreview Merged openstack/nova master: [func test] create pps resource on OVS agent RP https://review.opendev.org/c/openstack/nova/+/787205
13:02:32 gibi lyarwood, stephenfin: I'm not sure I get https://review.opendev.org/c/openstack/nova/+/800086/16/nova/tests/functional/test_servers_resource_request.py#b1669
13:03:31 stephenfin gibi: lyarwood is looking at your earlier statement that those tests are passing even though they wouldn't work in real life, and is saying the tests are therefore misleading
13:03:59 stephenfin and therefore it would be better to comment them out until such a time as their real-world equivalent scenario would pass
13:04:00 lyarwood stephenfin / gibi ; well I'm saying removing them isn't the way to document that
13:04:08 lyarwood right
13:04:31 lyarwood just seems odd to add things, remove them because they are passing when they shouldn't be and then later add coverage back in again
13:04:38 gibi stephenfin: ahh so it is about the interface attach tests that are started passing earlier than the actual implementation?
13:04:45 lyarwood gibi: right
13:04:51 lyarwood it's a nit
13:04:51 stephenfin yup, iiuc

Earlier   Later