Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-05-17
20:23:51 cfriesen who knows neutron and live-migration? Do we really need to process "network-changed" events on the dest compute node during a migration when we're going to explicitly call network_info = self.network_api.get_instance_nw_info() anyways in post_live_migration_at_destination()?
20:24:16 melwitt zzzeek: yes. that's why at the moment, it seems like caching these is the way to fix this, and backport it all the way to newton
20:24:52 zzzeek melwitt: it is. So, background, the transactioncontextmanager is a facade around the concept of the SQLAlchemy engine
20:25:07 mriedem cfriesen: we probably don't need to, but does refreshing the network info twice do any harm?
20:25:53 zzzeek this is why it has a method "get_engine()". when you use it as a decorator or context manager, it goes in to a "block" in which scope it actually procures an ORM session which uses an actual sqlalchemy.engine.Connection object
20:26:52 zzzeek melwitt: the concept of the Engine is described at http://docs.sqlalchemy.org/en/rel_1_1/core/engines.html http://docs.sqlalchemy.org/en/rel_1_1/core/connections.html http://docs.sqlalchemy.org/en/rel_1_1/core/pooling.html and would provide some background on what's going on
20:27:44 melwitt zzzeek: thanks
20:27:57 zzzeek patterns
20:27:57 zzzeek melwitt: the history of "enginefacade" itself is that when I came to work on openstack, there was already an "EngineFacade" local to openstack / oslo that evreyone was using, so the curent oslo.db version tries to improve upon that. however, like all layers of indirection, it is confusing. I wrote it and I never remember how it is organized when I go back and look at it, because it had to work on top of all these existing
20:28:57 cfriesen mriedem: turns out that it does when you have a lot (like 16) vifs on an instance. The live migration code can end up blocking for like 35+ seconds waiting to get the 'refresh_cache-%s' lock because it's racing against the incoming network-change events
20:29:11 zzzeek melwitt: so short story, if you make a new engine (and therefore a new connection pool) on every request, then yes, the "pool" is not working because you are making a whole new pool each time
20:29:19 melwitt zzzeek: I see. I too, find confusing :)
20:29:21 mriedem cfriesen: ok i wondered about the lock
20:29:26 cfriesen mriedem: the lack of fair locking means that the events are likely to drop/regain the lock before the live migration code gets it
20:29:52 zzzeek melwitt: ive tried for many years to get the docs / API to express this concept that when you have a URL in SQLAclhemy, you get a *factory*, and not the connection directly, but then with enginefacade in the mix, the bar for explaining is higher.
20:30:16 cfriesen mriedem: adding a greenthread.sleep(0) in the event path helps a lot, but that only works if you're using the "blocking" rpc executor
20:30:33 melwitt zzzeek: gotcha. thanks for explaining. I had been expecting something like a tcp socket pool where it would see us connecting to the same place repeatedly and reuse objects. but I think I understand now why it wouldn't be able to match those things up
20:30:53 mriedem cfriesen: what is the 'blocking' rpc executor?
20:32:18 cfriesen mriedem: oslo_messaging/_executors/impl_blocking.py
20:32:30 cfriesen mriedem: default RPC executor in oslo messaging
20:34:17 mriedem cfriesen: i see, https://docs.openstack.org/developer/oslo.messaging/executors.html
20:34:36 mriedem cfriesen: well, we could potentially ignore the event if we know the instance is undergoing a certain task state transition
20:36:56 mriedem executor='eventlet',
20:36:56 mriedem cfriesen: looking at nova.rpc, we use eventlet for the executor
20:37:07 cfriesen mriedem: we were just wondering internally whether we could ignore them if it was in the middle of a live migration. do you think the state contained in "instance" at the time it was pulled from the DB in ServerExternalEventsController would be recent enough or would we need to check the DB
20:38:29 mriedem cfriesen: between the time that the api pulls the instance from the db and the time it rpc casts to the compute and checks the state, i don't imagine there would be a huge difference, but it does become a race and you could end up ignoring an event that you actually wanted to process
20:38:50 zzzeek melwitt: oh, like an implicit global pool per URL. yeah, we don't do it that way. SQLA doesn't have any global registries. However, you're right that oslo.db *could* serve this purpose. it's just that a URL is bundled with all kinds of other things like pooling config, tranasction isolation, SQL mode stuff, encodings, etc.
20:39:44 cfriesen mriedem: hmm...that's sort of odd, I wasn't seeing a bunch of events getting bunched up all waiting on the lock. In that case the yield isn't guaranteed to buy you anything and only "fair locks" would provide a guarantee
20:40:11 zzzeek (sqla has some global registries but not linked to things like strings)
20:41:01 openstackgerrit Matt Riedemann proposed openstack/nova master: Plumbing for tagged nic attachment https://review.openstack.org/415622
20:41:02 melwitt zzzeek: out of curiosity, is there a better way to do something like connecting to several different DBs? or is what we have reasonable
20:41:03 openstackgerrit Matt Riedemann proposed openstack/nova master: Delete nic metadata when detaching interface https://review.openstack.org/435387
20:41:05 openstackgerrit Matt Riedemann proposed openstack/nova master: Libvirt support for tagged volume attachment https://review.openstack.org/418100
20:41:10 openstackgerrit Matt Riedemann proposed openstack/nova master: Delete disk metadata when detaching volume https://review.openstack.org/435082
20:41:16 openstackgerrit Matt Riedemann proposed openstack/nova master: Plumbing for tagged volume attachment https://review.openstack.org/391941
20:41:17 openstackgerrit Matt Riedemann proposed openstack/nova master: API support for tagged device attachment https://review.openstack.org/376021
20:41:19 openstackgerrit Matt Riedemann proposed openstack/nova master: Libvirt support for tagged nic attachment https://review.openstack.org/415520
20:42:26 zzzeek melwitt: in here I see "db.create_context_manager()" which seems pretty clean. what determines the other settings for this CELL db, they are taken from the global settings? like how do we know it's utf-8 encoding, stuff like that ?
20:43:08 melwitt zzzeek: they all use the same config, they're just clones of what we call the "main DB" they all have the same schema
20:43:36 dansmith clone in the schema sense
20:43:52 melwitt yeah, sorry
20:44:15 zzzeek melwitt: yup. OK so oslo.db could have a featrue like this. like in enginefacade, you give it the URL and some flag, "use_existing" or something, pulls it from a dictionaar like you have here keyed to URL
20:47:47 melwitt zzzeek: okay, cool. thanks. just wanted a sanity check of whether there were more appropriate objects to use that we didn't use
20:48:39 sdague efried: totally feel free to, it was a nice add
20:52:18 efried sdague Pursuant to your colors-by-hash idea, it occurred to me that it might be more useful to do that by service, now that we've got converged logs.
20:53:21 sdague efried: you don't have access that early
20:53:22 efried Just thinking whether consumer is more likely to grep away by req-id or by service - and I'm thinking it's the former by a mile.
20:53:29 sdague that's set by systemd itself
20:53:49 sdague efried: you don't need to grep by service though
20:53:57 sdague because -u does that for you
20:54:00 efried sdague Yeah, exactly my point.
20:54:18 efried It would be a visual break showing you when you're jumping services.
20:54:20 cfriesen task_states.MIGRATING is only for live-migration, right?
20:54:37 sdague right, so the reason I thought it would be nice to rainbow hash the req-id is that it becomes more clear when you have competing flows happening at the some time
20:54:52 sdague most of our races are actually related to that
20:54:57 efried sdague Makes sense.
20:55:19 sdague it's potentially totally gratuitous, but it was enough to wonder "hey, that might be useful"
20:55:56 efried sdague If it's just coloring the req-id string itself (not the whole message), it should be harmless at worst... unless you wind up printing black-on-black.
20:57:08 melwitt cells meeting in a few min
20:57:27 sdague efried: yeh, I was thinking just the req-id string
21:34:02 melwitt dansmith: got a bunch of unit test fails on my patch bc I think I need to clear that cache between tests
21:34:10 dansmith melwitt: yep
21:34:14 dansmith just like we do for the others
21:34:35 melwitt I just grepped for CELLS in the tests and didn't find anything. where are you doing it?
21:34:43 melwitt er, didn't find a clearing of it
21:36:12 melwitt oh, derp, I grepped in nova/tests but it's in nova/test.py
21:36:19 dansmith yepo
21:36:19 melwitt nvm
21:41:08 cfriesen mriedem: dansmith: would something like this seem okay to you? http://paste.openstack.org/show/609851/ I'm a little bit worried about _rollback_live_migration() which only calls setup_networks_on_host() and not get_instance_nw_info() like the network-changed notifier does
21:53:30 openstackgerrit melanie witt proposed openstack/nova master: Cache database and message queue connection objects https://review.openstack.org/465042
22:02:40 mriedem cfriesen: i don't really care for the instance.refresh thing
22:46:04 cburgess Is it actually possible to select additional scheduler filters to use on the boot command?
22:46:56 cfriesen mriedem: the point of that is to query from the DB the current task_state to see if it's migrating...got a better way to do that?
22:49:08 dansmith cfriesen: I don't see why we should skip refreshes altogether for migrating instances, nor do I think it's reasonable to do that heavy lookup every time
22:49:21 dansmith we already send events to both host and destination during migration
22:49:52 dansmith if you ask for any pre-review, can you do it in gerrit please? it's annoying to see only ten lines of context
22:50:31 cfriesen dansmith: we've seen the refresh take 35+ seconds due to lock contention with the incoming network-change events when there are 16 vifs. the events kept on acquiring the lock even though the migration code tried to get it first.
22:50:41 cfriesen dansmith: lack of fair locking was giving grief
22:52:59 cfriesen dansmith: with the 2-sec delay due to neutron batching, the DB lookup from instance.refresh() is way faster
22:53:31 cfriesen dansmith: and the reason to skip them for live-migrating instances is that after the migration is done we're going to refresh the cache explicitly
22:53:33 dansmith but for everyone else that's an extra db lookup on every event
22:55:23 cfriesen dansmith: got any suggestions? fair locking would help unblock the migration code but we'd still have the 2-sec neutron delays (and that seems serialized somehow so it takes ~8sec to finish getting all the network-changed events after a migration
22:56:34 mriedem cfriesen: do you really need the refresh at all?
22:56:56 mriedem what are the chances that the live migration completes and the task_state changes in the window of time between when the api gets the instances for the events and when you check in the compute?
22:58:08 cfriesen mriedem: that's why I asked you earlier...seems like there'd be a small race window where we might ignore an event we should have processed
22:58:38 cfriesen whether that would actually cause problems...I don't know
22:58:58 melwitt cburgess: what do you mean by select? other than adding them to enabled_filters in your nova.conf?
23:00:06 cfriesen mriedem: dansmith: to ignore it the instance would have been migrating at the time nova-api pulled the instance from the DB, so I *think* it should be valid to ignore the event
23:00:16 cburgess melwitt Exactly. We have available_filters and default_filters in nova.conf. default gets used when you do a launch but is there a way as part of a boot to add a filter to that list or use a different filter?
23:01:38 cfriesen cburgess: I don't see a way...get_filtered_hosts() calls self.filter_handler.get_filtered_objects(self.enabled_filters....) where self.enabled_filters is used as the list of filters to evaluate
23:01:45 melwitt cburgess: you mean potentially use different filters for different boot requests? I don't think so
23:02:02 dansmith cfriesen: I don't see why it's valid to ignore it ever, but especially not just because you might've lost the race where we already looked it up in the api
23:04:06 cfriesen dansmith: the event handler for the network-changed message just calls self.network_api.get_instance_nw_info(). the live-migration code will call that explicitly when the migration finishes.
23:04:32 cfriesen that's a relatively expensive call, so we'd like to minimize the number of times we call it
23:05:03 cfriesen and we'd like to avoid the main migration code contending on the lock with the events
23:05:24 cburgess melwitt cfriesen So whats what I thought. Which begs the question.. why do we have 2 config values?
23:05:39 melwitt cburgess: I think the only way to get different behavior for different boot would be scheduler hints (and add a filter that handles that hint) or extra specs to do it via flavor
23:06:03 cburgess melwitt Ahh that could be why hints. I haven't looked at that.
23:06:11 melwitt cburgess: that's a good question ... checking
23:06:29 cfriesen cburgess: 2 config values for what/

Earlier   Later