Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-05
16:40:50 sean-k-mooney ill repin hopefully for the last time
16:41:22 sean-k-mooney efried: thanks for finding it it woudl have been even more of a pain when backporting this
16:42:05 efried sean-k-mooney: Well, it's really not a big deal, especially since that method isn't exposed anywhere outside of the decorator. But sure, if you're going to respin; I imagine the other non-blocking nits are worse than that :)
16:42:39 sean-k-mooney well i wasnt but i can if needed
16:43:20 sean-k-mooney this has been part of an a downstream ci blocking bug for almost a month so i would like to finally get it fixed
16:48:05 openstackgerrit Lance Bragstad proposed openstack/nova master: WIP: experiment with oslo.limit interface https://review.openstack.org/615602
16:48:38 lbragstad johnthetubaguy i took a wild crack at bringing the oslo.limit logic closer to the actual enforcement point, based on the commit you have ^
16:49:07 johnthetubaguy lbragstad: ah, interesting, will have a look
16:49:38 lbragstad i guess i wanted to try and see if it was possible to make oslo.limit simple enough to not require a nova.limit module for dynamic limits
16:49:48 lbragstad but - what you have there for static limits totally makes sense
16:50:28 lbragstad i was just thinking that the original direction of the context manager might be hard to take advantage of if we go from api code -> limit module -> oslo limit
16:50:57 lbragstad since oslo.limit and the Enforcer context manager was written to be as close to the actual code consuming resources as possible
16:51:20 johnthetubaguy the problem is the retry check is always in another process at the moment
16:51:44 johnthetubaguy so not sure where the context manager will help, I am hoping jaypipes's patch will be different though
16:52:35 lbragstad dumb question, but what do you mean by the retry logic being in a different process?
16:52:58 lbragstad i was under the assumption that retry logic was in the same area of code that the resource consumption happens
16:53:00 sean-k-mooney what are people feeilngs about adding more debug logs in the schduler
16:53:15 sean-k-mooney or rather in the numa_toplogy_filter to be specific
16:54:06 sean-k-mooney im trying to debug a no valid host form schduler logs and the is nothing to go on to determin why the numa toplogy filter decied it was invlaid
16:54:18 johnthetubaguy lbragstad: it isn't at the moment sadly
16:54:55 lbragstad is the retry logic the thing that protects against race conditions between clients/
16:55:00 lbragstad or is that something else?
16:55:06 jaypipes sean-k-mooney: if a deployer is using the NUMATopologyFilter, they don't care about quickness of the scheduler. I say go for it.
16:55:39 johnthetubaguy sean-k-mooney: I prefer debug logs only on the reject path, if possible, but what jaypipes said too
16:55:58 johnthetubaguy lbragstad: it is that thing, let me link to an example
16:56:17 sean-k-mooney ya it wanted to pring the host toplogy and requested guest topology as a debug message only on failure
16:56:31 johnthetubaguy lbragstad: this is the recheck for build requests: https://review.openstack.org/#/c/615180/5/nova/conductor/manager.py
16:57:08 sean-k-mooney at the moment we jsut asy it did not fit
16:57:23 sean-k-mooney that could be a little noisy however...
16:57:30 johnthetubaguy lbragstad: the first check is in the API process, here: https://github.com/openstack/nova/blob/8d089111c8554e94e117ada3a7f51a42df59e84f/nova/compute/api.py#L868
16:57:36 lbragstad ahhh
16:57:45 johnthetubaguy lbragstad: the recheck is in the conductor, after calling the scheduler
16:57:46 lbragstad interesting... so that's not API code
16:57:57 lbragstad i see what you mean
16:58:05 melwitt sean-k-mooney, tssurya: looks like this already got explained but I can also pile on and say that per user quotas are per project only. so the two-level hierarchy in unifed limits in keystone gets you the same functionality from a nested quota standpoint
16:58:41 sean-k-mooney melwitt: ya that was news to me that user quotas are per project
16:58:52 melwitt gotcha
16:59:04 johnthetubaguy lbragstad: sadly the simplest examples of the recheck basically don't get moved to unified limits
16:59:20 lbragstad because it's not in nova-api?
17:00:04 johnthetubaguy lbragstad: well this is one that could move to unified limits with a context manager, but it doesn't really make sense for other reasons: https://review.openstack.org/#/c/615180/5/nova/api/openstack/compute/server_groups.py
17:00:27 stephenfin jaypipes: Out of curiosity, did you ever post your slides for "Scheduler Wars: A New Hope" anywhere?
17:00:31 lbragstad because it's not really a consumable resource, right?
17:00:41 johnthetubaguy lbragstad: yeah
17:00:47 lbragstad limiting server groups is more of a rate limiting thing
17:00:59 johnthetubaguy yeah, its a db bloat protection
17:01:07 lbragstad got it
17:02:10 openstackgerrit Eric Fried proposed openstack/nova master: Allow resource_provider_association_refresh=0 https://review.openstack.org/615606
17:02:15 johnthetubaguy lbragstad: just to confuse things, I actually propose we remove the rechecks for these rate-limit like things in the spec, since the check is being demoted lets tidy the code up a bit more if we can
17:02:56 lbragstad based on my super vague understand of all this, that seems reasonable
17:02:57 johnthetubaguy lbragstad: I am planning on keeping the recheck for everything in unified limits, but right now one part is in API the other is in the conductor process
17:03:09 melwitt johnthetubaguy: is the conductor recheck the only problem area for the oslo.limit verify? because I've had a TODO in my head to move the recheck back to nova-api
17:03:22 lbragstad oh, nice...
17:04:55 johnthetubaguy melwitt: not sure its too much of a problem really, but that is the only recheck that is relevant in the end. Isn't the issue that we need to write into the correct DB before we recheck?
17:06:05 lbragstad for historical context, what was the reason for moving the recheck logic to conductor? Just to have it closer to the database?
17:06:06 melwitt johnthetubaguy: it is, but related to behavior change we have where multi-create causes potentially a lot of instances to fall into ERROR state if recheck fails, I had been thinking we should instead count build requests + instances for the recheck and do that in nova-api instead of doing the conductor thing
17:06:27 melwitt lbragstad: because that's where the instance record is created, yes
17:06:33 lbragstad got it
17:07:16 johnthetubaguy lbragstad: would it not be OK just to make the context manager optional, because when verify=false the context manager is really strange?
17:07:28 melwitt I can propose a WIP patch today to show what I mean. I just hadn't gotten around to trying it out
17:07:29 johnthetubaguy melwitt: oh good thinking... build requests
17:07:35 lbragstad yeah - i guess you're right
17:07:50 lbragstad originally, we were thinking it would make adoption easier, but in this case it might not...
17:08:30 lbragstad we just didn't want to have to require developers to have to remember to put specific ordering in their API code (e.g., don't forget to recheck *here*)
17:09:23 lbragstad seemed like an interesting opportunity to encapsulate some of that behind the __exit__ of a context manager
17:10:13 johnthetubaguy lbragstad: if we make the recheck compulsory, I think it works (like oslo.limit has the configuration about if you recheck or not)
17:10:35 melwitt yeah, I suppose other people might run into a similar issue if they've got a resource create happening in a separate service/process as well. I'm sorry I missed this case at previous oslo.limit sessions :\
17:11:16 lbragstad well - i think this is just one of those things where you don't really see it fall apart until you're knee-deep in the code
17:11:36 johnthetubaguy yeah, I have similar regrets, that PoC patch has really made me think again on a few things
17:12:00 lbragstad which is good, who knows, maybe the context manager is just an over-optimization at this point
17:12:15 melwitt I think it's definitely a nice option where it fits
17:12:32 lbragstad what if we (oslo.limit) give nova a public API for enforce and recheck (which you can call optionall)?
17:12:32 johnthetubaguy yeah, I like the option of having it
17:12:58 jaypipes stephenfin: yeah, they're on good drive... one sec.
17:13:20 lbragstad and forget the context manager for now
17:13:56 lbragstad in the future, we could expose a context manager that consumes enforce() on __enter__ and recheck() or reverify() on __exit__() for compatibility
17:14:42 johnthetubaguy lbragstad: I think that works, although it makes "claim" seem like the wrong word, its more a resource request
17:14:55 jaypipes stephenfin: https://bit.ly/scheduler-wars-a-new-hope and https://bit.ly/scheduler-wars-revenge-of-the-split
17:15:19 cfriesen The vTPM spec is up at https://review.openstack.org/#/c/571111/ if any cores feel like taking a look. I believe all comments have been addressed.
17:15:31 lbragstad johnthetubaguy yeah - i'm not tied to the terminology, at the time it seemed easy to think of it as "I'm attempting to claim 4 cores on this project"
17:15:35 stephenfin jaypipes: Awesome. Thanks very much
17:15:45 jaypipes np
17:17:14 johnthetubaguy lbragstad: I would be tempted to say include the context manager from the beginning, but its hard to justify if we have no one to use it in v1
17:18:11 lbragstad do you think it's going to be hard to design enforce() and verify() with the hopes that there will be a context manager in the future?
17:19:03 johnthetubaguy lbragstad: that sounds good though, some kind of limit.check call sounds good. Not sure you need a recheck one, depending on the arguments you go for.
17:19:49 lbragstad doesn't nova need a verify() function that isn't associated to the context manager?
17:20:29 lbragstad that way nova-conductor can call it?
17:20:44 johnthetubaguy sorry, I just confused things there
17:22:10 johnthetubaguy if we have the recheck configuration move into olso.limit we need a verify call that takes a list of resource_names to check, only if configured to
17:22:27 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove QuotaEngine.register_resources() https://review.openstack.org/615613
17:22:28 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove get_quota_classes() driver method https://review.openstack.org/615615
17:22:28 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove defaults kwarg in get_project_quotas https://review.openstack.org/615614
17:22:29 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove FakeContext from quota unit tests https://review.openstack.org/615617
17:22:29 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove Context.quota_class https://review.openstack.org/615616
17:22:30 openstackgerrit Jay Pipes proposed openstack/nova master: quota: clean up DbQuotaDriver unit tests https://review.openstack.org/615619
17:22:30 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove _no_class tests https://review.openstack.org/615618
17:22:45 johnthetubaguy so that is what I waiting to see ^
17:23:13 jaypipes johnthetubaguy: that's mostly just cleanups and test refactoring. still have the new LimitsDriver stuff to push.
17:23:35 lbragstad yeah - i was thinking the main reason for not using a context manager was because nova-conductor does the verification step if configured to do so
17:23:35 johnthetubaguy jaypipes: did you see the thing I did here, for the direction I was thinking: https://review.openstack.org/#/c/615180
17:24:02 jaypipes johnthetubaguy: the series there is just slowly chipping away at the bloated quota driver interface, attempting to simplify it as much as possible before introducing further changes.

Earlier   Later