Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-05-17
20:11:38 dansmith I'm guessing it doesn't know that each re-connection to the same db is reusable though? so it's keeping 5 connections of every transaction manager?
20:11:50 zzzeek dansmith: we end up with 1.4K connections on an openstack even w/o separate cell DBs if we have dozens of processes. it's kind of a problem to which there are a few solutions
20:11:53 dansmith if that's the case, we should just not do that thing (i.e. do what melwitt has proposed)
20:11:57 melwitt zzzeek: I had thought the pool would hand back an object that is the same DB as before
20:12:10 dansmith zzzeek: well, it doesn't happen with melwitt's patch
20:12:25 zzzeek dansmith: right but, you don't need a patch. just turn down the pooling
20:12:53 zzzeek dansmith: with one exception, which is that to disable the pool entirely, you need to use the "nullpool" implementation and we might need to add a hook to oslo.db if you really want that. but that is not what melwitt's patch is doing.
20:13:02 dansmith well, we get different behavior than what we have today.. meaning we defeat the benefits of the connection pooling , if there are benefits
20:13:39 zzzeek dansmith: just in the last few IRC lines im not sure if all three of us are clear on every concept here, for example, I don't know what a "Transaction manager" is
20:13:52 dansmith zzzeek: the oslo.db context
20:14:02 melwitt it's the _TransactionContextManager class in oslo.db
20:14:06 dansmith https://review.openstack.org/#/c/465042/3/nova/context.py
20:14:10 dansmith L384 there ^
20:14:12 zzzeek also, the idea of a "pool" is that yes, it has a connection that it will reuse as needed
20:14:34 melwitt okay, so I don't know what we're doing to not get the reusability
20:15:05 dansmith melwitt: the pool manager has to know that a repeated connection attempt is "the same" in order to re-use an existing connection
20:15:19 dansmith so I'm wondering if we're confusing it such that it isn't re-using every time we create a new context there
20:15:34 zzzeek dansmith: ok more terms that confuse me, what is "the pool manager"
20:15:35 melwitt right. how does it normally know that I guess? yeah
20:16:04 dansmith zzzeek: I'm sure I'm not using the right term.. whatever the thing is in sqla that does the pooling.. you said "nullpool" above, which would be one of those things
20:16:21 dansmith melwitt: well, it can do connection pooling with one context easily
20:16:31 melwitt right ...
20:16:39 dansmith and that's all we've needed in the past
20:17:01 dansmith but now we're creating lots of those things
20:17:03 zzzeek dansmith: OK. so we are talking aobut SQLAlchemy QueuePool or NullPool. it doesn't know about connection attempts being "the same" or anything like that, it basically is asked for a DB connection, and it gives you one from its pool. if it doesnt have any, it either decides to give you an ad-hoc connection based on what you've configured "max_overflow" towards, or, it makes you wait
20:17:06 melwitt zzzeek: this is how we establish db connections (static) and what we did to "switch" is just create a new one of those each time https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L79-L80
20:17:07 mriedem i'd think it would key off the connection url
20:17:24 melwitt mriedem: that's what I had thought
20:17:31 dansmith maybe, but that has creds in it
20:17:47 dansmith zzzeek: but is that pool global, or per-context or what?
20:18:21 zzzeek dansmith: if nova is architected here per best practices, you'd have one pool per unique database URL
20:18:42 dansmith heh
20:18:45 zzzeek dansmith: if OTOH you are creating brand new engines every time you get a requset, then you're making new pools each time and that is the wrong way to do that
20:19:01 dansmith we're a long ways away from the pool in nova, so it's hard to say
20:19:27 zzzeek dansmith: so in this patch i see "db_connection", and if that is in fact an "Engine" object, or say the enginefacade factory around that, then that's OK, that's what should happen here.
20:19:40 zzzeek that's not a "connection", it's a factory for connections
20:19:47 zzzeek if that's the case, I dont really know what this is
20:20:20 dansmith enginefacade.transaction_context()
20:20:25 dansmith it's that ^
20:20:28 melwitt it's a _TransactionContextManager
20:20:41 zzzeek dansmith / melwitt ok
20:20:56 melwitt I modeled it off how it's done in our db/sqlalchemy/api.py for the static connections
20:21:32 zzzeek melwitt: OK then that's fine let me update my thing
20:21:55 efried sdague Thinking I should blast the ML to let 'em know about journalctl -a, yah?
20:21:59 zzzeek melwitt: to clarify, previosly, everyime new cell DB were used, you'd build up whole new _TransactionContextManager per request ?
20:22:22 efried I mean, it's in the doc now (when does that get flushed to docs.openstack.org?) but who's gonna go reread it?
20:23:03 melwitt zzzeek: yeah. and that was based on my limited knowledge of how to use oslo.db, if I want to establish a new connection from only a connection url, it seemed like that's the only way?
20:23:23 melwitt is to use enginefacade.transaction_context()
20:23:44 zzzeek melwitt: OK so, is that version of the code what's released in production and all that? because that will perform very, very badly
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 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:27:57 zzzeek patterns
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 cfriesen: looking at nova.rpc, we use eventlet for the executor
20:36:56 mriedem executor='eventlet',
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.

Earlier   Later