Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-06-24
20:15:59 mriedem efried: i have one of your favorite all time questions,
20:16:17 efried airspeed velocity of an unladen European swallow?
20:16:55 mriedem SchedulerReportClient.set_traits_for_provider uses the ProviderTree cache https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1019
20:17:08 mriedem but get_provider_traits does not https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L373
20:17:47 mriedem i'm working on splitting up https://review.opendev.org/#/c/654596/ and the first thing I was going to do was https://review.opendev.org/#/c/654596/2/nova/scheduler/client/report.py as a separate patch which was set_provider_traits w/o the cache,
20:18:06 mriedem so the ComputeVirtAPI/virt driver usage for get/set provider traits would not be tied to a cache
20:18:25 mriedem but now i can't remember why i decided to go the no-cache route,
20:18:45 mriedem since i could link the compute manager / virt api / driver / RT reportclient together and then they'd all be using the same instance of the ProviderTree cache
20:18:51 mriedem and maybe that's desirable
20:19:28 mriedem so i guess main question is, is there any reason get_provider_traits isn't using the cache?
20:22:03 mriedem my more nagging question is if i should try to share the reportclient among compute/rt/virtapi and lean on the cache...though i'm not sure i trust the cache (i guess we'd know if the cache was wrong when we try to set traits on the provider using the wrong generation)
20:22:24 efried mriedem: get_provider_traits is in code paths that have already decided whether the cache is in play or not. It's kind of one of those "low-level" methods that probably - I haven't checked - started off as private and was developed before we had fully developed the caching model.
20:23:26 efried set_traits_for_provider is currently only being used in the rt resource update loop, which has a retry on generation conflicts, i.e. "using" the cache "safely".
20:24:04 efried In principle, up to this point I think we've been avoiding using the cache for any report client *outside* of the compute service.
20:24:15 efried are you intending to *write* traits from the *scheduler*?
20:24:24 efried or conductor
20:24:27 efried ?
20:26:11 mriedem no
20:26:30 mriedem ComputeVirtAPI is part of the ComputeManager/ComputeDriver/RT turducken
20:27:10 efried IMO for "compute disabled" filter you should not use the cache, so get_provider_traits ought to be good as is.
20:27:14 mriedem the trait stuff in this case can be managed started from the compute manager (via call from the API), the driver, or the RT - and the former two are using the same VirtAPI instance
20:28:08 efried you may have to walk me through the flow at a high level here, or I can reread the spec quick...
20:28:12 mriedem ok if i'm not using the cache then i do need https://review.opendev.org/#/c/654596/2/nova/scheduler/client/report.py to set w/o the cache as well
20:28:27 sean-k-mooney mriedem: can it be managed from teh compute manager if the host is down
20:28:34 mriedem sean-k-mooney: no
20:28:41 sean-k-mooney that what i assumed
20:28:55 sean-k-mooney so if the host is actully down how would we update the trait
20:29:01 mriedem we wouldn't
20:29:18 mriedem see the note here https://specs.openstack.org/openstack/nova-specs/specs/train/approved/pre-filter-disabled-computes.html#scheduler-changes
20:29:47 efried mriedem: I'm not sure I see the purpose of the split in https://review.opendev.org/#/c/654596/2/nova/scheduler/client/report.py
20:29:53 sean-k-mooney ah right
20:29:55 efried where were you getting the generation from?
20:30:06 sean-k-mooney this is just covering disabled vs enabled not up vs down
20:30:12 efried if not from the cache?
20:30:26 mriedem efried:
20:30:27 mriedem get_provider_traits
20:30:34 mriedem returns a named tuple
20:30:38 mriedem with the traits list and the gen
20:31:37 mriedem i wasn't going to use set_traits_for_provider because if i'm not relying on the cache at all, then i don't want this to f me:
20:31:37 mriedem if not self._provider_tree.have_traits_changed(rp_uuid, traits):
20:32:03 mriedem i'm doing that myself here https://review.opendev.org/#/c/654596/2/nova/compute/manager.py@494
20:33:55 efried mriedem: compute manager is same process (and using same report client) as RT?
20:34:02 efried (sorry if I'm asking you to repeat yourself)
20:34:10 mriedem yeah
20:34:16 efried then the cache is hot
20:34:29 efried and you don't need to duplicate all that logic.
20:34:37 mriedem https://review.opendev.org/#/c/654596/2/nova/compute/manager.py@573
20:34:57 efried but you're concerned that the cache could be stale?
20:35:26 mriedem well my initial concern was just why get_provider_traits wasn't using the cache if set_traits_for_provider relies on the cache
20:35:41 mriedem which if i use the cache, just becomes an optimization
20:35:54 mriedem i.e. later adding a use_cache kwarg to get_provider_traits or whatever
20:37:24 mriedem looks like only fill_provider_mapping uses get_provider_traits today which was the stuff gibi added and isn't in compute, so i guess that's why it doesn't use the cache
20:37:59 efried Yes, and _refresh_associations, which is the low-level thing that updates the cache, but it refreshes a bunch of other shit you don't need as well.
20:38:41 mriedem there might have been some other reason i needed to hit placement directly, but it's been too long since i wrote this so i guess i'll just rebase and then try to see if things still work after re-using the same report client in the virtapi
20:39:16 mriedem might have been when the virt driver calls the virtapi, but the virtapi in the virt driver is the same virtapi that ComputeManager creates and passes a reference of itself
20:41:07 efried yeah at some point we made sure the report client was a singleton per process
20:43:49 efried mriedem: I think we want to tweak the report client to make this process more generic & reusable. Stand by.
20:45:42 efried Pull this bit of _refresh_associations
20:45:42 efried https://opendev.org/openstack/nova/src/branch/master/nova/scheduler/client/report.py#L794-L803
20:45:42 efried into get_provider_traits itself -- iow calling get_provider_traits should also update the cache
20:46:10 efried (...and dedup that part of _refresh_associations)
20:46:50 efried So then your thingy, and other thingies that do similar, can
20:46:50 efried get_provider_traits() to retrieve the trait info you want to muck with
20:46:50 efried muck with it
20:46:50 efried set_traits_for_provider() with the changed trait list
20:47:37 efried If we wanted to optimize that, we could add a refresh=$bool kwarg to get_provider_traits so you could optionally have it *just* return whatever's in the cache rather than refetching.
20:47:39 mriedem we can't have https://opendev.org/openstack/nova/src/branch/master/nova/scheduler/client/report.py#L802 in get_provider_traits for things that aren't using the cache, like gibi's calls from conductor for fill_provider_mapping
20:47:40 mriedem right?
20:48:05 efried I don't see why not.
20:48:30 efried We'd be updating the cache, but not reading from it anywhere.
20:48:42 efried Updating the cache is of negligible cost
20:48:46 mriedem " # NOTE(efried): This will blow up if called for a RP that doesn't # exist in our _provider_tree. "
20:48:49 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Add placement request filter for disabled computes https://review.opendev.org/654596
20:50:13 efried mriedem: by that point in _refresh_associations, we're already assured the provider is in our cache.
20:50:28 efried you're saying if it's called from elsewhere where that's not the case
20:50:35 efried so yeah, trap and ignore ValueError
20:50:37 efried done
20:50:55 openstackgerrit Merged openstack/python-novaclient master: Revert "Add irrelevant files in dsvm job" https://review.opendev.org/667151
20:51:26 mriedem blech
20:51:35 efried you could do that from within the new-and-improved get_provider_traits
20:51:37 mriedem but yeah that's what i meant - called from conductor where the rp isn't in the cache
20:51:39 efried or check the cache first.
20:52:13 mriedem i'm going to avoid all of that for now because then you could have questions like, "should the caller say if they want the cache and should fail if the rp isn't in the cache?"
20:52:15 efried if self._provider_tree.exists(rp_uuid):
20:52:37 efried "avoid all of that for now" is going to be harder and introduce more technical debt.
20:52:39 mriedem sure - but if i'm compute and expect it in the cache and it's not, we'd want that to fail, not be swallowed
20:53:05 mriedem that's why i said a use_cache kwarg on get_provider_traits seems easier
20:53:07 mriedem the caller says what it wants
20:53:43 efried but you need two knobs
20:53:45 mriedem the very first thing i want to do is just split this large-ish change up into several pieces so i can test in isolation and reason about it
20:53:55 efried 1) retrieve from the cache
20:53:55 efried 2) update the cache
20:54:02 efried one does not imply the other
20:54:18 mriedem ...
20:54:38 mriedem meaning refresh if it's not in the cache and return the cached results,
20:54:53 mriedem or don't get from the cache, get from the API, but update the cache...
20:54:53 mriedem ?
20:55:51 mriedem i'm just going to work on splitting this all up and put a TODO in my code that uses get_provider_traits to somehow leverage the cache - it can be debated in review at that point i think when i actually have something working end to end (which the change is now - the func test shows that, but i'm going to be splitting up and possibly breaking things)
20:56:10 efried okay
20:57:07 spatel sean-k-mooney: hey

Earlier   Later