Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-27
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
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,
13:33:16 sean-k-mooney i think melwitt was working on that
13:33:23 gibi ack
13:33:26 dansmith and I think unified limits had a long way to go to be landable even before
13:33:57 dansmith i.e. it's still reaching into oslo library internals and such, last I saw
13:34:15 sean-k-mooney oh ok melwitt raised it as posibel at risk for this cycle but i think she was hopeful it could still be ready in time
13:34:45 dansmith wow, okay I... would be surprised
13:35:29 sean-k-mooney ill take your word for it since i have not been following the details of it
13:35:52 dansmith I didn't see tempest tests for it,
13:36:16 dansmith but I definitely shook out some stuff from the glance implementation when I wrote tempest tests for it
13:36:35 dansmith I don't see docs, and I would think it needs docs
13:36:38 sean-k-mooney there are some https://review.opendev.org/q/topic:%22bp%252Funified-limits-nova%22+(status:open%20OR%20status:merged)

Earlier   Later