Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-27
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
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 stephenfin yup, iiuc
13:04:51 lyarwood it's a nit
13:05:17 lyarwood I shouldn't really -1 for it, it's just confusing as a reviewer tbh as opposed to leaving them commented out with a note
13:05:27 lyarwood IMHO
13:21:06 gibi sorry , I got distracted with a call
13:24:10 gibi OK, I got confused as you marked code lines that are about booting VMs not about interface attach
13:24:16 gibi anyhow
13:24:38 gibi those interface attach tests that are passing are not false positives
13:24:43 gibi those sceanrios really works
13:24:49 gibi but those scenarios are error scenarios
13:25:20 gibi so they are pretty useless for the end user
13:26:17 gibi I will respin the patches to move the service version checks to the compute.api
13:26:37 gibi but I'm not sure what to do about the interface attach tests you commented on
13:26:48 gibi I cannot comment them out as they are in the base class where they pass
13:27:18 lyarwood ah crap right sorry, ignore that then
13:27:25 gibi I can redefine them with empty body and a comment but that feels more confusin than actually doing nothing
13:27:33 sean-k-mooney you can boot with one ovs and one seriov prot today
13:27:48 gibi sean-k-mooney: ignore the context of the comment the comment is not about those test cases
13:28:16 gibi sean-k-mooney: it is about interface attach test cases that started to pass earlier (and therefore not mentioned in the code at all) than expected
13:28:17 sean-k-mooney right but im wondering why they were previouly mared as exped failure
13:28:28 sean-k-mooney was that because of resouce requests?
13:28:33 gibi sean-k-mooney: yepp,
13:28:38 sean-k-mooney ah ok
13:28:57 gibi sean-k-mooney: we had first patches to reject every operation with the new extended resource request format
13:29:09 gibi sean-k-mooney: then this patch adds the impl for them and remove the rejection
13:29:16 sean-k-mooney got it
13:29:31 sean-k-mooney ok ill leave it for not sicne i would have to start at the begining of the serise to review properly
13:30:02 gibi sean-k-mooney: yeah it is a long one :)
13:30:39 sean-k-mooney well its more if i jump in mid seriese without context i wont really understand what your doing so it wont be helpful
13:31:00 sean-k-mooney and i dont have time today unfortunetly to load all that context
13:31:16 sean-k-mooney but if there is anything you want me to look at just let me know
13:31:17 gibi sean-k-mooney: no worries, I got good reviews from stephenfin and lyarwood on the series
13:31:39 gibi sean-k-mooney: it is better to spread our effort, like making the mdev series land as well
13:32:17 sean-k-mooney ack yep thats why im working on it now
13:32:22 gibi cool
13:32:31 sean-k-mooney im hoping unified limits can also make it
13:33:02 gibi does the dansmith's comments have been resolved there?
13:33:15 dansmith not that I've seen,

Earlier   Later