Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-05
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.
17:24:06 johnthetubaguy lbragstad: yeah, its just right now we use a single call, we pass +0 extra resources in the conductor
17:24:54 johnthetubaguy jaypipes: yeah, my thinking was to put a new stack next to the old one, and remove the old stack later, but open to ideas
17:25:16 johnthetubaguy (took me all friday to work out that is what I was actually thinking)
17:27:12 lbragstad johnthetubaguy right - and that should still be possible if oslo.limit gives you enforce() and verify() functions, right?
17:27:34 lbragstad you'd just be calling them from two different places
17:27:58 johnthetubaguy lbragstad: yeah totally, I just was wondering if you needed the separation, I think you probably do
17:28:12 melwitt yeah, you're just thinking allow another option where the APIs are decoupled right? and then if they are co-located in some part of code, you could use the context manager
17:28:27 lbragstad ^ yeah
17:28:33 johnthetubaguy yeah, +1
17:28:33 melwitt because enforce == first check and verify == second check with +/- 0
17:28:41 lbragstad the context manager could use the same exact public APIs
17:28:51 johnthetubaguy totally agreed with that
17:28:55 melwitt that actually sounds like it fits nicely
17:29:02 johnthetubaguy +1
17:29:16 lbragstad ok - sounds like we have some things we can improve in oslo.limit then
17:29:31 johnthetubaguy I wondered if you could just have a single check API that is used in both sides of the context manager, but possibly not
17:29:52 johnthetubaguy i.e. use check in enter and exit
17:30:12 lbragstad you mean make enforce() generic enough to handle all cases?
17:30:16 johnthetubaguy but I think I prefer oslo.limit deciding if a recheck is done or not
17:30:17 johnthetubaguy yeah
17:30:39 lbragstad yeah - i think wxy-xiyuan tried to do something like that initially
17:31:32 jaypipes johnthetubaguy: did you want a review on the patch above?
17:31:52 jaypipes johnthetubaguy: I'm not a fan of mixing usage and limit information in the same function/object
17:32:33 jaypipes johnthetubaguy: lemme push up what I've been working on. perhaps it will make a bit more sense.
17:32:43 johnthetubaguy jaypipes: from a direction point of view, that would be great

Earlier   Later