| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 18:19:08 | mdbooth | We'd be hitting bugs here | |
| 18:19:28 | mdbooth | We're only relatively safe because we don't do that | |
| 18:20:04 | mdbooth | Ideally something which might read a few bdms would have its own read transaction | |
| 18:20:12 | mdbooth | Which covered all of them | |
| 18:20:25 | mdbooth | Then suddenly one of them doesn't have a uuid and we need to do a write | |
| 18:20:36 | mdbooth | This would fail, because we're in a read transaction. | |
| 18:21:00 | mdbooth | If we always do the write in an independent transaction, that doesn't matter | |
| 18:21:32 | mdbooth | Actually, I wonder if we'd hit that in practise... | |
| 18:24:51 | mdbooth | No, we currently still have separate micro-transactions everywhere | |
| 18:26:31 | _ix | Hello again. I was curious about how to best segment my deployment. I'd like some hypervisors running off of rados block devices, and others running off of local ssd. Are availabiltiy zones what I'm looking for? | |
| 18:27:09 | mdbooth | But if, for eg, we annotated BlockDeviceMapping.get_by_volume_id with @reader so that it fetched the joined instance in the same transaction as the bdm, that would be a bug. It would be pretty obtuse, though, because it's not at all obvious that _from_db_object can write | |
| 18:30:11 | _ix | I must be looking for host aggregates. | |
| 18:31:33 | mdbooth | jaypipes: Nah, the comments are longer than the code. | |
| 18:31:58 | mdbooth | s/code/code to fix the problems requiring comments/ | |
| 18:33:50 | sean-k-mooney2 | every time i read bdms i keep wanting to change it to dbms as i assume i reversed the letter because of my dyslexia | |
| 18:38:45 | jeblair | hi, we're looking at having nodepool ask nova for its quota periodically to make it more responsive to changes. does nova cache limit information for users (cores, ram, instances)? if so, what's a typical cache period? | |
| 18:40:46 | melwitt | jeblair: I don't think limit info is cached | |
| 18:42:45 | sean-k-mooney2 | melwitt: is limit info still stored in nova or is that contolled by keystone now for cores,ram and insances? | |
| 18:42:58 | jaypipes | mdbooth: actually, decorating the _from_db_object() with the @writer context is actually a very *explicit* way of saying the expectations of that method. https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L902 | |
| 18:43:38 | jaypipes | mdbooth: and functional tests of the data online migration will blow up nicely if it's set to @reader instead of @writer. | |
| 18:43:49 | jaypipes | mdbooth: and blowing up is a good thing in this case... | |
| 18:43:50 | mdbooth | jaypipes: Absolutely, but it's unnecessary except in this really weird edge case. | |
| 18:43:56 | jaypipes | mdbooth: agreed | |
| 18:44:04 | mdbooth | Which will go away... | |
| 18:44:11 | jaypipes | mdbooth: thus the comments saying so :) | |
| 18:44:19 | mdbooth | Except that now everything above it was unnecessarily promoted to writer | |
| 18:44:19 | jaypipes | mdbooth: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L904-L905 | |
| 18:44:38 | mdbooth | And now you have to go audit all the things to find out after the fact which ones were really supposed to be writers. | |
| 18:45:03 | jaypipes | yeah, that's the tradeoff I suppose | |
| 18:45:40 | dansmith | sean-k-mooney2: still in our db | |
| 18:46:12 | mdbooth | jaypipes: That races, btw | |
| 18:46:21 | sean-k-mooney2 | dansmith: oh ok was i imagining that we wanted to move it to keystone at some point? | |
| 18:47:06 | mdbooth | jaypipes: Concurrent reads can create different uuids | |
| 18:47:29 | jeblair | melwitt: thanks, that's easy then :) | |
| 18:49:11 | sean-k-mooney2 | mdbooth in which case it needs to be a writer lock no? | |
| 18:49:49 | mdbooth | sean-k-mooney2: Doesn't matter, unless both prior reads are done with 'for update' (or equivalent) | |
| 18:50:08 | mdbooth | Which I don't think they are | |
| 18:50:57 | mdbooth | You'll get 2 reads of 2 null values. They'll race to update. The first will write a value and block the second one. When the first commits, the second will write its own different value. | |
| 18:51:07 | mdbooth | Both functions will return objects with different uuids. | |
| 18:51:44 | sean-k-mooney2 | mdbooth: what you effectivly want to do is have a cas so only one updates the record | |
| 18:52:07 | sean-k-mooney2 | mdbooth: im guessing thats what 'for update' will give you | |
| 18:52:13 | mdbooth | cas? | |
| 18:52:18 | jaypipes | compare and swap | |
| 18:52:23 | mdbooth | Ah, yeah | |
| 18:52:24 | jaypipes | but we're overthinking this I think. | |
| 18:52:27 | mdbooth | That's what I wrote | |
| 18:52:42 | jaypipes | yeah, the update_with_match thing is the compare and swap | |
| 18:52:46 | jaypipes | regardless... | |
| 18:52:50 | mdbooth | https://review.openstack.org/#/c/242603/25/nova/objects/block_device.py | |
| 18:54:19 | mdbooth | jaypipes: Thing is, if you overthink the building blocks, it means you don't need to overthink everything else. Not so much, anyway. | |
| 18:54:44 | mdbooth | The caller doesn't need to be concerned with all these weird edge cases. It just works. | |
| 19:00:26 | openstackgerrit | Merged openstack/nova master: Fix doubling allocations on rebuild https://review.openstack.org/521662 | |
| 19:01:51 | zzzeek | mdbooth / jaypipes the retry decorartor is wanting, IMO | |
| 19:02:05 | zzzeek | there was originally some idea to integrate one into enginefacade itself so that it knows how to nest | |
| 19:02:40 | mdbooth | zzzeek: Could you use a savepoint for this, btw? | |
| 19:03:10 | zzzeek | mdbooth: not if you're looking to respond to other transactions also making changes b.c. isolation may prevent you from seeing those changes | |
| 19:03:40 | zzzeek | for an UPDATE i think repeatable read would mean you can't see another transactions updates | |
| 19:05:31 | mdbooth | zzzeek: Anyway, my approach was to use an independent transaction context explicitly. I figured this would always be safe. | |
| 19:06:02 | mdbooth | Then I was convinced it wasn't worth writing slight weird code for an extremely obtuse edge case where failure was safe. | |
| 19:06:03 | zzzeek | mdbooth: i think you are correct assuming that record is not part of the external tranasction. pre-enginefacade nova was all independent transactions. | |
| 19:06:37 | mdbooth | But then I flipped again because I figured this is a read transaction which only needs to write in an edge case (migrating a legacy record). | |
| 19:07:10 | mdbooth | So having an independent transaction is useful to avoid forcing the enclosing transaction and all its potential parents to become writers. | |
| 19:08:06 | mdbooth | And that's where I am now. | |
| 19:10:38 | zzzeek | mdbooth: i think leave it and goad me into working up a better retry situation for enginefacade | |
| 19:11:08 | mdbooth | zzzeek: So all of the above is in the context of code we'd want to delete next cycle anyway, as it's only for a migration :) | |
| 19:11:48 | mdbooth | Well cycle +1, because the online migration runs it too. | |
| 19:11:48 | zzzeek | mdbooth: nova's magical migrations...sure | |
| 19:11:52 | zzzeek | mdbooth: sure | |
| 19:12:00 | zzzeek | mdbooth: this is not the most concerning thing to me :) | |
| 19:13:10 | mdbooth | I think it's fine, tbh. I also doubt that many of nova's uses of the retry wrapper are a problem today, because we don't nest many transactions. | |
| 19:13:35 | mdbooth | It would be a bigger problem if we made wider use of the enginefacade | |
| 19:14:29 | mdbooth | jaypipes: Thanks again | |
| 19:22:30 | openstackgerrit | Chris Dent proposed openstack/nova master: Optional separate database for placement API https://review.openstack.org/362766 | |
| 19:33:20 | cdent | can someone besides mriedem put some eyes on https://review.openstack.org/#/c/523403/ is an auth config error that would be useful to backport. dansmith, jaypipes ? | |
| 19:33:55 | efried | cdent Patches are passing with your grenade fix - thanks for that. | |
| 19:34:06 | cdent | huzzah | |
| 19:34:30 | mriedem | cdent: i think melwitt enjoys auth config bug fixes | |
| 19:34:59 | melwitt | I totally do | |
| 19:35:02 | cdent | ah yes melwitt would be a fine choice | |
| 19:35:28 | cdent | apologies had not hear you around recently | |
| 19:35:35 | cdent | whereas dansmith and jaypipes … | |
| 19:35:57 | efried | cdent Is the kwarg really called oslo_config_config? | |
| 19:36:19 | cdent | yeah, isn’t that awesome, there are a few different olso_config_* | |
| 19:36:54 | efried | whee | |
| 19:37:42 | cdent | I think it is to try to be explicit about the fact that it can also accept keyword arguments which are configuration keys and values | |
| 19:38:05 | cdent | a sort of namespacing | |
| 19:41:27 | sean-k-mooney2 | cdent: we were going to do something similar with networking-ovs-dpdk devstack setting then reallised that would haver ment setting OVS_DPDK_OVS_REPO AND OVS_DPDK_DPDK_REPO so decided not to prepend everyting with OVS_DPDK | |
| 19:42:08 | cdent | oh my | |
| 19:42:52 | sean-k-mooney2 | we still have pretty verbose setting without that https://github.com/openstack/networking-ovs-dpdk/blob/master/devstack/settings | |
| 19:42:53 | efried | cdent Left a comment on that patch. Is there a reason we would need to keep that arg? | |
| 19:43:49 | cdent | efried: because we don’t know who might have use the deploy() method in the past | |
| 19:44:01 | cdent | that’s how you build a custom wsgi application, if you’re so inclined | |
| 19:44:07 | efried | cdent Is it somehow public? | |
| 19:44:24 | cdent | it is public by virtue of being python | |
| 19:44:47 | efried | What I mean is, is it used by some known external entity that we care about? | |
| 19:44:52 | cdent | and in the early days of placement was the entry point to the wsgi application stack until sean added the stuff in wsgi.py | |
| 19:44:57 | cdent | the point is that we don’t know | |
| 19:45:09 | efried | But we care? | |
| 19:45:13 | cdent | yes, of course | |
| 19:45:18 | efried | Oh, not of course. | |