Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-08-27
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)
13:36:57 sean-k-mooney https://review.opendev.org/c/openstack/tempest/+/790186 and https://review.opendev.org/c/openstack/tempest/+/804311
13:37:05 dansmith I see gibi has a bunch of +2s over the patch I'm -1 on, but I think some of those have code that reaches into oslo.limit
13:38:01 dansmith ah okay cool, glad to see those tempest patches
13:38:18 dansmith missed them being WIP I guess
13:38:32 gibi dansmith: as far as I understand even if we land unified limits it will be a sort of experimental optional feature
13:38:58 gibi there was no API impact so I considered it safe to experiment
13:39:09 dansmith mmmkay :)
13:39:19 gibi :)
13:40:40 sean-k-mooney https://www.youtube.com/watch?v=6wJXBUfcIOE
13:43:39 dansmith gibi: did you see my comment on this patch you have +Wd? https://review.opendev.org/c/openstack/nova/+/712139
13:43:46 dansmith I think those exceptions are wrong, no?
13:45:44 stephenfin gibi: I'm +2 on basically the whole QoS series now, bar the things you've already talked about and the last two patches (small issues there)
13:46:13 gibi dansmith: could be a bit more chatty yes, but ValueError tend to be used to point out which formal parameter got an unexpected value in the functin call
13:47:01 gibi stephenfin: thanks, much appreciated
13:47:26 dansmith gibi: sure, but is that caught and logged appropriately?
13:48:13 dansmith gibi: usually that sort of thing ends up as not a trace in a log line where you just see "ValueError: entity_type" and never know what was going on when it happen
13:48:17 dansmith but if so, cool
13:48:53 gibi dansmith: thanks for pointing that out. I dropped my vote. I have to go back and see how that error is propagated
13:49:26 dansmith gibi: I see there's a test for the specific string, so I guess it's intentional, but I still would like to know that it's turned into something useful :)
13:50:35 gibi dansmith: I agree with your concerns

Earlier   Later